Skip to content
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

Open
wants to merge 11 commits into
base: bsips-issuer-asset-disable-modify-max-supply
Choose a base branch
from

Conversation

jmjatlanta
Copy link
Contributor

@jmjatlanta jmjatlanta commented Sep 17, 2019

This PR will adjust the language and add a few details to PR #115 to hopefully clarify the desires of the BSIP.

Copy link
Member

@abitmore abitmore left a 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.

bsip-0048.md Outdated Show resolved Hide resolved
bsip-0048.md Outdated Show resolved Hide resolved
@abitmore
Copy link
Member

Also there is a discussion in bitshares/bitshares-core#1375 about adding a disable_issue flag, perhaps also add it in this PR?

And maybe #187?

bsip-0048.md Outdated Show resolved Hide resolved
bsip-0048.md Outdated Show resolved Hide resolved
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.
Copy link
Contributor

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".

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this considered resolved?

Copy link
Member

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.

bsip-0048.md Outdated Show resolved Hide resolved
bsip-0048.md Outdated Show resolved Hide resolved
bsip-0048.md Outdated Show resolved Hide resolved
bsip-0048.md Outdated Show resolved Hide resolved
@abitmore
Copy link
Member

Perhaps address #96 in this BSIP as well?

@jmjatlanta
Copy link
Contributor Author

Perhaps address #96 in this BSIP as well?

I don't mind the extra work, but I feel this BSIP is already larger than the original. Do you feel the specifications of #96 were discussed enough that including it in this PR will not cause delays while details are hashed out?

@abitmore
Copy link
Member

Do you feel the specifications of #96 were discussed enough

IMHO #96 is clear. Thanks.

@ryanRfox
Copy link
Contributor

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 Show resolved Hide resolved
bsip-0048.md Outdated Show resolved Hide resolved
bsip-0048.md Outdated Show resolved Hide resolved
bsip-0048.md Outdated Show resolved Hide resolved
bsip-0048.md Outdated Show resolved Hide resolved
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.
Copy link
Contributor

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?

Copy link
Member

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.

Copy link
Contributor

@pmconrad pmconrad Oct 8, 2019

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.

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Collaborator

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

Copy link
Member

@abitmore abitmore left a 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.

@abitmore abitmore changed the title Update PR 115 modify_max_supply Update PR 115 (BSIP48) modify_max_supply May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants