Added disable_modify_max_supply flag by xeroc · Pull Request #1375 · bitshares/bitshares-core

xeroc · 2018-10-15T07:38:18Z

Implementation proposal for proposed BSIP48
bitshares/bsips#115

xeroc · 2018-10-15T07:39:01Z

libraries/chain/asset_evaluator.cpp

+   // change of the disable_modify_max_supply flag; should only be set from 0 to 1
+   FC_ASSERT( ( old_options.flags & disable_modify_max_supply == o.new_options.flags & disable_modify_max_supply
+           || ( old_options.flags & disable_modify_max_supply ) == 0 ),
+           "The disable_modify_max_supply flag can not be deactivated." );


@Dimfred: could you elaborate what this is about?

So with our flag we want to block the issuer's ability to modify the max_supply of his created asset. However it is optional for the issuer to set the flag. But once set, the flag can't be reset, to guarantee that the promise (to not modify the max_supply) is kept.

So with the first statement old_options.flags & disable_modify_max_supply == o.new_options.flags & disable_modify_max_supply, the flag in the current state of the asset.options (old_options) is compared to the flag in the new state of the asset.options (o.new_options). The statement is true if flag isn't modified. That's a valid option setup.

The second case old_options.flags & disable_modify_max_supply ) == 0 is true when the flag is 0. When it's 1 (modification max_supply disabled) and should be reset ( first statement == false ) the statement will be false, because like said, it is permitted to reset the flag.

I'd recommend use issuer_permission for this kind of rule.

What @abitmore says!

As long as the permission is set (default), the flag can be toggeled at any time.
Once the permission is removed, the flag is frozen indefinitely.

That behavior is already define here:
https://github.com/bitshares/bitshares-core/pull/1375/files/4d50ac6b29875b81b8fed70a135ffc69c444895b#diff-6b07a69176a9fc1a8c6ea1087e4129adR289

Agree.

I would prefer to invert the meaning of the flag though, i. e. call it allow_modify_max_supply. Negative logic is always confusing.

In addition, the permission (and flag, if inverted as suggested) must be enabled for all existing assets.

Yeah you are right, but if we switch the flag to be a non inverted logic, it would be not activated by default. AFAICS all flags are 0 by default. This means for us if we make the flag enable_modify_max_supply the modification would be disabled by default. This would change the current behaviour of the asset_create_operation.

I think it would be possible to set the flag in the asset_create_evaluator::do_apply to make it default on creation, but then there would be more code and it would break the current pattern of having all flags 0 by default. But if you say this is okay, I agree with you.

My assumption is that the newly introduced flat is initialized with 0 for existing assets.

To keep current behavior for existing assets, we have to use a disable_* flag which changes from the current behavior when set.

Else, if we were to add an enable_issue_shares operation, and hard forked over, daily operations of CryptoBridge/OpenLedger/GDEX etc would break, not being able to issue new shares after hard fork.

Hence, the disable_* methodology.

We can automatically set the permission + flag on asset_create and asset_update until the hardfork takes effect. Slower alternative would be to switch them on at the time of the hardfork.

IMO the little extra code is well worth the clarity we gain.

There isn't really a default for the flags/permissions, the value must be specified at asset creation.

@pmconrad again please be aware that the client libraries and all the 3rd-party software that depends on the libraries would need to adapt to the change. If we use the disable_ methodology, unchanged clients will remain old behavior when creating new assets. This may save efforts greatly and avoid potential troubles.

Good point.

xeroc · 2018-10-16T14:39:38Z

We can automatically set the permission + flag on asset_create and asset_update until the hardfork takes effect. Slower alternative would be to switch them on at the time of the hardfork.

IMO the little extra code is well worth the clarity we gain.

There isn't really a default for the flags/permissions, the value must be specified at asset creation.

Sure, but the values used "in the past" have the upper bits all set to 0.

Surely, we can change the code to whatever the core team believes makes the most sense. Any additional feedback? @ryanRfox @abitmore @oxarbitrage

Dimfred · 2018-10-21T11:59:40Z

So I added now a disable_issue flag to the issuer_permissions and changed the code, in a way that it now removes the permissions to modify the disable_issue flag and/or the disable_modify_max_supply flag, when they are set.

Also the max_supply can now be modified when the current_supply = 0.

For the disable_issue flag I have a question. AFAIU this flag should be set when the max_supply of an asset was issued. What if someone accidentally sets this flag before the max_supply is issued? Should the enabling of the flag be permitted before the max_supply is issued?

Here is the current code, would be great if you could review it.
develop...Dimfred:feature/issuer-asset-disable-modify-max-supply

pmconrad · 2018-10-21T16:38:59Z

IMO the disable_issue flag should be independent from max_supply / current_supply. If it is set then no more tokens can be issued.

xeroc · 2018-10-23T07:46:02Z

IMO the disable_issue flag should be independent from max_supply / current_supply. If it is set then no more tokens can be issued.

Yes, it should be! I'll discuss this with @Dimfred again.

Do we all agree that it is easier to keep the disable_* methodology, now?

pmconrad · 2018-10-23T11:55:25Z

Do we all agree that it is easier to keep the disable_* methodology, now?

Yes. </gritting_teeth>

Dimfred · 2018-10-24T07:22:25Z

IMO the disable_issue flag should be independent from max_supply / current_supply. If it is set then no more tokens can be issued.

It is independently. But why should someone create an asset with the disable_issue flag, AFAIU he won't be able to do anything with it, because he cant issue any tokens. So maybe permit the creation with disable_issue` flag.

abitmore · 2018-10-24T11:57:57Z

But why should someone create an asset with the disable_issue flag, AFAIU he won't be able to do anything with it, because he cant issue any tokens.

He can issue sufficient amount first then update the asset to disable the permission, to tell the holders that he won't be able to issue more.

Dimfred · 2018-10-24T12:06:13Z

He can issue sufficient amount first then update the asset to disable the permission, to tell the holders that he won't be able to issue more.

Yes, okay. I didn't understood that the flag and permission are decoupled. I thought they walk hand in hand, like if I set the flag, the permission is removed automatically. But i got it, everything makes sense now.

I changed it, so that the permission has to be explicitly removed.

pmconrad · 2018-10-24T16:21:04Z

But why should someone create an asset with the disable_issue flag

We usually aim to provide tools that are as generally useful as possible. We do not care why someone should do that - we just make sure they can do it if they want to.

…levant variable

pmconrad · 2019-04-10T06:59:37Z

No apparent progress. Closing for now.

xeroc commented Oct 15, 2018

View reviewed changes

abitmore added the hardfork label Oct 25, 2018

Blockchain Projects BV and others added 2 commits February 22, 2019 13:00

Added disable_modify_max_supply flag

d94e162

Remove the test for permissions as that is already there, remove irre…

8c925d0

…levant variable

pmconrad mentioned this pull request Feb 22, 2019

[Hardfork] Added disable_modify_max_supply flag #1612

Closed

xeroc force-pushed the feature/issuer-asset-disable-modify-max-supply branch from f2f2003 to 8c925d0 Compare February 22, 2019 14:16

pmconrad closed this Apr 10, 2019

This was referenced Sep 18, 2019

BSIP 48 Add flag to prevent asset's max supply from being changed bitshares/bsips#115

Open

Update PR 115 (BSIP48) modify_max_supply bitshares/bsips#216

Open

abitmore deleted the feature/issuer-asset-disable-modify-max-supply branch May 14, 2020 20:35

Conversation

xeroc commented Oct 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dimfred Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dimfred Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xeroc commented Oct 16, 2018

Uh oh!

Dimfred commented Oct 21, 2018

Uh oh!

pmconrad commented Oct 21, 2018

Uh oh!

xeroc commented Oct 23, 2018

Uh oh!

pmconrad commented Oct 23, 2018

Uh oh!

Dimfred commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abitmore commented Oct 24, 2018

Uh oh!

Dimfred commented Oct 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmconrad commented Oct 24, 2018

Uh oh!

pmconrad commented Apr 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Dimfred Oct 15, 2018 •

edited

Loading

Dimfred Oct 16, 2018 •

edited

Loading

Dimfred commented Oct 24, 2018 •

edited

Loading

Dimfred commented Oct 24, 2018 •

edited

Loading