-
Notifications
You must be signed in to change notification settings - Fork 648
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added disable_modify_max_supply flag #1375
Conversation
libraries/chain/asset_evaluator.cpp
Outdated
// 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." ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dimfred: could you elaborate what this is about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend use issuer_permission for this kind of rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
Sure, but the values used "in the past" have the upper bits all set to Surely, we can change the code to whatever the core team believes makes the most sense. Any additional feedback? @ryanRfox @abitmore @oxarbitrage |
So I added now a Also the For the Here is the current code, would be great if you could review it. |
IMO the disable_issue flag should be independent from |
Yes, it should be! I'll discuss this with @Dimfred again. Do we all agree that it is easier to keep the |
Yes. </gritting_teeth> |
It is independently. But why should someone create an asset with the |
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. |
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. |
f2f2003
to
8c925d0
Compare
No apparent progress. Closing for now. |
Implementation proposal for proposed BSIP48
bitshares/bsips#115