Handle non staking tokens #1132
Conversation
… order for under/overflow checks
… to delegate account
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes revolve around enhancing the handling and validation of withdrawal processes, improving error handling, logging, and the management of withdrawal waitgroups across the interchain staking module. A notable addition is the mechanism to deal with unstakable tokens by sending them to the withdrawal account, aligning with the requirement to handle tokens that are not meant for staking, such as those received from third-party airdrops. Changes
Assessment against linked issues
Poem
x/interchainstaking/keeper/callbacks_test.go, x/interchainstaking/keeper/callbacks.go: ## Short summary
x/interchainstaking/keeper/ibc_packet_handlers_test.go: ## Short summary In the x/interchainstaking/keeper/delegation.go: ### Short Summary
x/interchainstaking/keeper/hooks.go: ## Short summary In the provided diff for x/interchainstaking/keeper/keeper.go: ### Short Summary x/interchainstaking/keeper/ibc_packet_handlers.go: ## Short summary
x/interchainstaking/types/zones.go: ## Short summary
x/interchainstaking/types/zones_test.go: ## Short summary
x/interchainstaking/keeper/receipt_test.go: ## Short summary In the x/interchainstaking/transfer_middleware.go: ### Short Summary x/interchainstaking/keeper/intent.go: ## Short summary In the Makefile: ## Short summary The Makefile now requires Go version 1.22 instead of 1.21 for building Quicksilver. The error message now includes the detected Go version. .github/workflows/buildtest.yaml: ## Short summary The diff updates the Go version from "1.21" to "1.22" in the setup steps for the GitHub Actions workflow file. It also modifies the target operating systems in the matrix strategy, removing "darwin" as a target OS and adding "arm64" as an architecture for the "darwin" target OS. .github/workflows/codeql.yml: ## Short summary In the .github/workflows/golangci.yml: ## Short summary The change in the .github/workflows/interchaintest.yaml: ## Short summary The diff involves updating the Go version from "1.21" to "1.22" in the workflow setup steps for setting up Go in multiple job configurations. Dockerfile: ## Short summary The Dockerfile has been updated to change the base image from Dockerfile.relayer: ## Short summary The Dockerfile for icq-relayer/Dockerfile: ## Short summary The Dockerfile has been updated to change the base image from proto/Dockerfile: ## Short summary The Dockerfile now uses |
![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: 20
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
docker-compose.ymlis excluded by:!**/*.ymlscripts/config/hermes.tomlis excluded by:!**/*.toml
Files selected for processing (16)
- scripts/setup.sh (2 hunks)
- scripts/simple-test.sh (1 hunks)
- scripts/vars.sh (2 hunks)
- x/interchainstaking/keeper/callbacks.go (5 hunks)
- x/interchainstaking/keeper/callbacks_test.go (10 hunks)
- x/interchainstaking/keeper/delegation.go (3 hunks)
- x/interchainstaking/keeper/delegation_test.go (9 hunks)
- x/interchainstaking/keeper/hooks.go (3 hunks)
- x/interchainstaking/keeper/ibc_packet_handlers.go (9 hunks)
- x/interchainstaking/keeper/ibc_packet_handlers_test.go (16 hunks)
- x/interchainstaking/keeper/keeper.go (1 hunks)
- x/interchainstaking/keeper/receipt.go (4 hunks)
- x/interchainstaking/keeper/validators.go (1 hunks)
- x/interchainstaking/transfer_middleware.go (1 hunks)
- x/interchainstaking/types/zones.go (3 hunks)
- x/interchainstaking/types/zones_test.go (3 hunks)
Additional comments: 43
x/interchainstaking/keeper/validators.go (1)
- 31-39: LGTM!
x/interchainstaking/transfer_middleware.go (1)
- 96-107: Verify the error handling in
HandleMsgTransferto ensure it's robust and correctly manages all potential failure scenarios.x/interchainstaking/keeper/hooks.go (1)
- 173-179: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [133-196]
Ensure consistent usage of
GetWithdrawalWaitgroup,SetWithdrawalWaitgroup, andIncrementWithdrawalWaitgroupmethods throughout the codebase for managingWithdrawalWaitgroup.scripts/vars.sh (2)
- 50-50: LGTM!
- 68-69: Verify that the renamed commands (
icq-relayer) are consistently updated throughout the project to avoid breaking changes.scripts/simple-test.sh (1)
- 111-136: Ensure that the logic for handling withdrawal, delegation, and deposit accounts before performing bank transactions is correctly implemented and robustly handles errors.
x/interchainstaking/types/zones.go (5)
- 26-28: LGTM!
- 45-48: Ensure that
SetWithdrawalWaitgroupcorrectly logs the action and sets the waitgroup value as intended.- 50-58: Verify that
DecrementWithdrawalWaitgroupcorrectly handles underflow and logs relevant information.- 60-66: Ensure that
IncrementWithdrawalWaitgroupcorrectly handles overflow and logs relevant information.- 70-98: Verify that
ValidateCoinsForZonecorrectly handles different coin validations based on zone settings.x/interchainstaking/keeper/delegation.go (1)
- 330-334: The condition
if zone.GetWithdrawalWaitgroup() == 0followed by triggering redemption rate calculation seems unrelated to the primary purpose ofFlushOutstandingDelegations. Ensure this logic is intentional and correctly placed.x/interchainstaking/keeper/receipt.go (1)
- 328-331: The introduction of
NewCompletedReceiptis a good practice for creating receipts. Ensure that the completion time is correctly set and used throughout the system to mark transactions as completed.x/interchainstaking/types/zones_test.go (4)
- 7-7: The import of
logfromgithub.com/tendermint/tendermint/libs/logis added. Ensure this new dependency is justified and used appropriately within the test cases.- 43-52: Refactoring in
TestDecrementWithdrawalWgintroduces logging and usesIncrementWithdrawalWaitgroupandDecrementWithdrawalWaitgroupmethods. Ensure that the logging functionality is correctly implemented and that the test accurately reflects the intended behavior of these methods.- 64-89: In
TestValidateCoinsForZone, new validation scenarios for different coin types have been added. Verify that these scenarios cover all relevant cases and that the assertions correctly reflect the expected behavior ofValidateCoinsForZone.- 43-52:
TestParseMemoFieldsintroduces tests for parsing memo fields from bytes. Ensure that the test cases adequately cover both valid and invalid scenarios and that the assertions accurately reflect the expected parsing outcomes.scripts/setup.sh (2)
- 269-269: Adding
100000000uotherto${VAL_ADDRESS_2}'s genesis account introduces a new token type. Ensure this aligns with the intended token management strategy and that the token denominationuotheris correctly defined elsewhere in the system.- 478-478: The epoch duration changes to
360sforepochand120sfordaymodify the timing of system operations. Confirm these new durations are intended and correctly implemented to meet the system's requirements.x/interchainstaking/keeper/ibc_packet_handlers.go (6)
- 355-371: The logic for handling different cases of
MsgSendbased on the sender and receiver addresses is clear. However, ensure that the logic within each case, especially for delegating rewards and handling withdrawals, is thoroughly tested to prevent any unintended side effects.- 767-779: The handling of failed
MsgSendoperations based on the sender and receiver addresses is appropriate. However, ensure that the specific actions taken in each case, such as logging and potentially requeuing or adjusting logic, are aligned with the intended system behavior for failed transactions.- 956-961: In the
HandleFailedRedeemTokensmethod, the decrement of theWithdrawalWaitgroupis correctly handled. However, ensure that the logic for triggering the redemption rate calculation after a failed batch token redemption is correctly implemented and that it does not inadvertently trigger under incorrect conditions.- 1001-1005: In the
HandleDelegatemethod, the decrement of theWithdrawalWaitgroupand the conditional logic for triggering the redemption rate calculation are correctly implemented. Ensure that the conditions under which the redemption rate calculation is triggered are thoroughly tested to prevent any unintended triggers.- 1045-1049: In the
HandleFailedDelegatemethod, the handling of failed delegation messages is appropriate. Ensure that the decrement of theWithdrawalWaitgroupand the conditional logic for triggering the redemption rate calculation after a failed delegation are correctly implemented and tested.- 1239-1248: In the
HandleWithdrawRewardsmethod, the decrement of theWithdrawalWaitgroupand the conditional logic for triggering the redemption rate calculation after rewards withdrawal are correctly implemented. Ensure that the conditions under which the redemption rate calculation is triggered are thoroughly tested to prevent any unintended triggers.x/interchainstaking/keeper/callbacks_test.go (10)
- 724-724: The method
IncrementWithdrawalWaitgroupis introduced to encapsulate the logic of incrementing theWithdrawalWaitgroup. Ensure that the method correctly handles edge cases and concurrency issues to prevent race conditions or incorrect increments.- 760-761: Similar to the previous comment, verify that
IncrementWithdrawalWaitgroupis properly implemented to handle concurrency and edge cases.- 790-791: As with the previous hunks, ensure that the implementation of
IncrementWithdrawalWaitgroupis robust against concurrency issues and edge cases.- 2450-2451: The change in error handling strategy to avoid blocking queues is noted. It's crucial to ensure that this approach does not lead to unhandled errors or silent failures that could affect the system's stability or data integrity.
- 2533-2533: The introduction of
SetWithdrawalWaitgroupencapsulates the logic for setting the withdrawal waitgroup. Verify that this method properly validates its inputs and handles any potential errors or edge cases.- 2552-2552: Using
GetWithdrawalWaitgroupto retrieve the waitgroup value is a good practice for encapsulation. Ensure that this method is consistent in its return values, especially in concurrent environments.- 2587-2587: The use of
SetWithdrawalWaitgrouphere for initialization purposes should be checked for correctness in terms of input validation and error handling.- 2604-2604: This assertion checks the expected state of
WithdrawalWaitgroupafter a series of operations. Ensure that the logic leading to this state is correctly implemented and accounts for concurrency.- 2622-2622: Again, verify that
SetWithdrawalWaitgroupis correctly implemented, especially in terms of handling edge cases and ensuring data integrity.- 2639-2639: This final assertion checks the state of
WithdrawalWaitgroupand balances. Confirm that the underlying logic accurately reflects the intended operations and that concurrency is properly managed.x/interchainstaking/keeper/delegation_test.go (8)
- 375-375: Ensure
denomis defined and its value is consistent with the test's context to avoid potential confusion or errors.- 405-405: The use of underscores in large numbers improves readability. Good practice.
- 420-420: Consistent use of underscores in large numbers for readability is maintained. Approved.
- 429-429: Formatting large integer values with underscores enhances readability. Continue this practice.
- 483-483: The use of underscores in large integer values is consistent and improves readability. Approved.
- 537-537: Ensure the
denomvariable is appropriately defined and used within the test's context.- 589-589: Maintain consistency in the use of the
denomvariable and formatting of large integer values.- 622-637: Verify that
mockAckis correctly implemented and used to simulate different test scenarios. Ensure error handling is consistent and comprehensive.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (7)
- x/interchainstaking/keeper/callbacks.go (5 hunks)
- x/interchainstaking/keeper/callbacks_test.go (10 hunks)
- x/interchainstaking/keeper/delegation.go (3 hunks)
- x/interchainstaking/keeper/hooks.go (3 hunks)
- x/interchainstaking/keeper/keeper.go (1 hunks)
- x/interchainstaking/types/zones.go (3 hunks)
- x/interchainstaking/types/zones_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (6)
- x/interchainstaking/keeper/callbacks.go
- x/interchainstaking/keeper/callbacks_test.go
- x/interchainstaking/keeper/delegation.go
- x/interchainstaking/keeper/hooks.go
- x/interchainstaking/keeper/keeper.go
- x/interchainstaking/types/zones.go
Additional comments: 4
x/interchainstaking/types/zones_test.go (4)
- 7-7: The import of
logfromgithub.com/tendermint/tendermint/libs/logis added. Ensure this new dependency is justified and used appropriately across the test cases.- 43-52: The test
TestDecrementWithdrawalWgcorrectly tests the increment and decrement of the withdrawal waitgroup, including the scenario where decrementing would lead to an error. Ensure the logging viatestlogis effectively capturing the necessary information for debugging.- 64-89: The test
TestValidateCoinsForZoneeffectively covers various scenarios for coin validation, including valid and invalid formats, and matching against validator addresses. This thorough testing ensures robust validation logic.- 4-10: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [242-308]
The test
TestParseMemoFieldsthoroughly covers the parsing of memo fields from bytes, including handling of invalid data and validation of parsed fields. This ensures the robustness of memo field parsing functionality.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/interchainstaking/keeper/ibc_packet_handlers.go (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/interchainstaking/keeper/ibc_packet_handlers.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/interchainstaking/transfer_middleware.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/interchainstaking/transfer_middleware.go
|
Need to check if this is still failing tests, when I first checked, it was. |
|
tests fail only on amd64 darwin. Will upgrade repo to go v1.22.0 and see if that changes this. Seems like a compatibility issue. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1132 +/- ##
==========================================
+ Coverage 62.65% 62.86% +0.20%
==========================================
Files 172 172
Lines 11331 11394 +63
==========================================
+ Hits 7100 7163 +63
+ Misses 3479 3478 -1
- Partials 752 753 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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/callbacks_test.go (12 hunks)
- x/interchainstaking/keeper/ibc_packet_handlers.go (10 hunks)
- x/interchainstaking/keeper/ibc_packet_handlers_test.go (16 hunks)
Files skipped from review as they are similar to previous changes (3)
- x/interchainstaking/keeper/callbacks_test.go
- x/interchainstaking/keeper/ibc_packet_handlers.go
- x/interchainstaking/keeper/ibc_packet_handlers_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- x/interchainstaking/keeper/callbacks.go (7 hunks)
- x/interchainstaking/keeper/delegation.go (3 hunks)
- x/interchainstaking/keeper/ibc_packet_handlers.go (15 hunks)
- x/interchainstaking/keeper/intent.go (1 hunks)
- x/interchainstaking/keeper/withdrawal_record.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- x/interchainstaking/keeper/callbacks.go
- x/interchainstaking/keeper/delegation.go
- x/interchainstaking/keeper/ibc_packet_handlers.go
Additional comments: 2
x/interchainstaking/keeper/intent.go (1)
- 206-206: Change in logging level from
ErrortoInfois appropriate for non-error conditions.x/interchainstaking/keeper/withdrawal_record.go (1)
- 34-34: Logging level change from
ErrortoInfoinAddWithdrawalRecordaligns with refining log verbosity.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/interchainstaking/keeper/ibc_packet_handlers_test.go (17 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/interchainstaking/keeper/ibc_packet_handlers_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- x/interchainstaking/keeper/callbacks_test.go (14 hunks)
- x/interchainstaking/keeper/ibc_packet_handlers_test.go (18 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/interchainstaking/keeper/callbacks_test.go
- x/interchainstaking/keeper/ibc_packet_handlers_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/interchainstaking/keeper/callbacks.go (11 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/interchainstaking/keeper/callbacks.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- .github/workflows/golangci.yml (1 hunks)
- x/interchainstaking/keeper/keeper.go (1 hunks)
- x/interchainstaking/keeper/withdrawal_record.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- .github/workflows/golangci.yml
- x/interchainstaking/keeper/keeper.go
- x/interchainstaking/keeper/withdrawal_record.go
|
so, that was a very rigorous way to handle this... :) |
1. Summary
Fixes #1118 - Allows deposits of non-staking tokens in deposit/delegate accounts to be collected and handled as rewards
2.Type of change
3. Implementation details
Summary by CodeRabbit
IncrementWithdrawalWaitgroupandSetWithdrawalWaitgroupmethods for better withdrawal management.SendToWithdrawalmethod for transferring coins inkeeper.go.IsDepositAddressmethod to validate deposit addresses.ValidateCoinsForZone.intent.goandwithdrawal_record.gofor better clarity.DepositLsmTxCallbackFailOnNonMatchingValidatorto prevent queue blocking.HandleWithdrawRewardsfunction to useSetWithdrawalWaitgroupfor consistency.WithdrawalWaitgroupusing getter and setter methods across several files.