collect requeued txs together #1222
Conversation
joe-bowman
requested review from
faddat,
minhngoc274 and
tuantran1702
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes introduced focus on enhancing the interchain staking mechanism, particularly addressing the handling of multi-validator scenarios and requeued withdrawal records. The improvements include better logic for withdrawal record management, adjustments in distribution and burn amount calculations, and the grouping of requeued unbondings. This ensures a more efficient and accurate processing of staking operations across different validators and user scenarios. Changes
Assessment against linked issues
Poem
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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Configuration used: CodeRabbit UI
Files selected for processing (3)
- x/interchainstaking/keeper/ibc_packet_handlers.go (2 hunks)
- x/interchainstaking/keeper/ibc_packet_handlers_test.go (4 hunks)
- x/interchainstaking/keeper/withdrawal_record.go (3 hunks)
Additional comments: 15
x/interchainstaking/keeper/withdrawal_record.go (2)
- 31-53: The
GetNextWithdrawalRecordSequencefunction correctly retrieves, increments, and stores the global withdrawal record sequence. This implementation aligns with the PR objectives and ensures that each withdrawal record has a unique sequence number. The error handling in case of unmarshalling failure is done by panicking, which is a common pattern in blockchain systems where consistency is critical. However, it's important to ensure that such panics are caught and handled appropriately at higher levels to prevent system crashes.- 171-183: The addition of
GetUserChainRequeuedWithdrawalRecordfunction is a significant improvement, as it allows for retrieving a requeued withdrawal record for a specific user and chain. This function directly supports the PR's objective of optimizing the handling of requeued transactions by updating existing records instead of creating new ones. The implementation iterates over withdrawal records with a specific status and returns the first matching requeued record for the given user and chain. This approach is efficient and aligns well with the described objectives. However, it's worth noting that if there are multiple requeued records for the same user and chain, this function will only return the first one encountered. Ensure this behavior is intended and documented.x/interchainstaking/keeper/ibc_packet_handlers.go (13)
- 855-896: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [35-48]
The
DeserializeCosmosTxTypedfunction is implemented correctly. It properly handles the deserialization of Cosmos transactions into a slice ofTypedMsgstructs, including error handling.
- 855-896: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [257-259]
The
HandleTimeoutfunction is correctly implemented as a placeholder for handling IBC packet timeouts. Ensure that appropriate timeout handling logic is added in the future if required by the application logic.
- 855-896: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [431-474]
The
HandleFailedBankSendfunction correctly handles the failure of a bank send message, including handling various scenarios based on the sender and receiver addresses. The structure and logic are consistent with the rest of the file.
- 855-896: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [476-496]
The
HandleFailedUnbondSendfunction correctly handles the failure of an unbond send message, including updating the withdrawal record status and requeuing the withdrawal. The implementation is consistent with the rest of the file.
- 855-896: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1010-1042]
The
HandleFailedRedeemTokensfunction correctly handles the failure of a redeem tokens for shares message, including decrementing the withdrawal waitgroup and triggering the redemption rate calculation. The implementation is consistent with the rest of the file.
- 855-896: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1109-1141]
The
HandleFailedDelegatefunction correctly handles the failure of a delegate message, including decrementing the withdrawal waitgroup and triggering the redemption rate calculation. The implementation is consistent with the rest of the file.
- 855-896: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1143-1175]
The
HandleUpdatedWithdrawAddressfunction correctly handles the update of a withdraw address, including updating the zone information. The implementation is consistent with the rest of the file.
- 855-896: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1177-1196]
The
GetValidatorForTokenfunction correctly retrieves the validator for a given token based on its denomination, including error handling. The implementation is consistent with the rest of the file.
- 855-896: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1198-1277]
The
UpdateDelegationRecordsForAddressfunction correctly updates delegation records for a given address, including triggering ICQ requests for new or updated records. The implementation is consistent with the rest of the file.
- 855-896: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1279-1348]
The
UpdateDelegationRecordForAddressfunction correctly updates a specific delegation record for a given address, including updating the delegation amount and triggering the redemption rate calculation. The implementation is consistent with the rest of the file.
- 855-896: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1350-1377]
The
HandleWithdrawRewardsfunction correctly handles the withdrawal of rewards, including decrementing the withdrawal waitgroup and triggering the redemption rate calculation. The implementation is consistent with the rest of the file.
- 855-896: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1379-1403]
The
TriggerRedemptionRatefunction correctly triggers the calculation of the redemption rate, including making an ICQ request to query all balances and then distributing rewards. The implementation is consistent with the rest of the file.
- 855-896: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1405-1458]
The
DistributeRewardsFromWithdrawAccountfunction correctly distributes rewards from the withdrawal account, including querying all balances, calculating fees, and preparing reward distribution messages. The implementation is consistent with the rest of the file.
|
@joe-bowman I'm going to fix lints, and this PR looks g2g otherwise. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- x/interchainstaking/keeper/ibc_packet_handlers.go (3 hunks)
- x/interchainstaking/keeper/ibc_packet_handlers_test.go (4 hunks)
- x/interchainstaking/keeper/withdrawal_record.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/interchainstaking/keeper/ibc_packet_handlers.go
- x/interchainstaking/keeper/withdrawal_record.go
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1222 +/- ##
==========================================
+ Coverage 63.13% 63.16% +0.02%
==========================================
Files 171 171
Lines 11247 11261 +14
==========================================
+ Hits 7101 7113 +12
- Misses 3422 3425 +3
+ Partials 724 723 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/interchainstaking/keeper/withdrawal_record.go (3 hunks)
Additional comments: 3
x/interchainstaking/keeper/withdrawal_record.go (3)
- 9-10: The addition of
gogotypesandsdkmathimports is appropriate for the protobuf encoding and mathematical operations required by the new functionality.- 20-27: The
InitWithdrawalRecordSequencefunction initializes the sequence to a hardcoded value of 512. Consider making this value configurable or documenting the reason for this specific choice.- 16-55: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-183]
Overall, the changes in
withdrawal_record.gointegrate well with the existing codebase, enhancing functionality without disrupting existing logic. The modifications align with the PR's objectives and are consistent with Cosmos SDK patterns.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/interchainstaking/keeper/withdrawal_record.go (3 hunks)
Additional comments: 1
x/interchainstaking/keeper/withdrawal_record.go (1)
- 9-10: The addition of
gogotypesandsdkmathimports aligns with the usage of protobuf for encoding and the new mathematical operations introduced in this file. Ensure that these libraries are used consistently throughout the project to avoid introducing multiple dependencies for similar functionalities.
1. Summary
Fixes #1201
Instead of spawning a new withdrawal record for every failed tx, look for an existing requeued withdrawal record to update first.
2.Type of change
3. Implementation details
Instead of blindly spawning a requeued withdrawal record for every failed msg (because withdrawal record often spawn multiple validators), we should check for an existing requeued withdrawal record first.
Note: we also replace the way we store the
WithdrawalRecordSequence, to be a protobuf encoded uint64, instead of storing the raw bytes.InitWithdrawalRecordSequencemust be called by the upgrade handler.Todo: need to write an upgrade handler to group existing requeued record by user.
Summary by CodeRabbit