Bug: unstake did not zero capacity #1954 #1955
Conversation
| capacity_details.total_capacity_issued, | ||
| ); | ||
|
|
||
| let capacity_to_withdraw = if staking_target_details.amount.eq(&amount) { |
There was a problem hiding this comment.
This if/else actually addresses the rounding issue. One could argue that it's redundant given the above, and one could also argue that it's very slightly more performant in such case. I'm kind of on the fence about it.
| @@ -528,7 +528,11 @@ impl<T: Config> Pallet<T> { | |||
| target: MessageSourceId, | |||
| target_details: StakingTargetDetails<BalanceOf<T>>, | |||
| ) { | |||
| StakingTargetLedger::<T>::insert(staker, target, target_details); | |||
| if target_details.amount.is_zero() { | |||
There was a problem hiding this comment.
this was an oversight - if they've unstaked everything this record should be deleted.
There was a problem hiding this comment.
Vaguely remember Wil or someone issued a similar fix in frequency, maybe they are unrelated
There was a problem hiding this comment.
Yes that was unrelated
https://github.com/LibertyDSNP/frequency/pull/1769/files
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1955 +/- ##
==========================================
+ Coverage 83.11% 83.13% +0.01%
==========================================
Files 56 56
Lines 4525 4530 +5
==========================================
+ Hits 3761 3766 +5
Misses 764 764 ☔ View full report in Codecov by Sentry. |
github-actions
bot
added
the
metadata-changed
github-actions
bot
added
metadata-changed
|
NVM ,it's already done. |
github-actions
bot
added
metadata-changed
There was a problem hiding this comment.
Should we add a migration to resolve any lingering issues from the bug?
There was a problem hiding this comment.
I think I could run some kind of script to check if there are any mainnet records with incorrect values, but I feel like that part is slightly less urgent than actually fixing the bug. It would be easy to devise a migration script that reaps StakingTargetDetails with a zero amount though.
Goal
The goal of this PR is to ensure that when an account unstakes all token for Capacity, the StakingTargetDetails is reaped. This eliminates an issue with rounding errors.
Closes #1954
Discussion
A bug was found where StakingTargetDetails.amount was zero (correctly) but StakingTargetDetails.capacity was non-zero (incorrectly), when unstaking everything for a target. This is due to rounding errors. Additionally, we should simply reap the StakingTargetDetails when everything is unstaked.
The change corrects for rounding errors and also reaps the StakingTargetDetails record if the resulting amount is zero (without checking the capacity value, but this is reasonable)
Note the CapacityDetails for Provider is not affected by this bug.
Checklist