Introducing AssetBalance type #5290

Janislav · 2024-09-24T12:46:59Z

Pull Request

Checklist

Please conduct a thorough self-review before opening the PR.

I am confident that the code works.
I have written sufficient tests.
I have written and tested required migrations.
I have updated documentation where appropriate.

Summary

Please include a succinct description of the purpose and content of the PR. What problem does it solve, and how? Link issues, discussions, other PRs, and anything else that will help the reviewer.

Non-Breaking changes

If this PR includes non-breaking changes, select the non-breaking label. On merge, CI will automatically cherry-pick the commit to a PR against the release branch.

codecov · 2024-09-24T12:59:50Z

Codecov Report

Attention: Patch coverage is 87.46439% with 44 lines in your changes missing coverage. Please review.

Project coverage is 70%. Comparing base (e7c9706) to head (2f52dce).

Files with missing lines	Patch %	Lines
state-chain/primitives/src/accounting.rs	86%	18 Missing ⚠️
state-chain/pallets/cf-asset-balances/src/lib.rs	83%	12 Missing and 2 partials ⚠️
state-chain/runtime/src/lib.rs	0%	0 Missing and 7 partials ⚠️
api/lib/src/queries.rs	0%	5 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##            main   #5290    +/-   ##
======================================
- Coverage     70%     70%    -0%     
======================================
  Files        487     488     +1     
  Lines      87463   87568   +105     
  Branches   87463   87568   +105     
======================================
+ Hits       61511   61519     +8     
- Misses     22672   22758    +86     
- Partials    3280    3291    +11

Flag	Coverage Δ
	`70% <87%> (-<1%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- Tests to check we don't mix assets - Tests for scalar operations - Tests for overladed operators

dandanlen

I think there are some issues with this.

I want to have a go at refactoring it, I have some ideas on how to split up the types/traits etc a bit more clearly.

Will put the PR in draft mode for now.

dandanlen · 2024-09-30T10:21:37Z

state-chain/primitives/src/accounting.rs

+pub struct AssetBalance {
+	/// The asset of the balance. For example, DOT, ETH, etc.
+	asset: Asset,
+	/// The amount of the balance.


nit: some of these doc comments don't add much information.

dandanlen · 2024-09-30T10:21:47Z

state-chain/primitives/src/accounting.rs

+/// replacement for the AssetAmount type where it is possible and straight forward. It's intended
+/// that this type is not deriving Copy or Clone to force the user to think about the handling of
+/// the resource.
+#[derive(Debug, Encode, Decode, TypeInfo, Eq)]


What happened to must_use?

dandanlen · 2024-09-30T12:29:28Z

state-chain/primitives/src/accounting.rs

+	/// Subtracts the given amount from the balance, saturating at 0.
+	/// Note: This is a primitive operation and should be used with caution.
+	/// It is the caller's responsibility to ensure **not** to mix assets.
+	pub fn saturating_sub_amount(&mut self, amount: AssetAmount) {
+		self.amount = self.amount.saturating_sub(amount);
+	}
+
+	/// Adds the given amount to the balance, saturating at MAX.
+	/// Note: This is a primitive operation and should be used with caution.
+	/// It is the caller's responsibility to ensure **not** to mix assets.
+	pub fn saturating_add_amount(&mut self, amount: AssetAmount) {
+		self.amount = self.amount.saturating_add(amount);
+	}


These methods should not exist. If we allow the checks to be trivially avoided like this, then this whole change is a bit pointless.

We also had methods like this in your first draft, so I was thinking this can be useful or should be possible. But yeah, I was also wondering...

Nope the first draft didn't have these. The reason was that we also want to try to track ownership (this why we don't implement Clone or Copy).

dandanlen · 2024-09-30T12:33:24Z

state-chain/primitives/src/accounting.rs

+	fn lt(&self, other: &Self) -> bool {
+		Self::ensure_asset_compatibility(self, other);
+		self.amount < other.amount
+	}
+
+	fn le(&self, other: &Self) -> bool {
+		Self::ensure_asset_compatibility(self, other);
+		self.amount <= other.amount
+	}
+
+	fn gt(&self, other: &Self) -> bool {
+		Self::ensure_asset_compatibility(self, other);
+		self.amount > other.amount
+	}
+
+	fn ge(&self, other: &Self) -> bool {
+		Self::ensure_asset_compatibility(self, other);
+		self.amount >= other.amount
+	}


These don't need to be derived, they are provided through partial_cmp

dandanlen · 2024-09-30T13:15:30Z

state-chain/pallets/cf-asset-balances/src/lib.rs

@@ -140,12 +137,12 @@ pub mod pallet {
 	/// Liabilities are funds that are owed to some external party.
 	#[pallet::storage]
 	pub type Liabilities<T: Config> =
-		StorageMap<_, Twox64Concat, Asset, BTreeMap<ExternalOwner, AssetAmount>, ValueQuery>;
+		StorageMap<_, Twox64Concat, Asset, BTreeMap<ExternalOwner, AssetBalance>, OptionQuery>;


Since we don't actually own these assets, I don't think use AssetBalance is correct here - should be AssetAmount.

I don't understand what you mean by "own"

What I mean is that the idea is that AssetBalance represents some assets that owned by network: controlled by the validators. Liabilities is for tracking things that are owed by the network: fees we have paid and need to refund, for example.

dandanlen · 2024-09-30T13:17:40Z

state-chain/pallets/cf-asset-balances/src/lib.rs

@@ -208,24 +205,24 @@ impl<T: Config> Pallet<T> {
 	fn reconcile(
 		chain: ForeignChain,
 		owner: &ExternalOwner,
-		amount_owed: &mut AssetAmount,
-		available: &mut AssetAmount,
+		amount_owed: &mut AssetBalance,


Here too - the amount owed is not some amount of asset that is owned on-chain, it's amount that is owed. I think using AssetAmount here would make more sense.

It makes sense in a way that everywhere you use AssetBalance you have more guarantees, and it's saver to use also if you change the implementation in the future. If you assume that the implementation is 100% correct, and we will never do any changes again that could introduce new bugs mixing up assets or accounting assets in a wrong way, then there is not much point in changing it at all in my opinion.

I agree checking the Asset might make sense here, but this is a rare case where we're comparing assets (owned) to liabilities (not owned). Most of the time we are dealing exclusively with assets owned by the network.

dandanlen · 2024-10-01T12:32:55Z

state-chain/primitives/src/accounting.rs

+
+	/// Ensures we consume the other asset, checks the compatibility and reduces it from asset
+	/// balance saturating at 0.
+	pub fn saturating_reduce(&mut self, other: Self) {


This also doesn't make much sense: we effectively delete other, which is what we're trying to avoid.

That is exactly what we want, or? We have 2 assets, we add the other asset to self, so other doesn't exist anymore (burned/destroyed/what ever). The whole point of this is to use the type system of rust to enforce handling an
value of an AssetBalance type in a way of a physical thing that is gone after I have used it. Like a coin for ex. I am a bit confused now...

Yes this function signature would make sense for adding, but not reducing. How can can you reduce an amount by consuming some other amount? You can't.

I think you can. Lets say you have two Assets. A (self) is 50 B (other) is 30. You reduce A by B. B is consumed by saturating_reduce and A is 20. B is consumed and gone. Makes total sense to me.

Where did B go in this example?

It's like saying: I have €10 and you give me €2, I reduce my 10 by 2 and now I have €8.

dandanlen · 2024-10-01T12:33:33Z

state-chain/primitives/src/accounting.rs

+	/// Ensures we consume the other asset, checks the compatibility and adds it to asset balance.
+	/// Wraps the actual checked_add method and so provides the same functionality. Doesn't modify
+	/// the original balance.
+	pub fn checked_add(&self, other: Self) -> Option<Self> {


This doesn't take ownership of self, so it duplicates funds: after calling this, the caller still owns self but now also has the result, which is self+other.

Indeed, the doc comment is not correct here. I changed the implementation after writing this. The intense was to have exactly something like checked_add (which is useful) but with the check of the assets.

There were 2 things we wanted to achieve: 1. avoid mixing assets. 2. avoid duplicating funds. This method duplicates funds.

Okay, I see what you mean... It's true that checked_add doesn't make a lost of sense here

dandanlen · 2024-10-01T12:35:32Z

state-chain/primitives/src/accounting.rs

+	/// Ensures we consume the other asset, checks the compatibility and subtracts it from asset
+	/// balance. Wraps the actual checked_sub method and so provides the same functionality. Doesn't
+	/// modify the original balance.
+	pub fn checked_sub(&self, other: Self) -> Option<Self> {


dandanlen · 2024-10-01T12:57:17Z

state-chain/primitives/src/accounting.rs

+	}
+}
+
+impl PartialOrd for AssetBalance {


I think deriving these is fine. (same for Ord and PartialEq).

In fact maybe it makes sense to not implement Eq/PartialEq (or only PartialEq?)... We might want to compare asset, or amount, but one balance is only equal to another balance if they are then same and we are ensuring that we can't duplicate balances.

In other words: if I have 1€ and you have 1€, we have the same amounts, and the same currency, but they are not the same coins. So asking "is my coin equal to yours?" is kind of meaningless.

Also Ord: it doesn't make sense to compare $1 to 1FLIP, so PartialOrd might be sufficient (which can return an ordering of None).

Janislav · 2024-10-02T11:45:44Z

My 2 cents: So after reading your comments, I am not sure if this change really makes a lot of sense. The only real benefit we get is that we are more safe in the future with the guarantee a new type can offer (don't mix assets, don't account in a wrong way). It will not add more benefit to our current codebase (assuming it is correct) other than making the code more complex because it's more complex to handle this type and possibly adding new bugs because we have to do a lot of refracting. Most likely we will end up in a scenario where we use in some places AssetBalance and in some other AssetAmount and after some time no one will remember why we use type A here and type B there and everything is getting even more confusing.

dandanlen · 2024-10-02T13:49:31Z

You're entitled to your opinion - I think the change makes sense if we can figure out how to implement it properly and with minimal friction. We have had bugs in the past that would have been caught by a system like this.

assuming it is correct

Exactly - we have to just 'assume' that everywhere we deal with asset balances, it's always correct. Instead, if we make this change, we only have to assume that the AssetBalances type is implemented correcty. That's a much smaller assumption :)

I agree it's not an urgent priority though - I'll keep working on it in the background.

dandanlen and others added 7 commits July 5, 2024 13:38

WIP

d802594

Merge branch 'main' into feat/asset-balances

67facc6

chore: Started to use AssetBalance type inside cf-asset-balances pallet

9ce7579

feature: Using AssetBalance in FreeBalances

452b7f1

tests: fixed tests after refactor

15f1720

chore: handle unwrap

dfc5376

chore: satisfied clippy 🫦

f7e79e3

Janislav added 16 commits September 24, 2024 17:17

test: Handling JSON Object as return parameter.

f49b2cb

Merge branch 'main' into feat/asset-balances

805fc95

chore: fixed linter

6703353

chore: changed interface of withhold_assets

ead725c

chore: removed nasty unwrap

24a15ba

chore: removed not needed handling by ref

5c67ab5

chore: try to unwrap the amount

f4a49ee

chore: added get_free_balance function

aa6e58b

chore: fixed linter

c5306ed

chore: fixed eslint

3feb1d3

chore: fixed prettier (again)

af8bbc8

refactor: Removed copy/clone from AssetBalance.

03b9c4d

refactor: AssetBalance type and add docs

07a43ef

chore: removed getEarnedBrokerFees function

af63394

chore: remove panic test to debug pipeline

4f192db

tests: Added more tests

5ab48c7

- Tests to check we don't mix assets - Tests for scalar operations - Tests for overladed operators

Janislav marked this pull request as ready for review September 27, 2024 09:03

Janislav requested review from dandanlen and martin-chainflip as code owners September 27, 2024 09:03

Merge branch 'main' into feat/asset-balances

2f52dce

dandanlen reviewed Oct 2, 2024

View reviewed changes

dandanlen marked this pull request as draft October 2, 2024 08:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introducing AssetBalance type #5290

Introducing AssetBalance type #5290

Janislav commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

dandanlen left a comment

dandanlen Sep 30, 2024

dandanlen Sep 30, 2024

dandanlen Sep 30, 2024

Janislav Oct 2, 2024

dandanlen Oct 2, 2024

dandanlen Sep 30, 2024

dandanlen Sep 30, 2024

Janislav Oct 2, 2024

dandanlen Oct 2, 2024

dandanlen Sep 30, 2024

Janislav Oct 2, 2024

dandanlen Oct 2, 2024

dandanlen Oct 1, 2024

Janislav Oct 2, 2024

dandanlen Oct 2, 2024

Janislav Oct 2, 2024

dandanlen Oct 2, 2024

dandanlen Oct 1, 2024

Janislav Oct 2, 2024

dandanlen Oct 2, 2024

Janislav Oct 2, 2024

dandanlen Oct 1, 2024

dandanlen Oct 1, 2024

dandanlen Oct 1, 2024

Janislav commented Oct 2, 2024 •

edited

Loading

dandanlen commented Oct 2, 2024

Introducing AssetBalance type #5290

Are you sure you want to change the base?

Introducing AssetBalance type #5290

Conversation

Janislav commented Sep 24, 2024 • edited Loading

Pull Request

Checklist

Summary

Non-Breaking changes

codecov bot commented Sep 24, 2024 • edited Loading

Codecov Report

dandanlen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Janislav commented Oct 2, 2024 • edited Loading

dandanlen commented Oct 2, 2024

Janislav commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

Janislav commented Oct 2, 2024 •

edited

Loading