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

refactor: inconsistent BalanceConversion fn #13610

Merged
merged 5 commits into from
Apr 4, 2023
Merged

refactor: inconsistent BalanceConversion fn #13610

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1177877
refactor: inconsistent BalanceConversion fn
wischli Mar 15, 2023
7dcc60a
Revert "refactor: inconsistent BalanceConversion fn"
wischli Mar 28, 2023
b0e812a
refactor: rename BalanceConversion trait
wischli Mar 28, 2023
5aec63f
feat: add ConversionFromAssetBalance
wischli Mar 28, 2023
2a7d183
Merge remote-tracking branch 'origin' into refactor/balance-conversion
wischli Apr 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions frame/assets/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1164,19 +1164,16 @@ fn balance_conversion_should_work() {
assert_ok!(Assets::force_create(RuntimeOrigin::root(), not_sufficient, 1, false, 10));
assert_eq!(asset_ids(), vec![23, 42, 999]);
assert_eq!(
BalanceToAssetBalance::<Balances, Test, ConvertInto>::to_asset_balance(100, 1234),
BalanceToAssetBalance::<Balances, Test, ConvertInto>::convert(100, 1234),
Err(ConversionError::AssetMissing)
);
assert_eq!(
BalanceToAssetBalance::<Balances, Test, ConvertInto>::to_asset_balance(
100,
not_sufficient
),
BalanceToAssetBalance::<Balances, Test, ConvertInto>::convert(100, not_sufficient),
Err(ConversionError::AssetNotSufficient)
);
// 10 / 1 == 10 -> the conversion should 10x the value
assert_eq!(
BalanceToAssetBalance::<Balances, Test, ConvertInto>::to_asset_balance(100, id),
BalanceToAssetBalance::<Balances, Test, ConvertInto>::convert(100, id),
Ok(100 * 10)
);
});
Expand Down
2 changes: 1 addition & 1 deletion frame/assets/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ where
///
/// Will return `Err` if the asset is not found, not sufficient or the fungible's minimum
/// balance is zero.
fn to_asset_balance(
fn convert(
balance: BalanceOf<F, T>,
asset_id: AssetIdOf<T, I>,
) -> Result<AssetBalanceOf<T, I>, ConversionError> {
Expand Down
5 changes: 3 additions & 2 deletions frame/support/src/traits/tokens/misc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,10 +189,11 @@ impl<
{
}

/// Converts a balance value into an asset balance.
/// Provides a conversion mechanism between two balances.
/// The most natural conversion would be native to asset balance.
pub trait BalanceConversion<InBalance, AssetId, OutBalance> {
type Error;
fn to_asset_balance(balance: InBalance, asset_id: AssetId) -> Result<OutBalance, Self::Error>;
fn convert(balance: InBalance, asset_id: AssetId) -> Result<OutBalance, Self::Error>;
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 what you are doing in #13608 PR, is different from what this meant for.
Here, the assets id, is id to which the in_balance should be converted, where in you example, its id from which it's being converted.
With updated function signature, the implementation can mean both, the id of in_balance, or the id of out_balance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With updated function signature, the implementation can mean both, the id of in_balance, or the id of out_balance.

IMO, that should be fine and expected of such a generic trait. Otherwise, its name should be more restricted (e.g. Option B) or it should provide a from_asset_balance (Option A).

Copy link
Contributor

Choose a reason for hiding this comment

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

I would not do option A, to not break the api and to follow the segregation principle.
For option B, I would not change the name of the existing trait and name new trait as AssetConversion for example.
This way we don't break existing APIs, and it look clear enough to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tonyalaribe what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

We already have the Convert trait. I would also stick to the old name to_asset_balance. But yeah, maybe the trait should be renamed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you are preferring Option B @bkchr? We need an opposite trait/function from_asset_balance for a #13608 which is a dependency of #13604

Copy link
Member

Choose a reason for hiding this comment

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

Yeah Option B. Generally the name of the trait BalanceConversion and then appears Asset in the function names is weird, but naming is always hard and I don't have a better idea :P from_asset_balance also sounds reasonable.

}

/// Trait to handle asset locking mechanism to ensure interactions with the asset can be implemented
Expand Down
6 changes: 3 additions & 3 deletions frame/transaction-payment/asset-tx-payment/src/payment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ where
// less than one (e.g. 0.5) but gets rounded down by integer division we introduce a minimum
// fee.
let min_converted_fee = if fee.is_zero() { Zero::zero() } else { One::one() };
let converted_fee = CON::to_asset_balance(fee, asset_id)
let converted_fee = CON::convert(fee, asset_id)
.map_err(|_| TransactionValidityError::from(InvalidTransaction::Payment))?
.max(min_converted_fee);
let can_withdraw =
Expand Down Expand Up @@ -146,10 +146,10 @@ where
) -> Result<(AssetBalanceOf<T>, AssetBalanceOf<T>), TransactionValidityError> {
let min_converted_fee = if corrected_fee.is_zero() { Zero::zero() } else { One::one() };
// Convert the corrected fee and tip into the asset used for payment.
let converted_fee = CON::to_asset_balance(corrected_fee, paid.asset())
let converted_fee = CON::convert(corrected_fee, paid.asset())
.map_err(|_| -> TransactionValidityError { InvalidTransaction::Payment.into() })?
.max(min_converted_fee);
let converted_tip = CON::to_asset_balance(tip, paid.asset())
let converted_tip = CON::convert(tip, paid.asset())
.map_err(|_| -> TransactionValidityError { InvalidTransaction::Payment.into() })?;

// Calculate how much refund we should return.
Expand Down