fix: denomination arithmetic

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

@@ -5,6 +5,8 @@ pub(crate) mod square_root;
 use parity_scale_codec::{Decode, Encode, MaxEncodedLen};
 use scale_info::TypeInfo;
 use sp_arithmetic::ArithmeticError;
+use sp_core::U256;
+use sp_runtime::traits::CheckedConversion;
 use sp_std::ops::{AddAssign, BitOrAssign, ShlAssign};
 use substrate_fixed::traits::{Fixed, FixedSigned, ToFixed};
 
@@ -104,13 +106,30 @@ fn calculate_accumulated_passive_issuance<Balance: Fixed>(passive_issuance: &[Ba
 		.fold(Balance::from_num(0), |sum, x| sum.saturating_add(*x))
 }
 
-pub(crate) fn convert_to_fixed<T: Config>(
-	x: u128,
-	denomination: u8,
-) -> Result<CurveParameterTypeOf<T>, ArithmeticError> {
-	let decimals = 10u128
-		.checked_pow(u32::from(denomination))
+pub(crate) fn convert_to_fixed<T: Config>(x: u128, denomination: u8) -> Result<CurveParameterTypeOf<T>, ArithmeticError>
+where
+	<CurveParameterTypeOf<T> as Fixed>::Bits: TryFrom<U256>, // TODO: make large integer type configurable in runtime
+{
+	let decimals = U256::from(10)
+		.checked_pow(denomination.into())
 		.ok_or(ArithmeticError::Overflow)?;
-	let scaled_x = x.checked_div(decimals).ok_or(ArithmeticError::DivisionByZero)?;
-	scaled_x.checked_to_fixed().ok_or(ArithmeticError::Overflow)
+	// Convert to U256 so we have enough bits to perform lossless scaling.
+	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).
+	// This function can panic in theory, but only if frac_nbits() would be larger
+	// than 256 - and no Fixed of that size exists.
+	x_u256.shl_assign(CurveParameterTypeOf::<T>::frac_nbits());
+	// Perform division. Due to the shift the precision/truncation is identical to
+	// division on the fixed type.
+	x_u256 = x_u256.checked_div(decimals).ok_or(ArithmeticError::DivisionByZero)?;
+	// Try conversion to integer type underlying the fixed type (e.g., i128 for a
+	// I75F53). If this overflows, there is nothing we can do; even the scaled value
+	// does not fit in the fixed type.
+	let truncated = x_u256.checked_into().ok_or(ArithmeticError::Overflow)?;
+	// Cast the integer as a fixed. We can do this because we've already applied the
+	// correct bit shift above.
+	let fixed = <CurveParameterTypeOf<T> as Fixed>::from_bits(truncated);
+	// Return the result of scaled conversion to fixed.
+	Ok(fixed)
 }

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

@@ -36,10 +36,10 @@ pub mod pallet {
 	use frame_system::pallet_prelude::*;
 	use parity_scale_codec::FullCodec;
 	use sp_arithmetic::ArithmeticError;
+	use sp_core::U256;
 	use sp_runtime::{
 		traits::{
-			Bounded, CheckedDiv, CheckedMul, One, SaturatedConversion, Saturating, StaticLookup, UniqueSaturatedInto,
-			Zero,
+			Bounded, CheckedConversion, One, SaturatedConversion, Saturating, StaticLookup, UniqueSaturatedInto, Zero,
 		},
 		BoundedVec,
 	};
@@ -129,11 +129,17 @@ pub mod pallet {
 		/// The maximum number of currencies allowed for a single pool.
 		#[pallet::constant]
 		type MaxCurrencies: Get<u32>;
-		/// The deposit required for each bonded currency.
 
 		#[pallet::constant]
 		type MaxStringLength: Get<u32>;
 
+		/// The maximum denomination that bonded currencies can use. This should
+		/// be configured so that
+		/// 10^MaxDenomination < 2^CurveParameterType::frac_nbits()
+		/// as larger denominations could result in truncation.
+		#[pallet::constant]
+		type MaxDenomination: Get<u8>;
+
 		/// The deposit required for each bonded currency.
 		#[pallet::constant]
 		type DepositPerCurrency: Get<DepositCurrencyBalanceOf<Self>>;
@@ -173,6 +179,23 @@ pub mod pallet {
 	#[pallet::pallet]
 	pub struct Pallet<T>(_);
 
+	#[pallet::hooks]
+	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
+		fn integrity_test() {
+			let scaling_factor = U256::from(10).checked_pow(T::MaxDenomination::get().into()).expect(
+				"`MaxDenomination` is set so high that the resulting scaling factor cannot be represented. /
+				Any attempt to mint or burn on a pool where `10^denomination > 2^256` _WILL_ fail.",
+			);
+
+			assert!(
+				U256::from(2).pow(T::CurveParameterType::frac_nbits().into()) > scaling_factor,
+				"In order to prevent truncation of balances, `MaxDenomination` should be configured such \
+				that the maximum scaling factor `10^MaxDenomination` is smaller than the fractional \
+				capacity `2^frac_nbits` of `CurveParameterType`",
+			);
+		}
+	}
+
 	#[pallet::storage]
 	#[pallet::getter(fn pools)]
 	pub(crate) type Pools<T: Config> = StorageMap<_, Twox64Concat, T::PoolId, PoolDetailsOf<T>, OptionQuery>;
@@ -256,7 +279,8 @@ pub mod pallet {
 	#[pallet::call]
 	impl<T: Config> Pallet<T>
 	where
-		<CurveParameterTypeOf<T> as Fixed>::Bits: Copy + ToFixed + AddAssign + BitOrAssign + ShlAssign,
+		<CurveParameterTypeOf<T> as Fixed>::Bits: Copy + ToFixed + AddAssign + BitOrAssign + ShlAssign + TryFrom<U256>,
+		CollateralCurrenciesBalanceOf<T>: Into<U256> + TryFrom<U256>, // TODO: make large integer type configurable
 	{
 		#[pallet::call_index(0)]
 		#[pallet::weight(Weight::from_parts(10_000, 0) + T::DbWeight::get().writes(1))]
@@ -270,10 +294,12 @@ pub mod pallet {
 		) -> DispatchResult {
 			let who = T::PoolCreateOrigin::ensure_origin(origin)?;
 
-			let currency_length = currencies.len();
+			ensure!(denomination <= T::MaxDenomination::get(), Error::<T>::InvalidInput);
 
 			let checked_curve = curve.try_into().map_err(|_| Error::<T>::InvalidInput)?;
 
+			let currency_length = currencies.len();
+
 			let current_asset_id = NextAssetId::<T>::get();
 
 			let (currency_ids, next_asset_id) = Self::generate_sequential_asset_ids(current_asset_id, currency_length)?;
@@ -661,13 +687,14 @@ pub mod pallet {
 
 			// With amount = max_value(), this trait implementation burns the reducible
 			// balance on the account and returns the actual amount burnt
-			let burnt = T::Fungibles::burn_from(
+			let burnt: U256 = T::Fungibles::burn_from(
 				asset_id.clone(),
 				&who,
 				Bounded::max_value(),
 				WithdrawalPrecision::BestEffort,
 				Fortitude::Force,
-			)?;
+			)?
+			.into();
 
 			if burnt.is_zero() {
 				// no funds available to be burnt on account; nothing to do here
@@ -677,16 +704,29 @@ pub mod pallet {
 			let sum_of_issuances = pool_details
 				.bonded_currencies
 				.into_iter()
-				.fold(FungiblesBalanceOf::<T>::zero(), |sum, id| {
-					sum.saturating_add(T::Fungibles::total_issuance(id))
-				});
+				.fold(U256::from(0), |sum, id| {
+					sum.saturating_add(T::Fungibles::total_issuance(id).into())
+				})
+				// Add the burnt amount back to the sum of total supplies
+				.checked_add(burnt)
+				.ok_or(ArithmeticError::Overflow)?;
 
-			let amount = burnt
-				.checked_mul(&total_collateral_issuance)
-				.ok_or(ArithmeticError::Overflow)? // TODO: do we need a fallback if this fails?
-				.checked_div(&sum_of_issuances)
-				.ok_or(Error::<T>::NothingToRefund)?; // should be impossible - how would we be able to burn funds if the sum of total
-									  // supplies is 0?
+			defensive_assert!(
+				sum_of_issuances >= burnt,
+				"burnt amount exceeds the total supply of all bonded currencies"
+			);
+
+			let amount: CollateralCurrenciesBalanceOf<T> = burnt
+				.checked_mul(total_collateral_issuance.into())
+				// As long as the balance type is half the size of a U256, this won't overflow.
+				.ok_or(ArithmeticError::Overflow)?
+				.checked_div(sum_of_issuances)
+				// Because sum_of_issuances >= burnt > 0, this is theoretically impossible
+				.ok_or(Error::<T>::Internal)?
+				.checked_into()
+				// Also theoretically impossible, as the result must be <= total_collateral_issuance
+				// if burnt <= sum_of_issuances, which should always hold true
+				.ok_or(Error::<T>::Internal)?;
 
 			if amount.is_zero()
 				|| T::CollateralCurrencies::can_deposit(
@@ -794,7 +834,7 @@ pub mod pallet {
 
 	impl<T: Config> Pallet<T>
 	where
-		<CurveParameterTypeOf<T> as Fixed>::Bits: Copy + ToFixed + AddAssign + BitOrAssign + ShlAssign,
+		<CurveParameterTypeOf<T> as Fixed>::Bits: Copy + ToFixed + AddAssign + BitOrAssign + ShlAssign + TryFrom<U256>,
 	{
 		fn calculate_collateral(
 			low: CurveParameterTypeOf<T>,
@@ -819,6 +859,7 @@ pub mod pallet {
 
 			Ok(real_costs)
 		}
+
 		fn calculate_normalized_passive_issuance(
 			bonded_currencies: &[FungiblesAssetIdOf<T>],
 			denomination: u8,

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

@@ -213,6 +213,7 @@ pub mod runtime {
 		pub const CurrencyDeposit: Balance = 500;
 		pub const MaxCurrencies: u32 = 50;
 		pub const CollateralAssetId: u32 = u32::MAX;
+		pub const MaxDenomination: u8 = 15;
 	}
 
 	impl pallet_bonded_coins::Config for Test {
@@ -232,6 +233,7 @@ pub mod runtime {
 		type PoolId = AccountId;
 		type RuntimeEvent = RuntimeEvent;
 		type RuntimeHoldReason = RuntimeHoldReason;
+		type MaxDenomination = MaxDenomination;
 	}
 
 	#[derive(Clone, Default)]
@@ -289,8 +291,8 @@ pub mod runtime {
 				.chain(collateral_assets)
 				.collect();
 
-			// NextAssetId is set to the maximum value of all collateral/bonded currency ids, plus one.
-			// If no currencies are created, it's set to 0.
+			// NextAssetId is set to the maximum value of all collateral/bonded currency
+			// ids, plus one. If no currencies are created, it's set to 0.
 			let next_asset_id = all_assets.iter().map(|(id, ..)| id).max().map_or(0, |id| id + 1);
 
 			pallet_assets::GenesisConfig::<Test> {

pallets/pallet-bonded-coins/src/tests/curves/arithmetic.rs

@@ -0,0 +1,135 @@
+use crate::{
+	curves::convert_to_fixed,
+	mock::{runtime::Test, Float},
+};
+use frame_support::assert_ok;
+use sp_runtime::ArithmeticError;
+
+#[test]
+fn test_convert_to_fixed_basic() {
+	let x = 1000u128;
+	let denomination = 2u8; // 10^2 = 100
+
+	let result = convert_to_fixed::<Test>(x, denomination).unwrap();
+	// Test runtime uses I75F53 for CurveParameterTypeOf, which is what we'll cover
+	// in testing.
+	let expected = Float::from_num(10); // 1000 / 100 = 10
+
+	assert_eq!(result, expected);
+}
+
+#[test]
+fn test_convert_to_fixed_with_remainder() {
+	let x = 1050u128;
+	let denomination = 2u8; // 10^2 = 100
+
+	let result = convert_to_fixed::<Test>(x, denomination).unwrap();
+	let expected = Float::from_num(10.5); // 1050 / 100 = 10.5
+
+	assert_eq!(result, expected);
+}
+
+#[test]
+fn test_convert_to_fixed_smaller_than_denomination() {
+	let x = 1050u128;
+	let denomination = 6u8; // 10^6 = 1000000
+
+	let result = convert_to_fixed::<Test>(x, denomination).unwrap();
+	let expected = Float::from_num(0.00105); // 1050 / 1000000 = 0.00105
+
+	assert_eq!(result, expected);
+}
+
+#[test]
+fn test_convert_to_fixed_large_value() {
+	let x = 1_000_000_000_000_000u128;
+	let denomination = 12u8; // 10^12 = 1_000_000_000_000
+
+	let result = convert_to_fixed::<Test>(x, denomination).unwrap();
+	let expected = Float::from_num(1000); // 1_000_000_000_000_000 / 1_000_000_000_000 = 1000
+
+	assert_eq!(result, expected);
+}
+
+#[test]
+fn test_convert_to_fixed_small_denomination() {
+	let x = 12345u128;
+	let denomination = 1u8; // 10^1 = 10
+
+	let result = convert_to_fixed::<Test>(x, denomination).unwrap();
+	let expected = Float::from_num(1234.5); // 12345 / 10 = 1234.5
+
+	assert_eq!(result, expected);
+}
+
+#[test]
+fn test_convert_to_fixed_overflow() {
+	let x = u128::MAX;
+	let denomination = 0u8; // 10^0 = 1, no scaling
+
+	let result = convert_to_fixed::<Test>(x, denomination);
+	assert!(result.is_err());
+	assert_eq!(result.unwrap_err(), ArithmeticError::Overflow);
+}
+
+#[test]
+fn test_convert_to_fixed_denomination_overflow() {
+	let x = 1000u128;
+	let denomination = 128u8; // 10^128 overflows
+
+	let result = convert_to_fixed::<Test>(x, denomination);
+	assert!(result.is_err());
+	assert_eq!(result.unwrap_err(), ArithmeticError::Overflow);
+}
+
+#[test]
+fn test_convert_to_fixed_overflow_avoided() {
+	let x = u128::MAX; // around 3.4e+38
+	let denomination = 17u8; // I75F53 should handle around 1.8e+22, 38 - 23 -> 17
+
+	let result = convert_to_fixed::<Test>(x, denomination);
+	assert_ok!(result);
+}
+
+#[test]
+fn test_convert_to_fixed_handles_large_denomination() {
+	let x = u128::MAX; // around 3.4e+38
+	let denomination = 22u8; // I75F53 should handle around 1.8e+22; this is the maximum safe denomination
+
+	let result = convert_to_fixed::<Test>(x, denomination);
+	assert_ok!(result);
+}
+
+#[test]
+fn test_convert_to_fixed_very_large_denomination() {
+	let denomination = 30u8; // I75F53 should handle around 1.8e+22, this can lead to overflow
+
+	// multiple of denomination would not result in remainder = 0
+	assert_ok!(convert_to_fixed::<Test>(10u128.pow(31), denomination));
+
+	// non-multiples of denomination could lead to overflow of remainder
+	assert_ok!(convert_to_fixed::<Test>(11u128.pow(31), denomination));
+	assert_ok!(convert_to_fixed::<Test>(10u128.pow(29), denomination));
+}
+
+#[test]
+fn test_convert_to_fixed_zero_denomination() {
+	let x = 1000u128;
+	let denomination = 0u8; // 10^0 = 1
+
+	let result = convert_to_fixed::<Test>(x, denomination).unwrap();
+	let expected = Float::from_num(1000); // 1000 / 1 = 1000
+
+	assert_eq!(result, expected);
+}
+
+#[test]
+fn test_convert_to_fixed_zero_input() {
+	let x = 0u128;
+	let denomination = 10u8; // 10^10 = large divisor
+
+	let result = convert_to_fixed::<Test>(x, denomination).unwrap();
+	let expected = Float::from_num(0); // 0 / any number = 0
+
+	assert_eq!(result, expected);
+}

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

@@ -1,3 +1,4 @@
+mod arithmetic;
 mod lmsr;
 mod polynomial;
 mod square_root;