Fix: pallet-pool-system - Ratio Calculation
#1856
Conversation
mustermeiszer
added
I2-bug
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1856 +/- ##
==========================================
+ Coverage 46.64% 46.68% +0.03%
==========================================
Files 167 167
Lines 13112 13129 +17
==========================================
+ Hits 6116 6129 +13
- Misses 6996 7000 +4 ☔ View full report in Codecov by Sentry. |
pallets/pool-system/src/lib.rs
Outdated
| let mut tranche_ratios = epoch | ||
| .tranches | ||
| .non_residual_tranches() | ||
| .map(|tranches| { | ||
| tranches | ||
| .iter() | ||
| // NOTE: Reversing amounts, as residual amount is on top. | ||
| // NOTE: Iterator of executed amounts is one time larger than the | ||
| // non_residual_tranche-iterator, but we anyways push all reamaining | ||
| // ratio to the residual tranche. | ||
| .zip(executed_amounts.rev()) | ||
| .map(|(tranche, &(invest, redeem))| { | ||
| Ok(Perquintill::from_rational( | ||
| tranche.supply.ensure_add(invest)?.ensure_sub(redeem)?, | ||
| total_assets, | ||
| )) | ||
| }) | ||
| .collect::<Result<Vec<_>, ArithmeticError>>() | ||
| }) |
There was a problem hiding this comment.
Can"t we simplify this by using existing Tranches API or does this not work because all API includes the residual tranche?
There was a problem hiding this comment.
it is already using tranches, but not the combine as I, as you said, need to exclude the residual tranche.
There was a problem hiding this comment.
Happy to add a cleaner solution if you have one at hand.
|
@wischli what level of testing for the ratios would be fine for you? 3 tranche pool and 1 tranche pool additionally, or more movement in the ratios? |
Thanks a lot for providing an alternate solution! IMO the new version is easier to follow and thus better readable but the first version cleaner codewise. So I vote for keeping the current version 2. Which one do you prefer? |
|
Would keep the first version |
| total_assets, | ||
| ) | ||
| } else { | ||
| Perquintill::one().ensure_sub(sum_non_residual_tranche_ratios)? |
There was a problem hiding this comment.
The sum of tranche ratios can never be higher than one, right?
There was a problem hiding this comment.
I think this line is annulated by line 1263. No matter which value sum_non_residual_tranche_ratios has, at the end of this iteration sum_non_residual_tranche_ratios will always have the value one(). Is this the expected behavior?
| Perquintill::from_rational(1u64, 3), | ||
| ]; | ||
|
|
||
| // Ensure ratios are 100 |
There was a problem hiding this comment.
IIUC
| // Ensure ratios are 100 | |
| // Ensure ratios are 1/3 each |
Description
The current ratio calculation has the following bug. The logic assumes that
Sum(tranche.supply) = total_assets, but as the residual tranche does only receive its variable interest after the ratio calculation, this is not true.In order to fix this, the following change does give the automatically all the remaining ratio to the residual tranche.
Changes and Descriptions
fn do_execute_epochChecklist: