refactor: use U256 for infallible scaling #796

rflechtner · 2024-11-13T13:05:39Z

I realised that scaling may be done more elegantly and less likely to overflow by using a U256 to implement what essentially amounts to direct division on a fixed point number with >= 128 integer bits. This way the only overflow we can get is when the result (the scaled value) is too large to fit into the target type.

In other words, the scaling logic is infallible, with the only error case being that the scaling factor is too small, so that the result cannot be represented.

I wanted to ask for your opinion on this solution before I apply it to my pending PR.

Checklist:

I have verified that the code works
- No panics! (checked arithmetic ops, no indexing array[3] use get(3), ...)
I have verified that the code is easy to understand
- If not, I have left a well-balanced amount of inline comments
I have left the code in a better state
I have documented the changes (where applicable)
- Either PR or Ticket to update the Docs
- Link the PR/Ticket here

rflechtner · 2024-11-13T13:11:06Z

pallets/pallet-bonded-coins/src/curves/mod.rs

+	let decimals = U256::from(10)
+		.checked_pow(denomination.into())


denomination is now cast to a U256, which seems a little dumb. We could also stick to using a u128 to perform this calculation (raising to the power of denomination) and then convert the result to U256. Not sure what's more efficient though.

rflechtner · 2024-11-13T13:12:00Z

pallets/pallet-bonded-coins/src/curves/mod.rs

+	let mut x_u256 = U256::from(x);
+	// Shift left to produce the representation that our fixed type would have (but
+	// with extra integer bits that would potentially not fit in the fixed type).
+	x_u256.shl_assign(CurveParameterTypeOf::<T>::frac_nbits());


Any idea if this can panic? I didn't find a checked_ variant of shl, so probably not?

The macro is implementing the trait:

impl<T> $crate::core_::ops::ShlAssign<T> for $name where T: Into<$name> { fn shl_assign(&mut self, shift: T) { *self = *self << shift; } }

the << operation can panic

Ok but the panic would occur if the shift is larger than the size of the number, right? Which would be an insane misconfiguration. Should we just prevent against misconfiguration the way @ntn-x2 suggested here and not check this at runtime?

I am against removing the checks from the runtime. Anything that the type system does not guarantee against should be checked, and having it in the runtime does not imply removing the static tests for the config itself.

Ok because there is no checked implementation, I would then implement a check to be run at each invocation of the function determining if the types have been chosen correctly.

Actually as long as we don't have this configurable this cannot panic, because the U256 is large enough to work with any Fixed type that exists

pallets/pallet-bonded-coins/src/curves/mod.rs

rflechtner · 2024-11-13T14:34:35Z

pallets/pallet-bonded-coins/src/curves/mod.rs

 pub(crate) fn convert_to_fixed<T: Config>(x: u128, denomination: u8) -> Result<CurveParameterTypeOf<T>, ArithmeticError>
 where
-	<CurveParameterTypeOf<T> as Fixed>::Bits: TryFrom<u128>,
+	<CurveParameterTypeOf<T> as Fixed>::Bits: TryFrom<U256>,


We could in principle make this more generic by not fixing the type of x (an Into<U256> trait bound or similar is enough) and by making the type of the big integer configurable in the runtime, so you can match it to the chosen balance and fixed types.

The big integer only needs to be the size of the balance type plus the size of the fractional bits of the fixed, so for a u64 balance and a I75F53 fixed a u128 would be sufficient for conversion.

Added a TODO in cffd4bd

rflechtner · 2024-11-13T14:57:00Z

pallets/pallet-bonded-coins/src/lib.rs

 	where
-		<CurveParameterTypeOf<T> as Fixed>::Bits: Copy + ToFixed + AddAssign + BitOrAssign + ShlAssign + TryFrom<u128>,
+		<CurveParameterTypeOf<T> as Fixed>::Bits: Copy + ToFixed + AddAssign + BitOrAssign + ShlAssign + TryFrom<U256>,
+		CollateralCurrenciesBalanceOf<T>: Into<U256> + TryFrom<U256>,


Using bigger integers is also a better fix for the overflow issues we had when computing the refund amounts. This second trait bound is for this fix.

We can also make this large integer type configurable, it just needs to have twice the size of the balance type.

No strong opinion on this. I don’t think any other project is using a balance type larger than u128.

Good thing is you can also configure it to be smaller, avoiding blowing it up to a U256 when it's completely unnecessary, as it would be if your balance is just a u64 (where a u128 would be enough)

Added a TODO in cffd4bd

rflechtner · 2024-11-13T14:57:33Z

pallets/pallet-bonded-coins/src/lib.rs

+				.fold(U256::from(0), |sum, id| {
+					sum.saturating_add(T::Fungibles::total_issuance(id).into())


this can now also no longer overflow

Ad96el · 2024-11-14T08:40:37Z

pallets/pallet-bonded-coins/src/lib.rs

 			defensive_assert!(
 				sum_of_issuances >= burnt,
 				"burnt amount exceeds the total supply of all bonded currencies"
 			);


This can panic or not?

this does not panic in production - it does though in non-optimised builds

Ad96el · 2024-11-14T08:44:54Z

pallets/pallet-bonded-coins/src/lib.rs

 	where
-		<CurveParameterTypeOf<T> as Fixed>::Bits: Copy + ToFixed + AddAssign + BitOrAssign + ShlAssign + TryFrom<u128>,
+		<CurveParameterTypeOf<T> as Fixed>::Bits: Copy + ToFixed + AddAssign + BitOrAssign + ShlAssign + TryFrom<U256>,
+		CollateralCurrenciesBalanceOf<T>: Into<U256> + TryFrom<U256>,


No strong opinion on this. I don’t think any other project is using a balance type larger than u128.

Ad96el · 2024-11-14T08:58:31Z

pallets/pallet-bonded-coins/src/curves/mod.rs

+	let mut x_u256 = U256::from(x);
+	// Shift left to produce the representation that our fixed type would have (but
+	// with extra integer bits that would potentially not fit in the fixed type).
+	x_u256.shl_assign(CurveParameterTypeOf::<T>::frac_nbits());


The macro is implementing the trait:

impl<T> $crate::core_::ops::ShlAssign<T> for $name where T: Into<$name> { fn shl_assign(&mut self, shift: T) { *self = *self << shift; } }

the << operation can panic

Little bug in the total sum calculation

refactor: use U256 for infallible scaling

8fba54f

rflechtner commented Nov 13, 2024

View reviewed changes

rflechtner requested review from Ad96el and ntn-x2 and removed request for ntn-x2 November 13, 2024 13:11

rflechtner commented Nov 13, 2024

View reviewed changes

pallets/pallet-bonded-coins/src/curves/mod.rs Show resolved Hide resolved

refactor!: use U256 to prevent overflow on refund

79f2261

rflechtner commented Nov 13, 2024

View reviewed changes

Ad96el reviewed Nov 14, 2024

View reviewed changes

Ad96el and others added 2 commits November 14, 2024 10:02

fix: total supply (#798)

d07d937

Little bug in the total sum calculation

chore: add TODOs

cffd4bd

rflechtner marked this pull request as ready for review November 14, 2024 12:06

Ad96el approved these changes Nov 14, 2024

View reviewed changes

rflechtner merged commit 08b71c4 into fix-denomination-arithmetic Nov 14, 2024
1 of 7 checks passed

rflechtner deleted the rf-U256-math branch November 14, 2024 12:13

		let decimals = U256::from(10)
		.checked_pow(denomination.into())

		.fold(U256::from(0), \|sum, id\| {
		sum.saturating_add(T::Fungibles::total_issuance(id).into())

refactor: use U256 for infallible scaling #796

refactor: use U256 for infallible scaling #796

Uh oh!

Conversation

rflechtner commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rflechtner Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rflechtner Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rflechtner commented Nov 13, 2024 •

edited

Loading

rflechtner Nov 13, 2024 •

edited

Loading

rflechtner Nov 13, 2024 •

edited

Loading