chore!: lsm redelegation follow-up #22538
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
x/staking/keeper/liquid_stake.go
Outdated
| store.Set(types.TotalLiquidStakedTokensKey, tokensBz) | ||
| err = store.Set(types.TotalLiquidStakedTokensKey, tokensBz) | ||
| if err != nil { | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
x/staking/keeper/liquid_stake.go
Outdated
| store.Delete(key) | ||
| err := store.Delete(key) | ||
| if err != nil { | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
x/staking/keeper/liquid_stake.go
Outdated
| store.Delete(k) | ||
| err := store.Delete(k) | ||
| if err != nil { | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
stana-miric
force-pushed
the
masa/lsm-updates-continued
branch
from
a7be6f2 to
d1cf885
Compare
x/staking/keeper/slash.go
Outdated
|
|
||
| // if the delegator holds a validator bond to destination validator, decrease the destination validator bond shares | ||
| if delegation.ValidatorBond { | ||
| if err := k.SafelyDecreaseValidatorBond(ctx, valDstAddr, math.LegacyDec(totalSlashAmount)); err != nil { |
There was a problem hiding this comment.
should we decrease bonded amount by totalSlashAmount or by total sharesToUnbond?
There was a problem hiding this comment.
I think we should move all this code within the for loop after calling the Unbond method. Basically, decrease the validator bond and both the global and validator liquid totals after every call to Unbond. This will avoid weird behavior when not the entire totalSlashAmount was unbonded. Also, calls to GetDelegation should not return an error (see the call inside the for loop).
There was a problem hiding this comment.
@MSalopek is there a reason you put it outside the for loop?
There was a problem hiding this comment.
To use the totalSlashAmount and to not call it multiple times inside the loop. You can put it in the loop though, it's probably better.
There was a problem hiding this comment.
moved code in the loop and added invariant for liquid stake (for bonded tokens already existed) b6edd57
There was a problem hiding this comment.
for bonded tokens already existed
You mean there was already an invariant for validator bond delegations? If so, how come it was not triggered by this issue?
There was a problem hiding this comment.
Could you also add an invariant for the validator liquid shares?
There was a problem hiding this comment.
yeah, i've added for validator liquid shares. Will check the triggering for the bond delegations to see what's happening
There was a problem hiding this comment.
i improved TestSlashRedelegation because it didnt test slashing of redelegated bonded fund and self-bonded funds, it tested only unbonded redelegations. Also i've changed existing invariant for validator delegations because it didnt checked val bonded delegations just not bonded. Now the invariant is triggered and checked. I also confirmed that with the old code it did not worked.
UPDATE:
TestSlashRedelegation now also contains case for liquid shares
Description
Follows: #22519
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...