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

Add BalanceToAssetBalance Conversion Type #8776

Closed
wants to merge 5 commits into from

Conversation

apopiak
Copy link
Contributor

@apopiak apopiak commented May 10, 2021

This PR adds the 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).

needed (AFAICT) for paritytech/statemint#51

Questions:

  • Should I add a trait (so that it can be injected as an associated type)?

converts based on a given existential deposit and the assets min_balance
@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 May 10, 2021
Comment on lines 203 to 207
impl<T: Config, Balance, ED> BalanceToAssetBalance<T, Balance, ED>
where
Balance: Into<<T as Config>::Balance> + FixedPointOperand + Ord + One,
<T as Config>::Balance: FixedPointOperand,
ED: Get<Balance>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
impl<T: Config, Balance, ED> BalanceToAssetBalance<T, Balance, ED>
where
Balance: Into<<T as Config>::Balance> + FixedPointOperand + Ord + One,
<T as Config>::Balance: FixedPointOperand,
ED: Get<Balance>,
impl<T, Balance, ED> BalanceToAssetBalance<T, Balance, ED>
where
T: Config,
<T as Config>::Balance: FixedPointOperand,
Balance: Into<<T as Config>::Balance> + FixedPointOperand + Ord + One,
ED: Get<Balance>,

/// deposit and the minimum asset balance.
///
/// Will return `Err` if the asset is not found or not sufficient.
pub fn to_asset_balance(balance: Balance, asset_id: <T as Config>::AssetId) -> Result<<T as Config>::Balance, ConversionError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

if there's no trait to be fulfilled, you might as well just expose a function rather than a struct.

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.

  1. Why does this need to be here in substrate? seems like the type of thing that should live where it is used.

  2. If so, I'd rewrite it as:

/// Convert from the native balance of a currency (e.g. an instance of [`pallet_balances`]) 
/// to the native balance of an asset pallet. 
fn balance_to_asset<Balance: Currency, Asset: Config>(from: Balances::Balance, to: Asset::AssetId) 
where
	Balances::Balance: ...,
	T::Balance: ...,
{
	let ed = Balances::minimum_balance();
	// blah blah
}

@apopiak
Copy link
Contributor Author

apopiak commented May 11, 2021

  1. Why does this need to be here in substrate? seems like the type of thing that should live where it is used.

Because it accesses private state (namely the Asset storage).

@apopiak
Copy link
Contributor Author

apopiak commented May 11, 2021

I could access min_balance via Inspect::minimum_balance() but AFAICT is_sufficient is not exposed via a trait.

@kianenigma
Copy link
Contributor

Can you provide a branch where this is used in it? or pseudo code of how it should fit in the mentioned PR.

@apopiak
Copy link
Contributor Author

apopiak commented May 11, 2021

@apopiak
Copy link
Contributor Author

apopiak commented May 11, 2021

(branch does not compile yet)

@paritytech paritytech deleted a comment from kianenigma May 11, 2021
@apopiak
Copy link
Contributor Author

apopiak commented Jun 10, 2021

superseded by #9076

@apopiak apopiak closed this Jun 10, 2021
@apopiak apopiak deleted the apopiak-to-asset-balance branch June 10, 2021 16:15
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants