search

bitshares / bitshares-ui

Fully featured Graphical User Interface / Reference Wallet for the BitShares Blockchain
https://wallet.bitshares.org
MIT License
518 stars 570 forks source link

Incorrect implementation of the disable-bit smartcoin issuer permissions & flags? #3732

Open grctest opened 1 week ago

grctest commented 1 week ago

Describe the bug

The new disable-bit permissions are as follows:

// If one of the following bits is set in asset issuer permissions,
// it means the asset issuer (or owner for bitassets) does NOT have the permission to update
// the corresponding flag, parameters or perform certain actions.
// This is to be compatible with old client software.
// Note: This comment is copied and reformatted above for better Doxygen documentation formatting.
lock_max_supply      = 0x200, ///< the max supply of the asset can not be updated
disable_new_supply   = 0x400, ///< unable to create new supply for the asset
// For disable_mcr_update, disable_icr_update and disable_mssr_update,
// if one of these is set in asset issuer permissions, and
// - if the bitasset owner has set a value for the corresponding parameter, the value can not be updated,
// - if the bitasset owner has not set a value for the corresponding parameter, the parameter can still be
//   updated by the price feed producers.
// Note: This comment is copied and reformatted above for better Doxygen documentation formatting.
disable_mcr_update   = 0x800, ///< the bitasset owner can not update MCR, permission only
disable_icr_update   = 0x1000, ///< the bitasset owner can not update ICR, permission only
disable_mssr_update  = 0x2000, ///< the bitasset owner can not update MSSR, permission only
disable_bsrm_update  = 0x4000, ///< the bitasset owner can not update BSRM, permission only
disable_collateral_bidding = 0x8000  ///< Can not bid collateral after a global settlement

So, if my understanding of the above is correct, then the following should be true:

This applies for the rest of these disable-bit flags, so when the toggle is disabled, the flag should become available to edit, right?

When you disable the issuer permission, the corresponding flag is missing, you're therefore unable to enable the flag to enable the new feature?

To Reproduce Steps to reproduce the behavior:

Expected behavior

Unlike the enable-bit issuer permissions, the disable-bit issuer permissions should result in the opposite behaviour in the flags - they should be visible and editable flags when their disable-bit issuer permission has been disabled.

Additional context

Core code reference: https://github.com/bitshares/bitshares-core/blob/3174d22b5267d0057d0857ac53086e9813c8559f/libraries/protocol/include/graphene/protocol/types.hpp#L205

This is the point where the disabled disable-bit permission fails to be included as a configurable flag: https://github.com/bitshares/bitshares-ui/blob/cba882168c99327e068b2ee23e5ccf8d704a589a/app/components/Account/AccountAssetCreate.jsx#L771

The double-negative terminology is confusing.

Misconfiguration of these disable-bit's corresponding flags could be preventing users from creating new smartcoins.

abitmore commented 1 week ago

I confirm that the current implementation is incorrect. When creating an asset with the "SmartCoin" option toggled on, if leave all permissions enabled (which is the default), the issuer permission value in the transaction is now 65535. The correct value should be 511.

image

grctest commented 1 week ago

Working on the astro-ui, having all issuer permission toggles set to true (enable-bits all enabled, disable-bits all actively disabling) then I too get 65535, only when I set all enable-bits to true, and all disable-bits to false toggles do I get 511 for issuer_permissions.

  "issuer_permissions": 65535,
  "flags": 0,

Do you mean that the disable-bits should only contribute their following values to the issuer_permission/flag calculations when their toggles have been set to false?

    lock_max_supply      = 0x200
    disable_new_supply   = 0x400
    disable_mcr_update   = 0x800
    disable_icr_update   = 0x1000
    disable_mssr_update  = 0x2000
    disable_bsrm_update  = 0x4000
    disable_collateral_bidding = 0x8000

Code for how I calculate issuer permissions: https://github.com/BTS-CM/astro-ui/blob/main/src/lib/common.js#L140

grctest commented 1 week ago

Another related query - should we be offering a global settlement flag to configure?

global_settle = 0x20, ///< allow the bitasset owner to force a global settlement, permission only

The docs state this is a permission only, so the flag should be permanently set to false, right? The asset issuer can perform the asset_global_settle with the permission set to true & its associated flag set to false?

Currently the reference UI offers the global_settle flag for possible activation, if its associated enable-bit permission is true, which I believe will cause the asset_create operation to be rejected by the network, right?

abitmore commented 1 week ago

For permission only bits, the flag can only be set to 0.

https://github.com/bitshares/bitshares-core/blob/d069614135b3e7a3661a0ebb64ee0dc6b4d4b4f4/libraries/protocol/asset_ops.cpp#L287-L289

   // The permission-only bits can not be set in flag
   FC_ASSERT( 0 == (flags & global_settle),
              "Can not set global_settle flag, it is for issuer permission only" );

https://github.com/bitshares/bitshares-core/blob/d069614135b3e7a3661a0ebb64ee0dc6b4d4b4f4/libraries/protocol/asset_ops.cpp#L320-L328

   FC_ASSERT( 0 == (flags & disable_mcr_update),
              "Can not set disable_mcr_update flag, it is for issuer permission only" );
   FC_ASSERT( 0 == (flags & disable_icr_update),
              "Can not set disable_icr_update flag, it is for issuer permission only" );
   FC_ASSERT( 0 == (flags & disable_mssr_update),
              "Can not set disable_mssr_update flag, it is for issuer permission only" );
   FC_ASSERT( 0 == (flags & disable_bsrm_update),
              "Can not set disable_bsrm_update flag, it is for issuer permission only" );
abitmore commented 1 week ago 
  "issuer_permissions": 511,

Generally speaking, setting the issuer permissions to 511 means that the issuer is allowed to do everything, which is the default value set by older versions of bitshares-ui. New versions should retain this behavior and be compatible.

By "allowed" I mean that the issuer has not waived this right.

grctest commented 1 week ago

Note: Editing in response to feedback

Issuer Permission UI Checkbox Value Asset Effect Resulting Hexadecimal Literal Value
charge_market_fee true (1) Able to configure market trading fees 0x01
charge_market_fee false (0) Unable to configure market trading fees 0
white_list true (1) Able to configure white list authorities 0x02
white_list false (0) Unable to configure white list authorities 0
override_authority true (1) Able to perform override transfer operations 0x04
override_authority false (0) Unable to perform override transfer operations 0
transfer_restricted true (1) Able to restrict transfers 0x08
transfer_restricted false (0) Unable to restrict transfers 0
disable_confidential true (1) Able to disable confidential operations 0x10
disable_confidential false (0) Unable to disable confidential operations 0
witness_fed_asset true (1) Able to enable witnesses to publish price feeds 0x20
witness_fed_asset false (0) Unable to enable witnesses to publish price feeds 0
committee_fed_asset true (1) Able to enable committee members to publish price feeds 0x40
committee_fed_asset false (0) Unable to enable committee members to publish price feeds 0
disable_force_settle true (1) Able to disable force settling 0x80
disable_force_settle false (0) Unable to disable force settling 0
global_settle true (1) Global settlement allowed 0x100
global_settle false (0) Global settlement disabled 0
lock_max_supply true (1) Unable to lock max supply 0x200
lock_max_supply false (0) Able to lock max supply 0
disable_new_supply true (1) Unable to disable new supply 0x400
disable_new_supply false (0) Able to disable new supply 0
disable_collateral_bidding true (1) Unable to disable collateral bidding 0x8000
disable_collateral_bidding false (0) Able to disable collateral bidding 0
disable_mcr_update true (1) Cannot update static MCR 0x800
disable_mcr_update false (0) Can update MCR 0
disable_icr_update true (1) Cannot update static ICR 0x1000
disable_icr_update false (0) Can update ICR 0
disable_mssr_update true (1) Cannot update static MSSR 0x2000
disable_mssr_update false (0) Can update MSSR 0
disable_bsrm_update true (1) Unable to set a Black Swan Recovery Method 0x4000
disable_bsrm_update false (0) Able to set Black Swan Recovery Method 0
Issuer Flag UI Checkbox Value Asset Effect Resulting Hexadecimal Literal Value
charge_market_fee true (1) Market fees enabled 0x01
charge_market_fee false (0) Market fees disabled 0
white_list true (1) White list enabled 0x02
white_list false (0) White list disabled 0
override_authority true (1) Override authority enabled 0x04
override_authority false (0) Override authority disabled 0
disable_force_settle true (1) Force settle disabled 0x10
disable_force_settle false (0) Force settle enabled 0
transfer_restricted true (1) Transfer restricted enabled 0x08
transfer_restricted false (0) Transfer restricted disabled 0
disable_confidential true (1) Disable confidential enabled 0x40
disable_confidential false (0) Disable confidential disabled 0
witness_fed_asset true (1) Witness fed asset enabled 0x80
witness_fed_asset false (0) Witness fed asset disabled 0
committee_fed_asset true (1) Committee fed asset enabled 0x100
committee_fed_asset false (0) Committee fed asset disabled 0
lock_max_supply true (1) Maximum supply is locked 0x200
lock_max_supply false (0) Maximum supply is unlocked 0
disable_new_supply true (1) Unable to issue new supply 0x400
disable_new_supply false (0) Able to issue new supply 0
disable_collateral_bidding true (1) Collateral bidding disabled 0x8000
disable_collateral_bidding false (0) Collateral bidding permitted 0

This is how I am currently understanding it, the reason why 511 isn't what's being calculated is because each of the new disable-bit issuer permissions are providing their hex value towards the final value when default checked true (which is by default disabling the feature)

Should it be the opposite, ie disable_mcr_update true = 0 and false = 0x800 then?

abitmore commented 1 week ago

issuer permission UI checkbox value asset effect resulting hexadecimal literal value lock_max_supply true (checked) Unable to lock max supply. 0x200

Not exactly.

lock_max_supply false (unchecked) Able to lock max supply. 0 disable_new_supply true Unable to configure flag. 0x400

Correct. Others should be inline with this narrative.

disable_new_supply false Able to configure flag. 0 disable_mcr_update true Unable to set static MCR. 0x800

Not exactly. (Also the same for the following 3.)

disable_mcr_update false Able to set static MCR 0 disable_icr_update true Unable to set static ICR 0x1000 disable_icr_update false Able to set static ICR. 0 disable_mssr_update true Unable to set static MSSR 0x2000 disable_mssr_update false Able to set static MSSR 0 disable_bsrm_update true Unable to set BSRM strategy. 0x4000 disable_bsrm_update false Able to set BSRM strategy 0 disable_collateral_bidding true Unable to configure flag 0x8000

Correct.

disable_collateral_bidding false Able to configure flag 0

issuer flags UI checkbox value asset effect resulting hexadecimal literal value lock_max_supply true Locked max supply 0x200 lock_max_supply false Unlocked max supply 0 disable_new_supply true Locked new supply 0x400 disable_new_supply false Unlocked new supply 0 disable_collateral_bidding true Disabled collateral bidding 0x8000 disable_collateral_bidding false Enabled collateral bidding 0

This is how I am currently understanding it, the reason why 511 isn't what's being calculated is because each of the new disable-bit issuer permissions are providing their hex value towards the final value when default checked true (which is by default disabling the feature)

Should it be the opposite, ie disable_mcr_update true = 0 and false = 0x800 then?

By default all permissions should be enable. That being said, the enable-bits should all be 1, and the disable-bits should all be 0. (I don't use "true" or "false" to describe it.)

abitmore commented 1 week ago

Why "Unable to set static MCR" is not correct? Because the underlying logic is "Unable to UPDATE MCR if static MCR is already set; and unable to set static MCR if static MCR is not already set".

See https://github.com/bitshares/bitshares-core/blob/d069614135b3e7a3661a0ebb64ee0dc6b4d4b4f4/libraries/protocol/include/graphene/protocol/types.hpp#L213-L222

    // For disable_mcr_update, disable_icr_update and disable_mssr_update,
    // if one of these is set in asset issuer permissions, and
    // - if the bitasset owner has set a value for the corresponding parameter, the value can not be updated,
    // - if the bitasset owner has not set a value for the corresponding parameter, the parameter can still be
    //   updated by the price feed producers.
    // Note: This comment is copied and reformatted above for better Doxygen documentation formatting.
    disable_mcr_update   = 0x800, ///< the bitasset owner can not update MCR, permission only
    disable_icr_update   = 0x1000, ///< the bitasset owner can not update ICR, permission only
    disable_mssr_update  = 0x2000, ///< the bitasset owner can not update MSSR, permission only
    disable_bsrm_update  = 0x4000, ///< the bitasset owner can not update BSRM, permission only
grctest commented 1 week ago

I've continued to edit my posts above with my latest understanding of the permissions/flags/locales

By default all permissions should be enable. That being said, the enable-bits should all be 1, and the disable-bits should all be 0. (I don't use "true" or "false" to describe it.)

I used true to represent a green checked active checkbox in the UI & false to represent the grey unchecked checkbox, in Javascript both 1 == true & 0 == false, however I've added 1's & 0's to the table to make it clearer.

With regards to what you mean above that disable-bits should all be 0, the current UI implementation of a disable-bit is as follows:

Issuer Permission UI Checkbox Value Asset Effect Resulting Hexadecimal Literal Value
disable_collateral_bidding true (1) Unable to configure flag 0x8000
disable_collateral_bidding false (0) Able to configure flag 0

Do you mean it should be as follows instead for each of the disable-bit issuer permissions?

Issuer Permission UI Checkbox Value Asset Effect Resulting Hexadecimal Literal Value
disable_collateral_bidding true (0) Unable to configure flag 0
disable_collateral_bidding false (1) Able to configure flag 0x8000

However, for a disable-bit issuer permission's relevant flag it should remain the other way around, like so since it's an enable-bit flag instead of a disable-bit flag?

Flag UI Checkbox Value Asset Effect Resulting Hexadecimal Literal Value
disable_collateral_bidding true (1) Collateral bidding permitted 0x8000
disable_collateral_bidding false (0) Collateral bidding disabled 0

A follow up question also is that when you disable an enable-bit issuer permission it's a permanent move if the supply is greater than 0, is the reverse true for disable-bit issuer permissions that the move from 0 to 1 is a permanent issuer permission configuration?

abitmore commented 1 week ago

By 1 or 0 I actually mean the "Resulting Hexadecimal Literal Value".

abitmore commented 1 week ago

Issuer Permission ... disable_new_supply true Unable to configure flag. 0x400 disable_new_supply false Able to configure flag. 0

As I've said these kind of description was correct.

Issuer Permission ... disable_new_supply true (1) Unable to disable new supply 0x400 disable_new_supply false (0) Able to disable new supply 0

I say these were "not exactly" which means they don't include all the underlying logic. In other words, these were wrong.

abitmore commented 1 week ago

However, for a disable-bit issuer permission's relevant flag it should remain the other way around, like so since it's an enable-bit flag instead of a disable-bit flag?

Yes. Bits in flags are all enable-bits. However please note that some bits are to disable something..

A follow up question also is that when you disable an enable-bit issuer permission it's a permanent move if the supply is greater than 0, is the reverse true for disable-bit issuer permissions that the move from 0 to 1 is a permanent issuer permission configuration?

Yes.

abitmore commented 1 week ago

image

grctest commented 1 week ago

I've updated the asset effects tables for reference

So, when you want all the disable-bits enabled by default, you want 511 issuer permission, but when you want all enable-bits + all disable-bits disabled, what issuer permission value are you expecting? 65535

An exhaustive list of every permission value + effect + hex value for issuer permissions and flags would be greatly appreciated

@sschiessl-bcp @gibbsfromncis @HarukaMa @svk31 @startailcoon Any thoughts on how we should implement the required changes in the reference UI?

abitmore commented 1 week ago

So, when you want all the disable-bits enabled by default, you want 511 issuer permission, but when you want all enable-bits + all disable-bits disabled, what issuer permission value are you expecting? 65535

To be clear, I want all FEATURES to be "can be enabled" by default, not every bit set to 1 by default. It means I want enable-bits be 1 and disable-bits be 0 by default, thus 511.

abitmore commented 1 week ago

The permissions and flags are both 16 bit in data storage (please think in binary form). Each bit represents a feature. The right-most bit represents "enable market fee", the left-most bit represents "disable collateral bidding", etc.

When I say a bit to be 0 or 1, I literally mean that one bit in binary form to store 0 or 1.

I want the default permissions to be 0000 0001 1111 1111 in binary form which is 511 in decimal form. It means all the features CAN BE ENABLED by the asset issuer/owner.

I want the default flags to be 0. It means all the features are disabled.

For UI/UX, I want to avoid "double negatives" for simplicity and clarity. Because typical users don't care how the data is stored, they only care about functionalities. The code should correctly translate what the user decides on the UI into data, and that's what programmers do.

I will add a data table later.

grctest commented 1 week ago

I've changed the Astro UX to use the same permission/flag wording as the reference UI, minus expanding the acronyms to full wording to avoid translation into incorrect acronym definitions, so the user isn't affected by the double negative terminology.

image

Now that I've adjusted the issuer permission to the new spec, the above yields:

"issuer_permissions": 511,
"flags": 0,

image The above yields:

"issuer_permissions": 65535,
"flags": 1631,
abitmore commented 1 week ago

Asset Issuer Permissions

Issuer Permission Which bit in data storage Value in binary form Value in Hex form Asset Effect Default
charge_market_fee ???? ???? ???? ???* ???? ???? ???? ???1 0x01 The asset issuer/owner can update the charge_market_fee flag (see the Flags section for more info) Yes
charge_market_fee ???? ???? ???? ???* ???? ???? ???? ???0 0 The asset issuer/owner can NOT update the charge_market_fee flag (see the Flags section for more info). Note: if the charge_market_fee flag is set to 0, can no longer start charging market trading fees; if the charge_market_fee flag is set to non-zero, the market trading fee rates can still be updated to 0 or non-zero at any time so that trading is not charged. -
white_list ???? ???? ???? ??*? ???? ???? ???? ??1? 0x02 The asset issuer/owner can update can update the white_list flag (see the Flags section for more info) Yes
white_list ???? ???? ???? ??*? ???? ???? ???? ??0? 0 The asset issuer/owner can NOT update the white_list flag (see the Flags section for more info). Note: if the white_list flag is set to 0, can no longer start enforcing whitelisting or blacklisting; if the white_list flag is set to non-zero, can still configure to not enforce whitelisting or blacklisting at any time by clearing the whitelisting and blacklisting authorities. -

(To be continued)

Asset Flags

Flag Which bit in data storage Value in binary form Value in Hex form Asset Effect Default
charge_market_fee ???? ???? ???? ???* ???? ???? ???? ???1 0x01 Market trades in this asset may be charged, depending on the fee rate settings. UI/UX decide
charge_market_fee ???? ???? ???? ???* ???? ???? ???? ???0 0 Market trades in this asset are not charged UI/UX decide
white_list ???? ???? ???? ??*? ???? ???? ???? ??1? 0x02 The use of this asset may be restricted by asset authorization (whitelist and/or blacklist), depending on the settings of whitelisting and blacklisting authorities. UI/UX decide
white_list ???? ???? ???? ??*? ???? ???? ???? ??0? 0 The use of this asset is not restricted by asset authorization (whitelist and/or blacklist) (but other restrictions may still apply) UI/UX decide

(To be continued)

abitmore commented 1 week ago

I've changed the Astro UX to use the same permission/flag wording as the reference UI, minus expanding the acronyms to full wording to avoid translation into incorrect acronym definitions, so the user isn't affected by the double negative terminology.

I don't think wording in the reference UI is perfect, but it is a good start. Thanks. We know the system is complicated and hard to make perfect UI/UX.