-
Notifications
You must be signed in to change notification settings - Fork 86
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
Update PR 115 (BSIP48) modify_max_supply #216
base: bsips-issuer-asset-disable-modify-max-supply
Are you sure you want to change the base?
Update PR 115 (BSIP48) modify_max_supply #216
Conversation
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.
Maybe there are more typos.
Also there is a discussion in bitshares/bitshares-core#1375 about adding a And maybe #187? |
bsip-0048.md
Outdated
The `disable_modify_max_supply` is a new created flag in the `asset_issuer_permissions`. | ||
The flag can be activated to forbid the modification of the `max_supply` (`asset_object.options`). | ||
Once the flag is activated, it can't be deactivated. | ||
The `disable_modify_max_supply` is a newly created flag in the `asset_issuer_permissions`. The flag can be activated to forbid the modification of the `max_supply` (`asset_object.options`). Once the flag is set to `true`, it can only be set to `false` if `current_supply` is zero. |
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.
Please be more specific. "flags" are different from "permissions". "permission" implies the permission to change the flag value, whereas the "flag" implies a specific restriction.
Same for "disable_issue".
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.
Let me see if I understand you correctly. From the programmer standpoint, disable_modify_max_supply is a flag, as it is a bit. But that muddies the waters, as others see it as a "permission". So I should re-word the paragraph to make disable_modify_max_supply
to be a "permission", as it is part of the "asset_issuer_permissions".
If the above is correct, I will modify the paragraph below as well.
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 I mean is this: https://github.com/bitshares/bitshares-core/blob/master/libraries/protocol/include/graphene/protocol/asset_ops.hpp#L56-L59
Assets have both flags and permissions. The flag
tells what is currently allowed. The permission
tells if the flag setting can be changed.
An inactive permission
can only be enabled when supply == 0
.
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.
The lightbulb just went on. Thanks for flipping the switch.
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.
Is this considered resolved?
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.
It's partially resolved. In the last commit one sentence of two got updated.
Perhaps address #96 in this BSIP as well? |
I agree #96 is clear by now. However, I prefer to have it drafted as a distinct BSIP, rather than bundled with this one. @jmjatlanta are you willing to assist with drafting that BSIP, else I can find another resource to do so. |
bsip-0048.md
Outdated
The `disable_modify_max_supply` is a new created flag in the `asset_issuer_permissions`. | ||
The flag can be activated to forbid the modification of the `max_supply` (`asset_object.options`). | ||
Once the flag is activated, it can't be deactivated. | ||
`disable_modify_max_supply` will be added to `asset_issuer_permissions`. The permission can be activated to forbid the modification of the `max_supply` (`asset_object.options`). The related flag can only be set to `false` if `current_supply` is zero. |
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 think you have mixed up the meaning of permissions and flags.
This negative logic puts knots in my brain just from reading the BSIP. It will also be a PITA to implement. I think we should move these flags to positive logic. How many clients for creating assets are out there that might break?
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.
How many clients for creating assets are out there that might break?
All clients.
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 think requiring clients to upgrade is better than being forever stuck with this twisted logic.
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 think I clarified my permissions/flags statement. Please check. I have also switched to using positive logic. See 858b7ee
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 don't think it's the way to go.
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 think it's better to revert to negative logic.
I agree here. If that twists user's minds we can present it positively in UI
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 think it's better to revert to negative logic.
This PR will adjust the language and add a few details to PR #115 to hopefully clarify the desires of the BSIP.