fix: denomination arithmetic #794
Conversation
| // This WILL overflow if 10^denomination > capacity(float) | ||
| .checked_div(decimals.checked_to_fixed().ok_or(ArithmeticError::Overflow)?) |
There was a problem hiding this comment.
The solution comes at the cost of one additional case where overflow can happen:
- Previously, overflow could happen where
capacity(balance) / 10^denomination > capacity(fixed), which for a u128 balance (capacity ~3.4e+38) and a I75F53 (capacity ~1.8e+22) meant that any denomination >= 17 is safe. - With the current solution, overflow will occur where
10^denomination > capacity(float), which for an I75F53 means that we have a hard cap at a denomination of 22.
We can possibly soften the hard cap, e.g. by iteratively dividing the remainder by 10 (or some other multiplicative component of decimals which can be represented), but denominations larger than the soft cap would progressively increase the probability of an overflow occurring.
For example, with an I75F53 and a denomination of 23, any amount ~1.8e+22 < x < e+23 would result in a remainder that cannot be represented and thus in an overflow error.
| .checked_mul(&burnt) | ||
| // This can indeed overflow if (sum_of_issuances - 1) * burnt > capacity(u128) | ||
| .ok_or(ArithmeticError::Overflow)? |
There was a problem hiding this comment.
Very large sum_of_issuances make large remainders more likely, which can in turn result in a product that is too large.
We can accept that refunds would sometimes fail, which would require accounts with lower balances to refund first (after which the situation may have changed).
Or we can look for fallback strategies, such as:
- saturating multiplication, which could mean that people receive only a fraction of the funds they would be entitled to
- scaling, which sacrifices precision
- partial refunds, where we only burn the amount of funds for which we can properly compute the collateral to be refunded, and leave the rest untouched.
| #[test] | ||
| fn test_convert_to_fixed_too_large_denomination() { | ||
| let x = u128::MAX; // around 3.4e+38 | ||
| let denomination = 30u8; // I75F53 should handle around 1.8e+22, this can't be represented | ||
|
|
||
| let result = convert_to_fixed::<Test>(x, denomination); | ||
| assert_ok!(result); | ||
| } |
There was a problem hiding this comment.
this test fails, as a reminder that the proposed solution is still imperfect
There was a problem hiding this comment.
Due to 694334c this test now passes; but not for u128::MAX, which results in an overflowing remainder.
| // divide first to avoid overflow, remainder will be handled seperately | ||
| let divisible_part = total_collateral_issuance |
There was a problem hiding this comment.
I realised that the approach of handling remainders separately can also improve the calculations of how much collateral is refunded, preventing overflow in many (but not all) cases.
|
|
||
| #[pallet::constant] | ||
| type MaxStringLength: Get<u32>; | ||
|
|
||
| /// The maximum denomination that bonded currencies can use. To ensure | ||
| /// safe operation, this should be set so that 10^MaxDenomination < |
There was a problem hiding this comment.
If you want to make sure that only these values are specified here, you can add an integrity test (available with the try-runtime feature) that only checks the values of the Config implementations for any given runtime, according to the specified rules. You can check an example here: https://github.com/paritytech/polkadot-sdk/blob/a875f1803253e469288f1a905e73861fb600bb28/polkadot/runtime/parachains/src/configuration.rs#L1258.
There was a problem hiding this comment.
This is to protect against malformed runtime-injected values inside a config trait, not too much about user-provided values.
Adresses issues with precision stemming from integer division in
convert_to_fixedand suggests a strategy to mitigate overflow problems onrefund_account.Checklist:
array[3]useget(3), ...)