Fix low rr withdrawals #1259
Fix low rr withdrawals #1259
Conversation
joe-bowman
requested review from
faddat,
minhngoc274 and
tuantran1702
as code owners
WalkthroughThe recent changes involve a correction in the Makefile for a formatting issue related to the Go version requirement, and an update in the application's upgrade logic to compensate users affected by a redemption rate issue on cosmoshub-4. Specifically, a function was added to mint coins and create new unbonding records for users impacted during specific epochs, ensuring fairness and rectifying the past discrepancy. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| if err := reimburseUsersWithdrawnOnLowRR(ctx, appKeepers); err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
The use of panic for error handling in the reimburseUsersWithdrawnOnLowRR function call is a concern. While it ensures that any failure in the reimbursement process halts the upgrade, it might be too aggressive for non-critical errors. Consider logging the error and gracefully handling recoverable errors to avoid unnecessary crashes.
| // for epochs 137 through 144 the redemption rate was adversely affected on cosmoshub-4 and unbonding users received less than they ought to have done. | ||
| // in order to compensate them, the portion of qAtoms they essentially didn't receive atoms for, we will re-mint, and create a new queued unbonding record. | ||
| // the below users will then receive the appropriate amount of atoms to make them whole. We have to remint the qatoms so that when the unbonding is complete | ||
| // there exists the requisite number of qatoms to burn. | ||
| func reimburseUsersWithdrawnOnLowRR(ctx sdk.Context, appKeepers *keepers.AppKeepers) error { | ||
| users := map[string]struct { | ||
| recipient string | ||
| amount sdk.Coin | ||
| }{ | ||
| "quick143de92kvypafazd200r7fw4pwqjhnlsm724edv": {recipient: "cosmos143de92kvypafazd200r7fw4pwqjhnlsm4w9t57", amount: sdk.NewCoin("uqatom", sdk.NewInt(54013979))}, | ||
| "quick14jy373j0rr5pmpy33e7jlkujc0ve3rdx546ln5": {recipient: "cosmos14jy373j0rr5pmpy33e7jlkujc0ve3rdxl32d2x", amount: sdk.NewCoin("uqatom", sdk.NewInt(3416073))}, | ||
| "quick14xyjk9rnc24my8lchp04f3c0fvzrjgl07grk5y": {recipient: "cosmos14xyjk9rnc24my8lchp04f3c0fvzrjgl04vnydk", amount: sdk.NewCoin("uqatom", sdk.NewInt(227822))}, | ||
| "quick15xq28alrsk6plt4dp7ag7pjvtyangmx635826c": {recipient: "cosmos15xq28alrsk6plt4dp7ag7pjvtyangmx66shcr2", amount: sdk.NewCoin("uqatom", sdk.NewInt(12303565))}, | ||
| "quick1776mt7n23mwcat5vx0cr3x00qgug3d49f45ymy": {recipient: "cosmos1776mt7n23mwcat5vx0cr3x00qgug3d49z3ykzk", amount: sdk.NewCoin("uqatom", sdk.NewInt(538085))}, | ||
| "quick1af74tzu8j679405llklm3yanpkhneaq7mnf3xa": {recipient: "cosmos1af74tzu8j679405llklm3yanpkhneaq7sherl0", amount: sdk.NewCoin("uqatom", sdk.NewInt(161997))}, | ||
| "quick1alaq3havngy0h5sezl98a8xc0jx7xhad74p9nx": {recipient: "cosmos1alaq3havngy0h5sezl98a8xc0jx7xhad433h25", amount: sdk.NewCoin("uqatom", sdk.NewInt(8776885))}, | ||
| "quick1cl6qj3wmf7eynyta7h7a0lud9jemsj6dcaqhxz": {recipient: "cosmos1cl6qj3wmf7eynyta7h7a0lud9jemsj6dnes9ls", amount: sdk.NewCoin("uqatom", sdk.NewInt(2522168))}, | ||
| "quick1e4cnw86pl73k2sfv7uwauflfl42qzncna2j9tt": {recipient: "cosmos1e4cnw86pl73k2sfv7uwauflfl42qzncnkwzhje", amount: sdk.NewCoin("uqatom", sdk.NewInt(43791))}, | ||
| "quick1jd463sarmhp4zyd27jc9zzedmu8tyqdzhmfu4v": {recipient: "cosmos1jd463sarmhp4zyd27jc9zzedmu8tyqdzulewv7", amount: sdk.NewCoin("uqatom", sdk.NewInt(324773))}, | ||
| "quick1jjwf2052uy7fvl8tl65lgxnyr7mggc7vpeq29y": {recipient: "cosmos1jjwf2052uy7fvl8tl65lgxnyr7mggc7v2ascuk", amount: sdk.NewCoin("uqatom", sdk.NewInt(383443))}, | ||
| "quick1lcqquw54wdq07qeu2sx643cp5ppqzy8t5mqf34": {recipient: "cosmos1lcqquw54wdq07qeu2sx643cp5ppqzy8tllsmg8", amount: sdk.NewCoin("uqatom", sdk.NewInt(90487))}, | ||
| "quick1lz6udrmecnjsqhv48fd8ytd8truvdhd2hq6ytn": {recipient: "cosmos1lz6udrmecnjsqhv48fd8ytd8truvdhd2uy2kjp", amount: sdk.NewCoin("uqatom", sdk.NewInt(778236))}, | ||
| "quick1m0e7wr3k4h6xtc97psr66e7njkmv0e9a4l95k9": {recipient: "cosmos1m0e7wr3k4h6xtc97psr66e7njkmv0e9a7m4x0h", amount: sdk.NewCoin("uqatom", sdk.NewInt(316020))}, | ||
| "quick1m6lxmqfgf3s4vu0ktl78w2sz28e86v60sgckht": {recipient: "cosmos1m6lxmqfgf3s4vu0ktl78w2sz28e86v60mvgywe", amount: sdk.NewCoin("uqatom", sdk.NewInt(76642954))}, | ||
| "quick1mf40cxs57a4px5hj5ul0ute2ej5ec6e26xuzcw": {recipient: "cosmos1mf40cxs57a4px5hj5ul0ute2ej5ec6e23zvspu", amount: sdk.NewCoin("uqatom", sdk.NewInt(5590497))}, | ||
| "quick1pwpz0acvw0mc0clr4kknedt94efhwzj8zydzvk": {recipient: "cosmos1pwpz0acvw0mc0clr4kknedt94efhwzj8fqas4y", amount: sdk.NewCoin("uqatom", sdk.NewInt(462513))}, | ||
| "quick1pzzdvazgat8t9epvh2n5xn6wk4zcfc549xj5q9": {recipient: "cosmos1pzzdvazgat8t9epvh2n5xn6wk4zcfc54wzzxeh", amount: sdk.NewCoin("uqatom", sdk.NewInt(13187))}, | ||
| "quick1q0u34n7dujy3mlataslm2qlups9yxqwfwn35d0": {recipient: "cosmos1q0u34n7dujy3mlataslm2qlups9yxqwf9hpx5a", amount: sdk.NewCoin("uqatom", sdk.NewInt(5382251))}, | ||
| "quick1qltxuz7zak8rgx30xvenh6muwrkf8z2d8ffmat": {recipient: "cosmos1qltxuz7zak8rgx30xvenh6muwrkf8z2dvdefye", amount: sdk.NewCoin("uqatom", sdk.NewInt(2564863))}, | ||
| "quick1qsk66jfz02x9r6433xdj5ptkpfp07ytk7ephk3": {recipient: "cosmos1qsk66jfz02x9r6433xdj5ptkpfp07ytk4a390r", amount: sdk.NewCoin("uqatom", sdk.NewInt(91503))}, | ||
| "quick1r83cmscpqhj36pltqt8msqkcxsnpkl4zqqk8xa": {recipient: "cosmos1r83cmscpqhj36pltqt8msqkcxsnpkl4ztyx4l0", amount: sdk.NewCoin("uqatom", sdk.NewInt(170790))}, | ||
| "quick1snvzr84cv8esmlwpcfqg26tfxndn3xwda889w3": {recipient: "cosmos1snvzr84cv8esmlwpcfqg26tfxndn3xwdkrhhhr", amount: sdk.NewCoin("uqatom", sdk.NewInt(1400544))}, | ||
| "quick1t3cwpvu4nrk2zqt9tmhsgkk4ra465q8eqvdljz": {recipient: "cosmos1t3cwpvu4nrk2zqt9tmhsgkk4ra465q8etgadts", amount: sdk.NewCoin("uqatom", sdk.NewInt(1668204))}, | ||
| "quick1uxpfv475505ylmwhxt8qmz6ewpur5hzhtkhat6": {recipient: "cosmos1uxpfv475505ylmwhxt8qmz6ewpur5hzhqj80jg", amount: sdk.NewCoin("uqatom", sdk.NewInt(41000))}, | ||
| "quick1vlfa0p6qm69hyu2zxcfy9zzuqhwkqwzn5tq6zh": {recipient: "cosmos1vlfa0p6qm69hyu2zxcfy9zzuqhwkqwznl0sgm9", amount: sdk.NewCoin("uqatom", sdk.NewInt(754997))}, | ||
| "quick1yr8fgts6d76g0u847zkng2e9l9nk4stw5dkzpu": {recipient: "cosmos1yr8fgts6d76g0u847zkng2e9l9nk4stwlfxscw", amount: sdk.NewCoin("uqatom", sdk.NewInt(1598954))}, | ||
| } |
There was a problem hiding this comment.
The hardcoded list of users and amounts in the reimburseUsersWithdrawnOnLowRR function raises concerns about maintainability and flexibility. Consider fetching this data from a configuration file or a database to make future adjustments easier without requiring code changes.
| if err := appKeepers.BankKeeper.MintCoins(ctx, icstypes.ModuleName, sdk.NewCoins(users[delegator].amount)); err != nil { | ||
| return err |
There was a problem hiding this comment.
Minting coins within a loop without any rate limiting or batching could potentially lead to performance issues or rate limiting from the underlying services. Consider implementing a batching mechanism or rate limiting to ensure system stability.
| if err := appKeepers.BankKeeper.SendCoinsFromModuleToModule(ctx, icstypes.ModuleName, icstypes.EscrowModuleAccount, sdk.NewCoins(users[delegator].amount)); err != nil { | ||
| return err |
There was a problem hiding this comment.
Sending coins from the module to the escrow account within the loop might also suffer from similar performance issues as the minting process. Evaluate the possibility of aggregating these transactions into fewer, larger transactions to improve efficiency.
| appKeepers.InterchainstakingKeeper.SetWithdrawalRecord(ctx, | ||
| icstypes.WithdrawalRecord{ | ||
| ChainId: "cosmoshub-4", | ||
| Delegator: delegator, | ||
| Recipient: users[delegator].recipient, | ||
| BurnAmount: users[delegator].amount, | ||
| Txhash: fmt.Sprintf("%064d", appKeepers.InterchainstakingKeeper.GetNextWithdrawalRecordSequence(ctx)), | ||
| Requeued: false, | ||
| Acknowledged: false, | ||
| Distribution: nil, | ||
| EpochNumber: 145, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Creating withdrawal records within a loop without error handling for each record creation could lead to partial updates if an error occurs midway. Implement error handling and consider a rollback mechanism for already processed records in case of failure to ensure data consistency.
1. Summary
Makes good withdrawals that unbonded with below target RR, due to #1200.
good_rr.unredeemed_qatoms = burn_amount - (unbonding_amount / good_rr)to determine how much of the originalburn_amountwas 'unredeemedgiven that the unbonding amount represents a portion of the burn_amount at thegood_rr`unredeemed_rrfor each user, and sends to the unbonding escrow account.unredeemedamount, in order to trigger the unbonding for theunredeemed_amountat the next epoch.Note: it would have been cleaner to have removed the unbonding tokens from the existing records, but given we are already making changes to the requeued records in thus upgrade, it would make this overly complicated. Given the number of qAtoms is relatively small, re-minting the atoms is sufficient.
Note: also fix Makefile that got a newline added.
Summary by CodeRabbit