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

Conversation

wischli
Copy link
Contributor

@wischli wischli commented Mar 15, 2023

Related: #13608

Description

Fixes an inconsistent function naming for BalanceConversion trait: The fn to_asset_balance does not align with generics InBalance and OutBalance as to_asset_balance implies OutBalance to never be the native one.

By fixing this, we enable to use BalanceConversion truly flexibly as desired, see #13608

UPDATE: As a result of @bkchr's remark, renamed BalanceConversion to ConversionToAssetBalance and added an opposite trait ConversionFromAssetBalance which will be used in #13608. Basically, this applies Option B, see below.

Alternative solutions

In case to_asset_balance API should not be broken, I see two alternatives:

Option A

Add from_asset_balance to BalanceConversion:

pub trait BalanceConversion<InBalance, AssetId, OutBalance> {
	type Error;
	fn to_asset_balance(balance: InBalance, asset_id: AssetId) -> Result<OutBalance, Self::Error>;
+	fn from_asset_balance(balance: OutBalance, asset_id: AssetId) -> Result<InBalance, Self::Error>;
}

The downside would be that this forces the only current implementor of BalanceConversion, which is BalanceToAssetBalance, to implement a function which it will never use.

Option B

  • Use existing BalanceConversion for true BalanceToAssetBalance conversion and rename
  • Add mirroring trait for AssetBalanceToBalance conversion
- pub trait BalanceConversion<InBalance, AssetId, OutBalance> {
+ pub trait BalanceToAssetBalance<InBalance, AssetId, OutBalance> {
	// snip
}

+ pub trait AssetBalanceToBalance<InBalance, AssetId, OutBalance> {
+	fn from_asset_balance(balance: InBalance, asset_id: AssetId) -> Result<OutBalance, Self::Error>;
+ }

The downside here is that we reduce the flexibility of this trait and create more noise.

cumulus companion: paritytech/cumulus#2408

@wischli wischli mentioned this pull request Mar 15, 2023
4 tasks
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.

@wischli wischli requested review from bkchr and muharem and removed request for bkchr and muharem March 28, 2023 12:10
@joepetrowski joepetrowski added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit B1-note_worthy Changes should be noted in the release notes T1-runtime This PR/Issue is related to the topic “runtime”. labels Apr 4, 2023
@joepetrowski
Copy link
Contributor

bot merge

@paritytech-processbot
Copy link

Error: "Check reviews" status is not passing for paritytech/cumulus#2408

@bkchr
Copy link
Member

bkchr commented Apr 4, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit bbcc63a into paritytech:master Apr 4, 2023
gpestana pushed a commit that referenced this pull request Apr 23, 2023
* refactor: inconsistent BalanceConversion fn

* Revert "refactor: inconsistent BalanceConversion fn"

This reverts commit 1177877.

* refactor: rename BalanceConversion trait

* feat: add ConversionFromAssetBalance
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/april-updates-for-substrate-and-polkadot-devs/2764/1

nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* refactor: inconsistent BalanceConversion fn

* Revert "refactor: inconsistent BalanceConversion fn"

This reverts commit 1177877.

* refactor: rename BalanceConversion trait

* feat: add ConversionFromAssetBalance
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. B1-note_worthy Changes should be noted in the release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants