Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix low rr withdrawals #1259

Merged
merged 2 commits into from
Mar 9, 2024
Merged

Fix low rr withdrawals #1259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cad36e3
add low rr withdrawal restitutions
Mar 8, 2024
e0ad68b
fix makefile
Mar 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,7 @@ BUILD_TARGETS := build install

check_version:
ifneq ($(GO_MINOR_VERSION),21)
@echo "ERROR: Go version 1.21 is required for building Quicksilver. Detected version: $(GO_MAJOR_VERSION).$(GO_MINOR_VERSION). There are
consensus breaking changes between binaries compiled with different Go versions."
@echo "ERROR: Go version 1.21 is required for building Quicksilver. Detected version: $(GO_MAJOR_VERSION).$(GO_MINOR_VERSION). There are consensus breaking changes between binaries compiled with different Go versions."
exit 1
endif

Expand Down
71 changes: 71 additions & 0 deletions app/upgrades/v1_5.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,83 @@ func V010500UpgradeHandler(
// collate requeued withdrawal records
collateRequeuedWithdrawals(ctx, appKeepers)

if err := reimburseUsersWithdrawnOnLowRR(ctx, appKeepers); err != nil {
panic(err)
}
Comment on lines +79 to +81
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// add claims metadata

return mm.RunMigrations(ctx, configurator, fromVM)
}
}

// 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))},
}
Comment on lines +89 to +125
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


for _, delegator := range utils.Keys(users) {

// mint the coins
if err := appKeepers.BankKeeper.MintCoins(ctx, icstypes.ModuleName, sdk.NewCoins(users[delegator].amount)); err != nil {
return err
Comment on lines +130 to +131
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// send them to the escrow module account
if err := appKeepers.BankKeeper.SendCoinsFromModuleToModule(ctx, icstypes.ModuleName, icstypes.EscrowModuleAccount, sdk.NewCoins(users[delegator].amount)); err != nil {
return err
Comment on lines +135 to +136
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
},
)
Comment on lines +139 to +151
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
return nil
}

// collateRequeuedWithdrawals will iterate, per zone, over requeued queued and active withdrawal records and
// collate them into a single record for a delegator/recipient/epoch tuple.
func collateRequeuedWithdrawals(ctx sdk.Context, appKeepers *keepers.AppKeepers) {
Expand Down
Loading