Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Balance to Asset Balance Conversion#9076

Merged
12 commits merged intomasterfrom
apopiak/master-balance-conversion
Jul 20, 2021
Merged

Balance to Asset Balance Conversion#9076
12 commits merged intomasterfrom
apopiak/master-balance-conversion

Conversation

@apopiak
Copy link
Contributor

@apopiak apopiak commented Jun 10, 2021

This PR adds the BalanceConversion trait and BalanceToAssetBalance type to pallet_assets to allow converting native balances to asset specific ones.
This is a "low resolution" conversion based on the existential deposit (of the native currency) and the min_balance (of the asset).

Note: The implementation actually expects a generic fungible, so the incoming balance could theoretically be an asset as well. (So the same implementation could work for chains that only have the assets pallet.)

PR also adds some derives for Imbalance that seemed useful but are unconnected to the conversion.

needed for: paritytech/cumulus#488

supersedes #8776

@apopiak apopiak added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jun 10, 2021
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

LGTM, but I am not really familiar with its usage on cumulus yet.

@apopiak apopiak added the D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited label Jun 11, 2021
@apopiak
Copy link
Contributor Author

apopiak commented Jun 11, 2021

I guess this needs an audit?
The particular changes are pretty innocuous, but will be relevant for transaction payments?

@apopiak
Copy link
Contributor Author

apopiak commented Jul 8, 2021

Should the trait live in frame_support?

@shawntabrizi
Copy link
Member

@emostov emostov self-requested a review July 15, 2021 17:37
@emostov
Copy link
Contributor

emostov commented Jul 15, 2021

Code itself lgtm. But what is the use case and the justification for using min_balance of assets to establish a conversion rate? From what I understand, in order to faithfully use this conversion one must assume the min_balance between two assets is equivalent in relative "value" - is this the correct way to think about it?

@emostov emostov removed their request for review July 15, 2021 18:42
Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

code is good to me,

but test needs to fixed, CI doesn't compile them

@gui1117
Copy link
Contributor

gui1117 commented Jul 16, 2021

But what is the use case and the justification for using min_balance of assets to establish a conversion rate? From what I understand, in order to faithfully use this conversion one must assume the min_balance between two assets is equivalent in relative "value" - is this the correct way to think about it?

Yes I think this is the correct usecase, when min_balance hold a relatively same value.
Usually the minimum balance cover the cost to have the balance and other account information on chain.

@apopiak
Copy link
Contributor Author

apopiak commented Jul 20, 2021

bot merge

@ghost
Copy link

ghost commented Jul 20, 2021

Trying merge.

@ghost ghost merged commit f31dff7 into master Jul 20, 2021
@ghost ghost deleted the apopiak/master-balance-conversion branch July 20, 2021 14:55
@redzsina redzsina added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Aug 17, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants