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

denominated units for balances in events #750

Merged
merged 26 commits into from
Oct 28, 2022
Merged

Conversation

SkymanOne
Copy link
Contributor

@SkymanOne SkymanOne commented Sep 19, 2022

Closes #673

Example of denomination of balances when running a call on substrate contracts node:

       Event Balances ➜ Transfer
         from: 5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY
         to: 5FUdyfmrdbk1H5F1E4HnweiQjENqwAWnPxH2Mkty6LeK5at9
         amount: 100.2nDOT
       Event Balances ➜ Reserved
         who: 5FUdyfmrdbk1H5F1E4HnweiQjENqwAWnPxH2Mkty6LeK5at9
         amount: 100.2nDOT
       Event Balances ➜ Deposit
         who: 5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY
         amount: 188.457902μDOT
       Event TransactionPayment ➜ TransactionFeePaid
         who: 5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY
         actual_fee: 4.23927238nDOT
         tip: 0DOT

Update

Balances can also be inputed in a denominated format (i.e. 500MDOT for 500 x 10^6 x 10^<tokenDecimals>DOT).
The token symbol must match the one specified in the running local chain. If the chain config does not have a token symbol and/or decimals. UNIT is used as a default symbol and 12 decimals are used for denomination

This includes:

  • --value for call and instantiate commands
  • --storage_deposit_limit for any command

Includes fixes addressed in #751

@SkymanOne SkymanOne added the blocked This task is blocked until blockers are resolved. label Sep 19, 2022
@SkymanOne
Copy link
Contributor Author

Currently blocked by #654 in subxt repo

@SkymanOne SkymanOne self-assigned this Sep 20, 2022
@SkymanOne
Copy link
Contributor Author

Updated documentation will follow after the approval from code owners

@SkymanOne SkymanOne marked this pull request as ready for review September 21, 2022 17:58
@SkymanOne SkymanOne removed the blocked This task is blocked until blockers are resolved. label Sep 22, 2022
Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

The main thing is the code in balance.rs needs to be tightened up a bit, needs more tests since there are bugs there.

Also I think we are missing decoding Balance for contract events, for that the decode_contract_event would need to check for fields with the type name Balance: https://github.com/paritytech/cargo-contract/blob/05494e2bd62b48c6b79b99bd91c0657ee8656103/crates/transcode/src/lib.rs#L250

crates/cargo-contract/src/cmd/extrinsics/balance.rs Outdated Show resolved Hide resolved
crates/cargo-contract/src/cmd/extrinsics/balance.rs Outdated Show resolved Hide resolved
@SkymanOne SkymanOne requested review from ascjones and removed request for ascjones September 28, 2022 21:25
@SkymanOne SkymanOne requested review from cmichi and ascjones and removed request for cmichi and HCastano September 30, 2022 10:04
Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

The handling of the decimal parsing is much better now 👍

The conversion from raw u128 to DenominatedBalance needs some testing and tidying up.

One big thing that looks to be missing is the handling of Balance values when actually interacting with contracts. We should be able to use a denominated balance when invoking e.g. https://github.com/paritytech/ink/blob/master/examples/erc20/lib.rs#L130.

And also when displaying the resulting contract Transfer event: https://github.com/paritytech/ink/blob/master/examples/erc20/lib.rs#L27.

This needs to be done in https://github.com/paritytech/cargo-contract/blob/05494e2bd62b48c6b79b99bd91c0657ee8656103/crates/transcode/src/lib.rs#L250

crates/cargo-contract/src/cmd/extrinsics/balance.rs Outdated Show resolved Hide resolved
crates/cargo-contract/src/cmd/extrinsics/balance.rs Outdated Show resolved Hide resolved
crates/cargo-contract/src/cmd/extrinsics/balance.rs Outdated Show resolved Hide resolved
crates/cargo-contract/src/cmd/extrinsics/balance.rs Outdated Show resolved Hide resolved
crates/cargo-contract/src/cmd/extrinsics/balance.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

BalanceVariant::from still needs some tidying up.

For some inspiration, have a look at formatBalance that polkadot.js uses. It does so in a fairly concise manner. See e.g. calcSi

crates/cargo-contract/src/cmd/extrinsics/balance.rs Outdated Show resolved Hide resolved
crates/cargo-contract/src/cmd/extrinsics/balance.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM

crates/cargo-contract/src/cmd/extrinsics/balance.rs Outdated Show resolved Hide resolved
@SkymanOne SkymanOne merged commit f6c5269 into master Oct 28, 2022
@SkymanOne SkymanOne deleted the gn-denominated-units branch October 28, 2022 16:54
@ascjones ascjones mentioned this pull request Nov 22, 2022
@ascjones ascjones mentioned this pull request Feb 14, 2023
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.

Denominated units for Balances
2 participants