fix(x/accounts/default/lockup) Lockup account track undelegation when unbonding entry is mature #22254
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
julienrbrt
changed the title
julienrbrt
added
the
backport/v0.52.x
|
maybe user should manually track this delegation through an execution since matured ubd doesnot mean that the fund has returned to the account yet |
|
@sontrinh16 your pull request is missing a changelog! |
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across various files related to lockup accounts in the Cosmos SDK. Key changes include enhancements to the Changes
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
🧹 Outside diff range and nitpick comments (17)
x/accounts/proto/cosmos/accounts/defaults/lockup/lockup.proto (2)
26-37: LGTM: UnbondingEntry message is well-defined, consider adding comments.The
UnbondingEntrymessage is correctly structured and uses appropriate types for each field. It aligns well with the PR objectives for tracking undelegation.Consider adding comments for each field to improve documentation. For example:
message UnbondingEntry { // Unique identifier for the unbonding entry uint64 id = 1; // Timestamp when the unbonding period ends google.protobuf.Timestamp end_time = 2 [(gogoproto.nullable) = false, (amino.dont_omitempty) = true, (gogoproto.stdtime) = true]; // Amount of coins being unbonded cosmos.base.v1beta1.Coin amount = 3 [(gogoproto.nullable) = false, (amino.dont_omitempty) = true]; // Address of the validator from which the delegation is being unbonded string validator_address = 4 [(cosmos_proto.scalar) = "cosmos.ValidatorAddressString"]; }This will enhance the clarity and maintainability of the code.
Missing Implementation for Managing
UnbondingEntryThe
UnbondingEntrymessage has been added tolockup.proto, but there are no corresponding functions or handlers implemented to manage these entries. To fully address the PR objectives, please ensure that the necessary logic for tracking, storing, and handlingUnbondingEntryinstances is included. Specifically, consider implementing:
- Functions to create and update
UnbondingEntryinstances.- Handlers to process matured entries and execute account operations based on them.
- Integration points where
UnbondingEntryis utilized within the account management workflow.🔗 Analysis chain
Line range hint
1-37: Implementation details needed for complete solution.The addition of the
UnbondingEntrymessage is a good start towards addressing the issue described in the PR objectives. However, to fully implement the solution, we need to see additional changes, such as:
- Logic for tracking and storing these
UnbondingEntryinstances.- Implementation of the process to check for matured entries before executing account operations.
- If going with the manual approach, a new message type for the owner to acknowledge unbonding and trigger
TrackUndelegation.Could you provide more information on how these
UnbondingEntryinstances will be used and managed? Here's a script to check for related implementations:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for UnbondingEntry usage and related implementations echo "Searching for UnbondingEntry usage:" rg --type go "UnbondingEntry" -g '!*.pb.go' echo "\nSearching for TrackUndelegation implementation:" rg --type go "TrackUndelegation" echo "\nSearching for new message types related to manual unbonding acknowledgment:" rg --type proto "message\s+.*Unbond.*"Length of output: 35508
Script:
#!/bin/bash # Search for new message types related to manual unbonding acknowledgment in proto files echo "Searching for new message types related to manual unbonding acknowledgment:" rg "message\s+.*Unbond.*" -g '*.proto'Length of output: 1444
Script:
#!/bin/bash # Search for functions handling UnbondingEntry in Go files echo "Searching for functions that interact with UnbondingEntry:" rg "UnbondingEntry" -g '*.go' echo "\nSearching for handlers related to UnbondingEntry operations:" rg "HandleUnbondingEntry" -g '*.go'Length of output: 19860
tests/e2e/accounts/lockup/utils.go (2)
75-85: LGTM with a minor suggestion for consistency.The
queryUnbondingEntriesmethod is well-implemented and follows the existing patterns in the file. It enhances the test suite's capabilities for testing unbonding entries.For consistency with other query methods in this file, consider adding a comment describing the method's purpose, like so:
// queryUnbondingEntries queries the unbonding entries for a given account func (s *E2ETestSuite) queryUnbondingEntries(ctx sdk.Context, app *simapp.SimApp, accAddr []byte) *types.QueryUnbondingEntriesResponse { // ... (existing implementation) }
87-95: LGTM with a suggestion for improved flexibility.The
setupStakingParamsmethod is well-implemented and properly updates the staking parameters for testing purposes.To improve flexibility, consider parameterizing the unbonding time:
func (s *E2ETestSuite) setupStakingParams(ctx sdk.Context, app *simapp.SimApp, unbondingTime time.Duration) { params, err := app.StakingKeeper.Params.Get(ctx) require.NoError(s.T(), err) params.UnbondingTime = unbondingTime err = app.StakingKeeper.Params.Set(ctx, params) require.NoError(s.T(), err) }This change allows for different unbonding times in various test scenarios, enhancing the method's reusability.
x/accounts/proto/cosmos/accounts/defaults/lockup/query.proto (1)
46-53: Changes align with PR objectives, consider updating documentationThe addition of
QueryUnbondingEntriesRequestandQueryUnbondingEntriesResponsemessages aligns well with the PR objectives. These changes support the implementation of manual tracking of unbonding entries, addressing the concerns raised about accurate representation of spendable amounts in lockup accounts.To ensure clarity for users and developers:
- Consider updating the module's documentation to explain the purpose and usage of these new query types.
- It may be helpful to add examples of how these queries will be used in the context of manually acknowledging unbonding and triggering
TrackUndelegation.x/accounts/defaults/lockup/permanent_locking_account.go (1)
106-106: LGTM! Consider grouping related handlers.The addition of
TrackUndelegationEntryhandler is consistent with the PR objectives and follows the existing naming convention.Consider grouping related handlers together for better code organization. For example, you could place
TrackUndelegationEntrynext toUndelegatesince they are both related to undelegation operations.x/accounts/defaults/lockup/utils_test.go (1)
80-82: LGTM! Consider adding a comment for clarity.The change improves the mock context by providing a more realistic response for undelegation messages, which aligns with the PR objective. This will allow for more accurate testing of undelegation-related functionality.
Consider adding a brief comment explaining why this specific amount (1 "test" coin) was chosen for the mock response. This would enhance code readability and make it easier for other developers to understand the test setup.
case "/cosmos.staking.v1beta1.MsgUndelegate": + // Mock a successful undelegation of 1 "test" coin return &stakingtypes.MsgUndelegateResponse{ Amount: sdk.NewCoin("test", math.NewInt(1)), }, nilx/accounts/proto/cosmos/accounts/defaults/lockup/tx.proto (1)
110-119: LGTM! Consider adding a comment for theidfield.The new
MsgTrackUndelegationmessage is well-structured and consistent with other messages in the file. It appropriately uses thecosmos.AddressStringscalar type for thesenderfield and includes necessary options.For improved clarity and consistency with other messages in the file, consider adding a comment for the
idfield to explain its purpose. For example:string sender = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"]; - uint64 id = 2; + // id of the undelegation entry to track + uint64 id = 2;x/accounts/defaults/lockup/delayed_locking_account_test.go (1)
104-115: Improved undelegation testing logicThe changes enhance the test by focusing on the unbonding process, which aligns well with the PR objectives. The new checks for unbonding sequence and entry details provide a more comprehensive verification of the undelegation process.
Consider adding a comment explaining the significance of
ubdSeq-1to improve code readability. For example:// Use ubdSeq-1 as the UnbondingSequence is incremented after the entry is added ubdEntry, err := acc.UnbondEntries.Get(sdkCtx, ubdSeq-1)x/accounts/defaults/lockup/periodic_locking_account_test.go (2)
143-144: Userequire.Equalfor better assertion clarityCurrently, you're asserting string equality using
require.True:require.True(t, ubdEntry.ValidatorAddress == "val_address")For improved readability and consistency with other assertions, use
require.Equal:Apply this diff:
-require.True(t, ubdEntry.ValidatorAddress == "val_address") +require.Equal(t, "val_address", ubdEntry.ValidatorAddress)
154-154: AssertDelegatedFreeamount after undelegation trackingAfter tracking the undelegation, you assert that
DelegatedLockingis zero. It's important to also check theDelegatedFreeamount to confirm that the undelegated tokens are now free:require.True(t, delLocking.Equal(math.ZeroInt()))Add an assertion for
DelegatedFree:Apply this diff:
require.True(t, delLocking.Equal(math.ZeroInt())) +delFree, err := acc.DelegatedFree.Get(ctx, "test") +require.NoError(t, err) +require.True(t, delFree.Equal(math.NewInt(1)))tests/e2e/accounts/lockup/continous_lockup_test_suite.go (2)
Line range hint
1-1: Correct the filename spelling to 'continuous'.The filename
continous_lockup_test_suite.gois misspelled. It should becontinuous_lockup_test_suite.goto reflect correct spelling and improve codebase consistency.Apply this change to correct the filename:
-// File: continous_lockup_test_suite.go +// File: continuous_lockup_test_suite.go
179-191: Enhance test assertions to verifyDelegatedFreeamount after tracking undelegation.Currently, the test checks that
DelegatedLockingamount is zero after tracking undelegation. To fully validate the state, consider adding an assertion to verify that theDelegatedFreeamount has increased accordingly.Apply this diff to include the additional assertion:
require.True(t, delLocking.AmountOf("stake").Equal(math.ZeroInt())) + delFree := lockupAccountInfoResponse.DelegatedFree + require.True(t, delFree.AmountOf("stake").Equal(math.NewInt(100)))This ensures that both the locking and free delegated amounts reflect the expected values after the operation.
api/cosmos/accounts/defaults/lockup/lockup.pulsar.go (1)
1284-1291: Adjust field comments to follow Go conventionsAccording to the Uber Go Style Guide, comments on exported fields should start with the field name. Update the comments for
Id,EndTime,Amount, andValidatorAddressto begin with the field name for clarity and consistency.Apply this diff to correct the comments:
- // entry id + // Id is the entry ID. Id uint64 `protobuf:"varint,1,opt,name=id,proto3" json:"id,omitempty"` - // end time of entry + // EndTime represents the end time of the unbonding entry. EndTime *timestamppb.Timestamp `protobuf:"bytes,2,opt,name=end_time,json=endTime,proto3" json:"end_time,omitempty"` - // unbond amount + // Amount is the unbonded amount. Amount *v1beta1.Coin `protobuf:"bytes,3,opt,name=amount,proto3" json:"amount,omitempty"` - // validator address + // ValidatorAddress is the address of the validator. ValidatorAddress string `protobuf:"bytes,4,opt,name=validator_address,json=validatorAddress,proto3" json:"validator_address,omitempty"`api/cosmos/accounts/defaults/lockup/query.pulsar.go (2)
Line range hint
3570-3600: Correct the field numbering in protobuf descriptorsIn the
QueryLockupAccountInfoResponseand related message types, verify that the field numbers in the protobuf descriptors are correctly incremented and do not conflict with existing fields. Misnumbered fields can lead to serialization issues.Double-check the
.protofiles to ensure that the field numbers are unique and correctly assigned.
Line range hint
3843-3856: Align with Uber Go Style Guide for variable namingIn the generated code, some variables use underscores (e.g.,
file_cosmos_accounts_defaults_lockup_query_proto_depIdxs). According to the Uber Go Style Guide, variable names should be in mixedCaps or mixedCaps with initialisms. While some of these may be auto-generated, consider configuring the code generator to adhere more closely to the style guide if possible.Example:
var fileCosmosAccountsDefaultsLockupQueryProtoDepIdxs = []int32{ /* ... */ }api/cosmos/accounts/defaults/lockup/tx.pulsar.go (1)
6689-6690: Fix the grammatical error in the comment forMsgTrackUndelegation.The comment has a grammatical error and is missing a period at the end. According to the Uber Golang style guide, comments should be complete sentences ending with a period.
Please apply the following diff to correct the comment:
-// MsgTrackUndelegation defines a message that enable lockup account to update delegation tracking +// MsgTrackUndelegation defines a message that enables a lockup account to update delegation tracking.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (3)
x/accounts/defaults/lockup/types/lockup.pb.gois excluded by!**/*.pb.gox/accounts/defaults/lockup/types/query.pb.gois excluded by!**/*.pb.gox/accounts/defaults/lockup/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
- api/cosmos/accounts/defaults/lockup/lockup.pulsar.go (6 hunks)
- api/cosmos/accounts/defaults/lockup/query.pulsar.go (13 hunks)
- api/cosmos/accounts/defaults/lockup/tx.pulsar.go (18 hunks)
- tests/e2e/accounts/lockup/continous_lockup_test_suite.go (2 hunks)
- tests/e2e/accounts/lockup/delayed_lockup_test_suite.go (2 hunks)
- tests/e2e/accounts/lockup/periodic_lockup_test_suite.go (3 hunks)
- tests/e2e/accounts/lockup/permanent_lockup_test_suite.go (3 hunks)
- tests/e2e/accounts/lockup/utils.go (2 hunks)
- x/accounts/defaults/lockup/continuous_locking_account.go (1 hunks)
- x/accounts/defaults/lockup/continuous_locking_account_test.go (1 hunks)
- x/accounts/defaults/lockup/delayed_locking_account.go (1 hunks)
- x/accounts/defaults/lockup/delayed_locking_account_test.go (1 hunks)
- x/accounts/defaults/lockup/lockup.go (5 hunks)
- x/accounts/defaults/lockup/periodic_locking_account.go (1 hunks)
- x/accounts/defaults/lockup/periodic_locking_account_test.go (1 hunks)
- x/accounts/defaults/lockup/permanent_locking_account.go (1 hunks)
- x/accounts/defaults/lockup/permanent_locking_account_test.go (1 hunks)
- x/accounts/defaults/lockup/utils_test.go (1 hunks)
- x/accounts/proto/cosmos/accounts/defaults/lockup/lockup.proto (2 hunks)
- x/accounts/proto/cosmos/accounts/defaults/lockup/query.proto (1 hunks)
- x/accounts/proto/cosmos/accounts/defaults/lockup/tx.proto (1 hunks)
🧰 Additional context used
📓 Path-based instructions (18)
api/cosmos/accounts/defaults/lockup/lockup.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.api/cosmos/accounts/defaults/lockup/query.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.api/cosmos/accounts/defaults/lockup/tx.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/e2e/accounts/lockup/continous_lockup_test_suite.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"tests/e2e/accounts/lockup/delayed_lockup_test_suite.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"tests/e2e/accounts/lockup/periodic_lockup_test_suite.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"tests/e2e/accounts/lockup/permanent_lockup_test_suite.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"tests/e2e/accounts/lockup/utils.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"x/accounts/defaults/lockup/continuous_locking_account.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/accounts/defaults/lockup/continuous_locking_account_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/accounts/defaults/lockup/delayed_locking_account.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/accounts/defaults/lockup/delayed_locking_account_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/accounts/defaults/lockup/lockup.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/accounts/defaults/lockup/periodic_locking_account.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/accounts/defaults/lockup/periodic_locking_account_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/accounts/defaults/lockup/permanent_locking_account.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/accounts/defaults/lockup/permanent_locking_account_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"x/accounts/defaults/lockup/utils_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🪛 GitHub Check: CodeQL
api/cosmos/accounts/defaults/lockup/lockup.pulsar.go
[warning] 805-805: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 807-807: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain haltapi/cosmos/accounts/defaults/lockup/query.pulsar.go
[warning] 1785-1785: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 1787-1787: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 2214-2214: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 2216-2216: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain haltapi/cosmos/accounts/defaults/lockup/tx.pulsar.go
[warning] 4358-4358: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 4360-4360: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
🔇 Additional comments (18)
x/accounts/proto/cosmos/accounts/defaults/lockup/lockup.proto (1)
7-9: LGTM: New imports are appropriate for the added message.The new imports for
cosmos_proto/cosmos.protoandgoogle/protobuf/timestamp.protoare correctly added to support the newUnbondingEntrymessage definition.tests/e2e/accounts/lockup/utils.go (1)
Line range hint
1-95: Overall, the changes enhance the e2e test suite capabilities.The additions to
tests/e2e/accounts/lockup/utils.goimprove the testing framework for the lockup module. The new methodsqueryUnbondingEntriesandsetupStakingParamsallow for more comprehensive e2e tests, particularly for testing unbonding entries and custom staking parameters.The changes adhere to the Uber Golang style guide and are consistent with the existing code structure. These enhancements will contribute to better test coverage for the changes associated with the pull request.
x/accounts/proto/cosmos/accounts/defaults/lockup/query.proto (2)
46-47: LGTM: QueryUnbondingEntriesRequest message addedThe addition of the
QueryUnbondingEntriesRequestmessage is appropriate. It follows the common pattern of using an empty message for simple requests that don't require additional parameters.
49-53: LGTM: QueryUnbondingEntriesResponse message added, but verify UnbondingEntry typeThe
QueryUnbondingEntriesResponsemessage is well-structured for returning a list of unbonding entries. However, please ensure that theUnbondingEntrytype is properly defined and imported.Run the following script to verify the UnbondingEntry type definition:
✅ Verification successful
Let's attempt to locate the
UnbondingEntrytype definition by searching within all.protofiles.
Verified: UnbondingEntry type is properly defined
The
UnbondingEntrytype is defined inx/accounts/proto/cosmos/accounts/defaults/lockup/lockup.proto, ensuring it is properly defined and accessible for theQueryUnbondingEntriesResponsemessage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the UnbondingEntry type definition # Test: Search for UnbondingEntry definition in proto files rg --type proto 'message\s+UnbondingEntry' # Test: Check for imports of files that might contain UnbondingEntry rg --type proto 'import.*UnbondingEntry'Length of output: 147
Script:
#!/bin/bash # Description: Locate the UnbondingEntry type definition within .proto files # Find all .proto files and search for the UnbondingEntry message fd --extension proto --type f | xargs rg 'message\s+UnbondingEntry'Length of output: 156
x/accounts/defaults/lockup/permanent_locking_account.go (1)
111-111: LGTM! Verify the implementation of QueryUnbondingEntries.The addition of
QueryUnbondingEntrieshandler is consistent with the PR objectives and follows the existing naming convention.Please ensure that the
QueryUnbondingEntriesmethod is properly implemented in thePermanentLockingAccountstruct. Run the following script to verify:x/accounts/defaults/lockup/utils_test.go (1)
Line range hint
1-124: Overall assessment: The changes improve test utilities.The modification to the
newMockContextfunction enhances the simulation of undelegation messages in the test environment. This change aligns well with the PR objectives and should lead to more accurate testing of undelegation-related functionality in the lockup package.x/accounts/defaults/lockup/delayed_locking_account_test.go (1)
Line range hint
1-190: Overall assessment of changesThe modifications to the
TestDelayedAccountUndelegatefunction effectively address the PR objectives by improving the testing of the undelegation process. The focus on unbonding sequences and entries provides a more accurate representation of the account's state during undelegation.The test coverage for the changes associated with the pull request appears sufficient, as it directly tests the new functionality related to tracking undelegation entries.
x/accounts/defaults/lockup/continuous_locking_account.go (1)
203-203: LGTM! Verify the implementation of QueryUnbondingEntries.The addition of the
QueryUnbondingEntriesquery handler registration is appropriate and aligns with the PR objectives. This enhancement will allow theContinuousLockingAccountto handle queries related to unbonding entries.To ensure completeness, let's verify the implementation of the
QueryUnbondingEntriesmethod:✅ Verification successful
Implementation of
QueryUnbondingEntriesVerified SuccessfullyThe
QueryUnbondingEntriesmethod is correctly implemented inx/accounts/defaults/lockup/lockup.go. The registration incontinuous_locking_account.goproperly references this method, ensuring that unbonding entries are handled as intended.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of QueryUnbondingEntries method # Test: Search for the QueryUnbondingEntries method implementation rg --type go -A 5 'func \(.*\) QueryUnbondingEntries\('Length of output: 644
x/accounts/defaults/lockup/periodic_locking_account.go (1)
346-346: LGTM. Please provide implementation details forQueryUnbondingEntries.The addition of the
QueryUnbondingEntriesquery handler aligns with the PR objectives to address issues with the lockup account's handling of undelegation. This change enhances the account's capabilities to manage and respond to queries about unbonding processes.Could you please provide the implementation details of the
QueryUnbondingEntriesmethod? This will help ensure that it correctly handles the querying of unbonding entries and aligns with the overall functionality of thePeriodicLockingAccount.tests/e2e/accounts/lockup/permanent_lockup_test_suite.go (1)
26-26: Initialize staking parameters for accurate test executionSetting up staking parameters ensures that the staking-related tests operate under the intended conditions, preventing unexpected behavior.
x/accounts/defaults/lockup/continuous_locking_account_test.go (1)
121-121: Confirm delegated locking amount is updated correctlyThe assertion that
delLockingequals zero after tracking the undelegation entry ensures that the delegated locking amount is updated appropriately. This verification is crucial to confirm that the undelegation logic functions as expected.tests/e2e/accounts/lockup/delayed_lockup_test_suite.go (1)
26-26: LGTMThe addition of
s.setupStakingParams(ctx, app)correctly initializes the staking parameters necessary for the test, ensuring that the subsequent staking operations function as expected.tests/e2e/accounts/lockup/continous_lockup_test_suite.go (1)
26-26: Verify the implementation ofsetupStakingParams.Ensure that the
s.setupStakingParams(ctx, app)function is properly defined and sets up the staking parameters as intended. This setup is crucial for the tests that depend on staking configurations.If
setupStakingParamsis not defined, please implement it or remove the call if it's unnecessary.tests/e2e/accounts/lockup/periodic_lockup_test_suite.go (1)
26-26: Configuration of staking parameters is appropriateThe addition of
s.setupStakingParams(ctx, app)initializes the staking parameters necessary for the test environment. This ensures that staking-related operations perform as expected.api/cosmos/accounts/defaults/lockup/query.pulsar.go (3)
1671-1996: Ensure consistency in method implementationsThe methods for
fastReflection_QueryUnbondingEntriesRequestare generated but lack specific implementations due to the empty struct. Verify if this is intentional. If the request type does not contain any fields, consider whether it's necessary, or if fields should be added.Please confirm that the empty implementation is appropriate for your use case.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 1785-1785: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 1787-1787: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
2214-2216: Avoid potential panics in consensus methodsStatic analysis tools have identified possible panics in the consensus-related methods, which could cause a chain halt.
This issue has been previously flagged. Ensure that proper error handling is implemented to prevent consensus failures.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 2214-2214: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 2216-2216: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
2050-2290:⚠️ Potential issueHandle potential nil pointer dereferences
In the
fastReflection_QueryUnbondingEntriesResponsemethods, ensure that nil checks are in place when accessing pointers. While the generated code handles some nil checks, it's crucial to prevent possible panics due to nil pointer dereferences, especially in methods likeGet,Set, andMutable.Review the methods to ensure that all pointer accesses are safe. For example:
func (x *fastReflection_QueryUnbondingEntriesResponse) Get(descriptor protoreflect.FieldDescriptor) protoreflect.Value { if x == nil { // Handle nil receiver } // Existing implementation... }🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 2214-2214: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 2216-2216: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain haltapi/cosmos/accounts/defaults/lockup/tx.pulsar.go (1)
4181-4647: LGTM!The code correctly defines the new message type
MsgTrackUndelegationand its associated methods, adhering to the protobuf definitions and Go coding conventions.🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 4358-4358: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
[warning] 4360-4360: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
| ubdSeq, err := acc.UnbondingSequence.Peek(sdkCtx) | ||
| require.NoError(t, err) | ||
| // sequence should be the previous one | ||
| ubdEntry, err := acc.UnbondEntries.Get(sdkCtx, ubdSeq-1) | ||
| require.NoError(t, err) | ||
| require.True(t, ubdEntry.Amount.Amount.Equal(math.NewInt(1))) | ||
| require.True(t, ubdEntry.ValidatorAddress == "val_address") | ||
|
|
||
| _, err = acc.TrackUndelegationEntry(sdkCtx, &lockuptypes.MsgTrackUndelegation{ | ||
| Sender: "owner", | ||
| Id: ubdSeq - 1, | ||
| }) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage and improve error handling
The additions to TestPermanentAccountUndelegate improve the test coverage by verifying the state after undelegation. However, consider the following suggestions to further enhance the test:
- Add assertions to verify the state before undelegation, providing a more comprehensive check.
- Use
require.NoErrorconsistently for all error checks to ensure the test fails immediately on any error. - Improve the comment on line 83 to be more descriptive, e.g., "Retrieve the unbonding entry for the previous sequence".
- Add assertions to verify the state after tracking the undelegation entry.
Here's a suggested improvement:
// Verify state before undelegation
delLockingBefore, err := acc.DelegatedLocking.Get(ctx, "test")
require.NoError(t, err)
require.True(t, delLockingBefore.Equal(math.NewInt(1)))
// Undelegate
_, err = acc.Undelegate(sdkCtx, &lockuptypes.MsgUndelegate{
Sender: "owner",
ValidatorAddress: "val_address",
Amount: sdk.NewCoin("test", math.NewInt(1)),
})
require.NoError(t, err)
ubdSeq, err := acc.UnbondingSequence.Peek(sdkCtx)
require.NoError(t, err)
// Retrieve the unbonding entry for the previous sequence
ubdEntry, err := acc.UnbondEntries.Get(sdkCtx, ubdSeq-1)
require.NoError(t, err)
require.True(t, ubdEntry.Amount.Amount.Equal(math.NewInt(1)))
require.Equal(t, "val_address", ubdEntry.ValidatorAddress)
_, err = acc.TrackUndelegationEntry(sdkCtx, &lockuptypes.MsgTrackUndelegation{
Sender: "owner",
Id: ubdSeq - 1,
})
require.NoError(t, err)
// Verify state after tracking undelegation
delLockingAfter, err := acc.DelegatedLocking.Get(ctx, "test")
require.NoError(t, err)
require.True(t, delLockingAfter.Equal(math.ZeroInt()))These changes will provide a more comprehensive test of the undelegation process.
Consider adding a brief comment explaining the purpose of TrackUndelegationEntry to improve code readability.
| @@ -149,4 +149,5 @@ func (dva DelayedLockingAccount) RegisterExecuteHandlers(builder *accountstd.Exe | |||
|
|
|||
| func (dva DelayedLockingAccount) RegisterQueryHandlers(builder *accountstd.QueryBuilder) { | |||
| accountstd.RegisterQueryHandler(builder, dva.QueryVestingAccountInfo) | |||
| accountstd.RegisterQueryHandler(builder, dva.QueryUnbondingEntries) | |||
There was a problem hiding this comment.
💡 Codebase verification
🔍 Missing Implementation of QueryUnbondingEntries Method
The QueryUnbondingEntries query handler has been registered, but the implementation of the QueryUnbondingEntries method was not found in the codebase.
- Ensure that the
QueryUnbondingEntriesmethod is implemented within theDelayedLockingAccountstruct. - Verify that the method adheres to the expected signature and correctly handles the request and response types.
🔗 Analysis chain
LGTM. Verify the implementation of QueryUnbondingEntries.
The addition of the QueryUnbondingEntries query handler is consistent with the PR objectives and follows the established pattern for registering query handlers. However, the implementation of this method is not visible in the current file.
Please run the following script to verify the existence and implementation of the QueryUnbondingEntries method:
If the method is not found or the types are not defined, please implement them to ensure the newly added query handler functions correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of QueryUnbondingEntries method
# Test: Search for the QueryUnbondingEntries method implementation
ast-grep --lang go --pattern $'func (dva DelayedLockingAccount) QueryUnbondingEntries(ctx context.Context, req *lockuptypes.QueryUnbondingEntriesRequest) (*lockuptypes.QueryUnbondingEntriesResponse, error) {
$$$
}'
# Test: Check if the QueryUnbondingEntriesRequest and QueryUnbondingEntriesResponse types are defined
rg --type go -e 'type QueryUnbondingEntriesRequest struct' -e 'type QueryUnbondingEntriesResponse struct'
Length of output: 693
| t.Run("ok - execute tracking unbonding entry", func(t *testing.T) { | ||
| msg := &types.MsgTrackUndelegation{ | ||
| Sender: ownerAddrStr, | ||
| Id: 0, | ||
| } | ||
| err = s.executeTx(ctx, msg, app, accountAddr, accOwner) | ||
| require.NoError(t, err) | ||
|
|
||
| // check if tracking is updated accordingly | ||
| lockupAccountInfoResponse := s.queryLockupAccInfo(ctx, app, accountAddr) | ||
| delLocking := lockupAccountInfoResponse.DelegatedLocking | ||
| require.True(t, delLocking.AmountOf("stake").Equal(math.ZeroInt())) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test cases for multiple unbonding entries
Currently, the test tracks undelegation for a single entry with Id: 0. To ensure comprehensive coverage, consider adding test cases that handle multiple unbonding entries and verify that tracking works correctly for each.
| require.True(t, entries[0].Amount.Amount.Equal(math.NewInt(100))) | ||
| require.True(t, entries[0].ValidatorAddress == val.OperatorAddress) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use require.Equal for clearer assertions
Using require.Equal instead of require.True with an equality condition provides more informative error messages if the test fails.
Apply this diff:
- require.True(t, entries[0].ValidatorAddress == val.OperatorAddress)
+ require.Equal(t, val.OperatorAddress, entries[0].ValidatorAddress)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| require.True(t, entries[0].Amount.Amount.Equal(math.NewInt(100))) | |
| require.True(t, entries[0].ValidatorAddress == val.OperatorAddress) | |
| require.True(t, entries[0].Amount.Amount.Equal(math.NewInt(100))) | |
| require.Equal(t, val.OperatorAddress, entries[0].ValidatorAddress) |
| // check if an entry is added | ||
| unbondingEntriesResponse := s.queryUnbondingEntries(ctx, app, accountAddr) | ||
| entries := unbondingEntriesResponse.UnbondingEntries | ||
| require.True(t, entries[0].Amount.Amount.Equal(math.NewInt(100))) | ||
| require.True(t, entries[0].ValidatorAddress == val.OperatorAddress) |
There was a problem hiding this comment.
Check for empty unbonding entries before accessing
Accessing entries[0] without verifying that entries is not empty could lead to an index out of range error if no unbonding entries are present. Please add a check to ensure entries contains at least one element before accessing it.
Apply this diff to add the length check:
+ require.Greater(t, len(entries), 0)
require.True(t, entries[0].Amount.Amount.Equal(math.NewInt(100)))
require.Equal(t, val.OperatorAddress, entries[0].ValidatorAddress)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // check if an entry is added | |
| unbondingEntriesResponse := s.queryUnbondingEntries(ctx, app, accountAddr) | |
| entries := unbondingEntriesResponse.UnbondingEntries | |
| require.True(t, entries[0].Amount.Amount.Equal(math.NewInt(100))) | |
| require.True(t, entries[0].ValidatorAddress == val.OperatorAddress) | |
| // check if an entry is added | |
| unbondingEntriesResponse := s.queryUnbondingEntries(ctx, app, accountAddr) | |
| entries := unbondingEntriesResponse.UnbondingEntries | |
| require.Greater(t, len(entries), 0) | |
| require.True(t, entries[0].Amount.Amount.Equal(math.NewInt(100))) | |
| require.Equal(t, val.OperatorAddress, entries[0].ValidatorAddress) |
x/accounts/defaults/lockup/lockup.go
Outdated
| entry, err := bva.UnbondEntries.Get(ctx, msg.Id) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Handle missing unbonding entries explicitly
When retrieving the unbonding entry with bva.UnbondEntries.Get(ctx, msg.Id), if the entry does not exist, it may return an error that's not descriptive. To improve user experience, check for a collections.ErrNotFound error and return a more specific message.
Apply this diff to enhance error handling:
entry, err := bva.UnbondEntries.Get(ctx, msg.Id)
if err != nil {
- return nil, err
+ if errors.Is(err, collections.ErrNotFound) {
+ return nil, sdkerrors.ErrNotFound.Wrapf("unbonding entry with ID %d not found", msg.Id)
+ }
+ return nil, err
}This provides clearer feedback when an unbonding entry is not found.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| entry, err := bva.UnbondEntries.Get(ctx, msg.Id) | |
| if err != nil { | |
| return nil, err | |
| } | |
| entry, err := bva.UnbondEntries.Get(ctx, msg.Id) | |
| if err != nil { | |
| if errors.Is(err, collections.ErrNotFound) { | |
| return nil, sdkerrors.ErrNotFound.Wrapf("unbonding entry with ID %d not found", msg.Id) | |
| } | |
| return nil, err | |
| } |
x/accounts/defaults/lockup/lockup.go
Outdated
| UnbondEntries: collections.NewMap(d.SchemaBuilder, UnbondEntriesPrefix, "unbond_entries", collections.Uint64Key, codec.CollValue[lockuptypes.UnbondingEntry](d.LegacyStateCodec)), | ||
| UnbondingSequence: collections.NewSequence(d.SchemaBuilder, UnbondingSequencePrefix, "unbonding_sequence"), |
There was a problem hiding this comment.
Initialize renamed field correctly in newBaseLockup
Following the renaming of WithdrawedCoins to WithdrawnCoins, make sure to update its initialization in the newBaseLockup function.
Apply this diff:
BaseLockup := &BaseLockup{
Owner: collections.NewItem(d.SchemaBuilder, OwnerPrefix, "owner", collections.BytesValue),
OriginalLocking: collections.NewMap(d.SchemaBuilder, OriginalLockingPrefix, "original_locking", collections.StringKey, sdk.IntValue),
DelegatedFree: collections.NewMap(d.SchemaBuilder, DelegatedFreePrefix, "delegated_free", collections.StringKey, sdk.IntValue),
DelegatedLocking: collections.NewMap(d.SchemaBuilder, DelegatedLockingPrefix, "delegated_locking", collections.StringKey, sdk.IntValue),
- WithdrawedCoins: collections.NewMap(d.SchemaBuilder, WithdrawedCoinsPrefix, "withdrawed_coins", collections.StringKey, sdk.IntValue),
+ WithdrawnCoins: collections.NewMap(d.SchemaBuilder, WithdrawnCoinsPrefix, "withdrawn_coins", collections.StringKey, sdk.IntValue),
UnbondEntries: collections.NewMap(d.SchemaBuilder, UnbondEntriesPrefix, "unbond_entries", collections.Uint64Key, codec.CollValue[lockuptypes.UnbondingEntry](d.LegacyStateCodec)),
UnbondingSequence: collections.NewSequence(d.SchemaBuilder, UnbondingSequencePrefix, "unbonding_sequence"),
addressCodec: d.AddressCodec,Also, update the prefix variable if necessary.
Committable suggestion was skipped due to low confidence.
| var ( | ||
| md_QueryUnbondingEntriesRequest protoreflect.MessageDescriptor | ||
| ) | ||
|
|
||
| func init() { | ||
| file_cosmos_accounts_defaults_lockup_query_proto_init() | ||
| md_QueryUnbondingEntriesRequest = File_cosmos_accounts_defaults_lockup_query_proto.Messages().ByName("QueryUnbondingEntriesRequest") | ||
| } | ||
|
|
||
| var _ protoreflect.Message = (*fastReflection_QueryUnbondingEntriesRequest)(nil) | ||
|
|
||
| type fastReflection_QueryUnbondingEntriesRequest QueryUnbondingEntriesRequest | ||
|
|
||
| func (x *QueryUnbondingEntriesRequest) ProtoReflect() protoreflect.Message { | ||
| return (*fastReflection_QueryUnbondingEntriesRequest)(x) | ||
| } | ||
|
|
||
| func (x *QueryUnbondingEntriesRequest) slowProtoReflect() protoreflect.Message { | ||
| mi := &file_cosmos_accounts_defaults_lockup_query_proto_msgTypes[2] | ||
| if protoimpl.UnsafeEnabled && x != nil { | ||
| ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) | ||
| if ms.LoadMessageInfo() == nil { | ||
| ms.StoreMessageInfo(mi) | ||
| } | ||
| return ms | ||
| } | ||
| return mi.MessageOf(x) | ||
| } |
There was a problem hiding this comment.
Add documentation comments for new message types
The newly introduced QueryUnbondingEntriesRequest lacks a documentation comment explaining its purpose and usage. According to the Go coding guidelines and best practices, exported types and functions should include comments.
Consider adding a comment like:
// QueryUnbondingEntriesRequest is the request type for querying unbonding entries.
type QueryUnbondingEntriesRequest struct {
// Fields...
}| // QueryUnbondingEntriesRequest is used to query the lockup account unbonding entries. | ||
| type QueryUnbondingEntriesRequest struct { | ||
| state protoimpl.MessageState | ||
| sizeCache protoimpl.SizeCache | ||
| unknownFields protoimpl.UnknownFields | ||
| } | ||
|
|
||
| func (x *QueryUnbondingEntriesRequest) Reset() { | ||
| *x = QueryUnbondingEntriesRequest{} | ||
| if protoimpl.UnsafeEnabled { | ||
| mi := &file_cosmos_accounts_defaults_lockup_query_proto_msgTypes[2] | ||
| ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) | ||
| ms.StoreMessageInfo(mi) | ||
| } | ||
| } | ||
|
|
||
| func (x *QueryUnbondingEntriesRequest) String() string { | ||
| return protoimpl.X.MessageStringOf(x) | ||
| } | ||
|
|
||
| func (*QueryUnbondingEntriesRequest) ProtoMessage() {} | ||
|
|
||
| // Deprecated: Use QueryUnbondingEntriesRequest.ProtoReflect.Descriptor instead. | ||
| func (*QueryUnbondingEntriesRequest) Descriptor() ([]byte, []int) { | ||
| return file_cosmos_accounts_defaults_lockup_query_proto_rawDescGZIP(), []int{2} | ||
| } | ||
|
|
||
| // QueryUnbondingEntriesResponse returns the lockup account unbonding entries. | ||
| type QueryUnbondingEntriesResponse struct { | ||
| state protoimpl.MessageState | ||
| sizeCache protoimpl.SizeCache | ||
| unknownFields protoimpl.UnknownFields | ||
|
|
||
| // UnbondingEntry defines the list of unbonding entries. | ||
| UnbondingEntries []*UnbondingEntry `protobuf:"bytes,1,rep,name=unbonding_entries,json=unbondingEntries,proto3" json:"unbonding_entries,omitempty"` | ||
| } | ||
|
|
||
| func (x *QueryUnbondingEntriesResponse) Reset() { | ||
| *x = QueryUnbondingEntriesResponse{} | ||
| if protoimpl.UnsafeEnabled { | ||
| mi := &file_cosmos_accounts_defaults_lockup_query_proto_msgTypes[3] | ||
| ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) | ||
| ms.StoreMessageInfo(mi) | ||
| } | ||
| } | ||
|
|
||
| func (x *QueryUnbondingEntriesResponse) String() string { | ||
| return protoimpl.X.MessageStringOf(x) | ||
| } | ||
|
|
||
| func (*QueryUnbondingEntriesResponse) ProtoMessage() {} | ||
|
|
||
| // Deprecated: Use QueryUnbondingEntriesResponse.ProtoReflect.Descriptor instead. | ||
| func (*QueryUnbondingEntriesResponse) Descriptor() ([]byte, []int) { | ||
| return file_cosmos_accounts_defaults_lockup_query_proto_rawDescGZIP(), []int{3} | ||
| } | ||
|
|
||
| func (x *QueryUnbondingEntriesResponse) GetUnbondingEntries() []*UnbondingEntry { | ||
| if x != nil { | ||
| return x.UnbondingEntries | ||
| } | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Add documentation comments for new response types
The QueryUnbondingEntriesResponse struct is missing a documentation comment. Following the Uber Go Style Guide, exported types should have comments to improve code readability and maintainability.
Add a comment to describe the response type:
// QueryUnbondingEntriesResponse is the response type containing unbonding entries.
type QueryUnbondingEntriesResponse struct {
// Fields...
}
sontrinh16
requested review from
facundomedica,
JulianToledano and
ziscky
as code owners
| x.Entries = *clv.list | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.accounts.defaults.lockup.v1.UnbondingEntries")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.accounts.defaults.lockup.v1.UnbondingEntries")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.accounts.defaults.lockup.v1.UnbondingEntries does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| x.ValidatorAddress = value.Interface().(string) | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.accounts.defaults.lockup.v1.UnbondingEntry")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/integration/accounts/lockup/utils.go (1)
94-94: Remove unnecessary type conversionThe expression
time.Second * 10already results in atime.Duration, so wrapping it withtime.Duration()is unnecessary and can be omitted for clarity.Apply this diff to remove the unnecessary conversion:
- params.UnbondingTime = time.Duration(time.Second * 10) + params.UnbondingTime = time.Second * 10🧰 Tools
🪛 golangci-lint (1.62.2)
94-94: unnecessary conversion
(unconvert)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (7)
api/cosmos/accounts/defaults/lockup/v1/lockup.pulsar.go(6 hunks)api/cosmos/accounts/defaults/lockup/v1/query.pulsar.go(14 hunks)tests/integration/accounts/lockup/continous_lockup_test_suite.go(3 hunks)tests/integration/accounts/lockup/delayed_lockup_test_suite.go(4 hunks)tests/integration/accounts/lockup/periodic_lockup_test_suite.go(4 hunks)tests/integration/accounts/lockup/permanent_lockup_test_suite.go(3 hunks)tests/integration/accounts/lockup/utils.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/integration/accounts/lockup/permanent_lockup_test_suite.go
- tests/integration/accounts/lockup/delayed_lockup_test_suite.go
- tests/integration/accounts/lockup/continous_lockup_test_suite.go
- tests/integration/accounts/lockup/periodic_lockup_test_suite.go
🧰 Additional context used
📓 Path-based instructions (3)
api/cosmos/accounts/defaults/lockup/v1/query.pulsar.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
api/cosmos/accounts/defaults/lockup/v1/lockup.pulsar.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
tests/integration/accounts/lockup/utils.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
🪛 golangci-lint (1.62.2)
tests/integration/accounts/lockup/utils.go
94-94: unnecessary conversion
(unconvert)
🔇 Additional comments (4)
api/cosmos/accounts/defaults/lockup/v1/lockup.pulsar.go (1)
Line range hint 594-2020: Additions are acceptable and conform to style guidelines
The new message types UnbondingEntries and UnbondingEntry are appropriately defined. The generated code adheres to Go conventions and the Uber Go Style Guide. Field naming is consistent, initialisms are correctly capitalized, and the code integrates seamlessly with existing structures.
tests/integration/accounts/lockup/utils.go (2)
75-87: Tests cover unbonding entries appropriately
The queryUnbondingEntries method enhances the test suite by adding the ability to query unbonding entries, ensuring that the new functionality is adequately tested. The implementation correctly constructs the request and handles the response, following the established testing patterns.
89-97: setupStakingParams method correctly updates staking parameters
The setupStakingParams method appropriately configures the staking parameters for the tests. By setting the UnbondingTime to a shorter duration, it allows the tests to execute more efficiently. The method follows best practices for modifying module parameters within tests.
🧰 Tools
🪛 golangci-lint (1.62.2)
94-94: unnecessary conversion
(unconvert)
api/cosmos/accounts/defaults/lockup/v1/query.pulsar.go (1)
1643-2556: Added query message types are correctly implemented
The new message types QueryUnbondingEntriesRequest and QueryUnbondingEntriesResponse are well-defined and integrate properly with the existing query functionality. The generated code follows Go conventions and the Uber Go Style Guide. Field names are clear, and initialisms are correctly capitalized.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
x/accounts/defaults/lockup/continuous_locking_account_test.go (1)
Line range hint
1-119: Consider making frequently used test values constants.The string "val_address" appears multiple times in the test file. Consider defining it as a constant at the package level for better maintainability.
package lockup +const ( + testValidatorAddr = "val_address" +)🧰 Tools
🪛 golangci-lint (1.62.2)
109-109: string
val_addresshas 4 occurrences, make it a constant(goconst)
x/accounts/defaults/lockup/periodic_locking_account_test.go (2)
138-149: Enhance test coverage for unbonding entriesWhile the test verifies basic functionality, consider adding the following test cases:
- Multiple unbonding entries for the same validator
- Edge cases like zero amount unbonding
- Verification of complete account state after unbonding maturity
Add these assertions after line 149:
_, err = acc.UnbondEntries.Get(sdkCtx, "val_address") require.Error(t, err) +// Verify complete account state +delFree, err := acc.DelegatedFree.Get(ctx, "test") +require.NoError(t, err) +require.True(t, delFree.Equal(math.ZeroInt())) + +// Try unbonding with zero amount +_, err = acc.Undelegate(sdkCtx, &lockuptypes.MsgUndelegate{ + Sender: "owner", + ValidatorAddress: "val_address", + Amount: sdk.NewCoin("test", math.ZeroInt()), +}) +require.Error(t, err)
Line range hint
406-406: Fix incorrect comparison in unbonding entry matchingThe condition
entry.CreationHeight == entry.CreationHeightis comparing a value with itself, which is always true. This should compare with the unbonding entry from the staking module.Apply this fix:
-if e.CompletionTime.Equal(entry.EndTime) && entry.CreationHeight == entry.CreationHeight { +if e.CompletionTime.Equal(entry.EndTime) && e.CreationHeight == entry.CreationHeight {x/accounts/defaults/lockup/lockup.go (2)
749-784: Add documentation for QuerySpendableTokens methodThe method would benefit from documentation explaining its purpose, parameters, and return values.
Add this documentation:
+// QuerySpendableTokens calculates the spendable token amount for each denom +// by subtracting locked coins that are not bonded from the account balance. +// Parameters: +// - ctx: The context for the query +// - lockedCoins: The currently locked coins for the account +// Returns: +// - QuerySpendableAmountResponse containing the spendable tokens +// - error if the calculation fails func (bva BaseLockup) QuerySpendableTokens(ctx context.Context, lockedCoins sdk.Coins) ( *lockuptypes.QuerySpendableAmountResponse, error, ) {
587-596: Improve error handling in getUbdEntriesThe method should provide more specific error handling for different failure scenarios.
Apply this improvement:
func (bva BaseLockup) getUbdEntries(ctx context.Context, delAddr, valAddr string) ([]stakingtypes.UnbondingDelegationEntry, error) { + if delAddr == "" || valAddr == "" { + return nil, sdkerrors.ErrInvalidAddress.Wrap("delegator or validator address cannot be empty") + } + resp, err := accountstd.QueryModule[*stakingtypes.QueryUnbondingDelegationResponse]( ctx, &stakingtypes.QueryUnbondingDelegationRequest{DelegatorAddr: delAddr, ValidatorAddr: valAddr}, ) if err != nil { + if strings.Contains(err.Error(), "not found") { + return nil, stakingtypes.ErrNoUnbondingDelegation + } return nil, err } + if resp == nil || resp.Unbond == nil { + return nil, sdkerrors.ErrInvalidRequest.Wrap("invalid response from query") + } return resp.Unbond.Entries, nil }api/cosmos/accounts/defaults/lockup/v1/tx.pulsar.go (1)
Line range hint
5213-5275: Breaking API Change: Removal ofMsgWithdrawandMsgWithdrawResponseMessagesThe removal of
MsgWithdrawandMsgWithdrawResponsemessages is a significant change that could break backward compatibility for clients interfacing with the API. This can impact any dependent services or applications that rely on these messages for withdrawal operations.Consider the following actions to mitigate potential issues:
- Deprecation Notice: Instead of immediate removal, consider deprecating these messages first, providing clients with ample warning and time to adapt.
- Update Documentation: Ensure all API documentation is updated to reflect the removal of these messages, and guide users on alternative methods if available.
- Versioning: If the API follows semantic versioning, increment the major version number to indicate a breaking change.
- Communication: Notify all stakeholders and users of the API about this change through appropriate channels.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/accounts/defaults/lockup/v1/query.pb.gois excluded by!**/*.pb.gox/accounts/defaults/lockup/v1/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (13)
api/cosmos/accounts/defaults/lockup/v1/tx.pulsar.go(4 hunks)tests/integration/accounts/lockup/continous_lockup_test_suite.go(3 hunks)tests/integration/accounts/lockup/delayed_lockup_test_suite.go(3 hunks)tests/integration/accounts/lockup/periodic_lockup_test_suite.go(4 hunks)x/accounts/defaults/lockup/continuous_locking_account.go(2 hunks)x/accounts/defaults/lockup/continuous_locking_account_test.go(1 hunks)x/accounts/defaults/lockup/delayed_locking_account.go(2 hunks)x/accounts/defaults/lockup/delayed_locking_account_test.go(1 hunks)x/accounts/defaults/lockup/lockup.go(10 hunks)x/accounts/defaults/lockup/periodic_locking_account.go(1 hunks)x/accounts/defaults/lockup/periodic_locking_account_test.go(1 hunks)x/accounts/proto/cosmos/accounts/defaults/lockup/v1/query.proto(3 hunks)x/accounts/proto/cosmos/accounts/defaults/lockup/v1/tx.proto(0 hunks)
💤 Files with no reviewable changes (1)
- x/accounts/proto/cosmos/accounts/defaults/lockup/v1/tx.proto
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/integration/accounts/lockup/continous_lockup_test_suite.go
- x/accounts/defaults/lockup/periodic_locking_account.go
- tests/integration/accounts/lockup/periodic_lockup_test_suite.go
- tests/integration/accounts/lockup/delayed_lockup_test_suite.go
- x/accounts/defaults/lockup/continuous_locking_account.go
- x/accounts/defaults/lockup/delayed_locking_account.go
🧰 Additional context used
📓 Path-based instructions (5)
x/accounts/defaults/lockup/continuous_locking_account_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/accounts/defaults/lockup/periodic_locking_account_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
api/cosmos/accounts/defaults/lockup/v1/tx.pulsar.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/accounts/defaults/lockup/delayed_locking_account_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/accounts/defaults/lockup/lockup.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
x/accounts/defaults/lockup/continuous_locking_account_test.go
109-109: string val_address has 4 occurrences, make it a constant
(goconst)
x/accounts/defaults/lockup/lockup.go
406-406: dupSubExpr: suspicious identical LHS and RHS for == operator
(gocritic)
🔇 Additional comments (7)
x/accounts/proto/cosmos/accounts/defaults/lockup/v1/query.proto (3)
47-50: LGTM! Proper scalar type usage for validator address.
The validator address field correctly uses the cosmos.ValidatorAddressString scalar type, ensuring proper address validation.
52-56: LGTM! Well-structured response message.
The response message properly uses repeated field for unbonding entries, allowing multiple entries to be returned.
67-74: LGTM! Proper coin type definition with gogoproto annotations.
The spendable tokens field correctly uses the cosmos base coin type with proper gogoproto annotations for nullable and casting.
x/accounts/defaults/lockup/delayed_locking_account_test.go (1)
104-108: LGTM! Proper validation of unbonding entries.
The test correctly validates:
- Existence of unbonding entries
- Entry amount matches undelegated amount
- Validator address is preserved
x/accounts/defaults/lockup/continuous_locking_account_test.go (2)
105-109: LGTM! Proper validation of unbonding entries.
The test correctly validates:
- Existence of unbonding entries
- Entry amount matches undelegated amount
- Validator address is preserved
🧰 Tools
🪛 golangci-lint (1.62.2)
109-109: string val_address has 4 occurrences, make it a constant
(goconst)
111-115: Add timeline validation for unbonding process.
The test should verify the intermediate state during the unbonding period, similar to the delayed locking account test.
x/accounts/defaults/lockup/lockup.go (1)
38-38: Fix typo in field name: "WithdrawedCoins" should be "WithdrawnCoins"
The field name uses incorrect grammar. The past participle of "withdraw" is "withdrawn".
Also applies to: 58-58, 74-75
| err = acc.CheckUbdEntriesMature(sdkCtx) | ||
| require.NoError(t, err) | ||
|
|
||
| // Update context time to unlocked all the original locking amount | ||
| sdkCtx = sdkCtx.WithHeaderInfo(header.Info{ | ||
| Time: endTime.Add(time.Second), | ||
| }) | ||
|
|
||
| _, err = acc.Delegate(sdkCtx, &lockuptypes.MsgDelegate{ | ||
| Sender: "owner", | ||
| ValidatorAddress: "val_address", | ||
| Amount: sdk.NewCoin("test", math.NewInt(6)), | ||
| }) | ||
| require.NoError(t, err) | ||
| _, err = acc.UnbondEntries.Get(sdkCtx, "val_address") | ||
| require.Error(t, err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeline validation for unbonding process.
The test should verify the intermediate state during the unbonding period.
Add assertions to check:
- State before maturity
- State after advancing context time
err = acc.CheckUbdEntriesMature(sdkCtx)
require.NoError(t, err)
+
+// Verify intermediate state
+delLocking, err = acc.DelegatedLocking.Get(ctx, "test")
+require.NoError(t, err)
+require.True(t, delLocking.Equal(math.NewInt(1))) // Should still be locked
+
+// Advance time to maturity
+sdkCtx = sdkCtx.WithHeaderInfo(header.Info{
+ Time: entries.Entries[0].CompletionTime,
+})
+
+err = acc.CheckUbdEntriesMature(sdkCtx)
+require.NoError(t, err)Committable suggestion skipped: line range outside the PR's diff.
x/accounts/defaults/lockup/lockup.go
Outdated
| removeKeys := []string{} | ||
| err = bva.UnbondEntries.Walk(ctx, nil, func(key string, value lockuptypes.UnbondingEntries) (stop bool, err error) { | ||
| for i := 0; i < len(value.Entries); i++ { | ||
| entry := value.Entries[i] | ||
| // if not mature then skip | ||
| if entry.EndTime.After(currentTime) { | ||
| return false, nil | ||
| } | ||
|
|
||
| skipIteration := false | ||
| entries, err := bva.getUbdEntries(ctx, delAddr, key) | ||
| if err != nil { | ||
| // if ubd delegation is empty then skip the next iteration check | ||
| if !errorsmod.IsOf(err, stakingtypes.ErrNoUnbondingDelegation) { | ||
| return true, err | ||
| } | ||
|
|
||
| skipIteration = true | ||
| } | ||
|
|
||
| found := false | ||
| // check if the entry is still exist in the unbonding entries | ||
| if !skipIteration { | ||
| for _, e := range entries { | ||
| if e.CompletionTime.Equal(entry.EndTime) && entry.CreationHeight == entry.CreationHeight { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // if not found or ubd delegation is empty then assume ubd entry is being handled | ||
| if !found || skipIteration { | ||
| err = bva.TrackUndelegation(ctx, sdk.NewCoins(entry.Amount)) | ||
| if err != nil { | ||
| return true, err | ||
| } | ||
|
|
||
| // remove entry | ||
| value.Entries = append(value.Entries[:i], value.Entries[i+1:]...) | ||
| i-- | ||
| } | ||
| } | ||
|
|
||
| if len(value.Entries) == 0 { | ||
| removeKeys = append(removeKeys, key) | ||
| } else { | ||
| err = bva.UnbondEntries.Set(ctx, key, value) | ||
| if err != nil { | ||
| return true, err | ||
| } | ||
| } | ||
|
|
||
| return false, nil | ||
| }) | ||
|
|
||
| for _, key := range removeKeys { | ||
| err = bva.UnbondEntries.Remove(ctx, key) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve safety of entry removal logic
The current implementation modifies the slice while iterating and uses a separate list for removal, which could be error-prone. Consider using a more robust approach.
Apply this improvement:
-removeKeys := []string{}
+type entryUpdate struct {
+ key string
+ entries *lockuptypes.UnbondingEntries
+ remove bool
+}
+updates := make([]entryUpdate, 0)
err = bva.UnbondEntries.Walk(ctx, nil, func(key string, value lockuptypes.UnbondingEntries) (stop bool, err error) {
+ newEntries := make([]*lockuptypes.UnbondingEntry, 0, len(value.Entries))
for i := 0; i < len(value.Entries); i++ {
entry := value.Entries[i]
if entry.EndTime.After(currentTime) {
+ newEntries = append(newEntries, entry)
continue
}
// ... rest of the maturity check logic ...
- value.Entries = append(value.Entries[:i], value.Entries[i+1:]...)
- i--
}
- if len(value.Entries) == 0 {
- removeKeys = append(removeKeys, key)
+ update := entryUpdate{key: key}
+ if len(newEntries) == 0 {
+ update.remove = true
} else {
- err = bva.UnbondEntries.Set(ctx, key, value)
+ value.Entries = newEntries
+ update.entries = &value
}
+ updates = append(updates, update)
return false, nil
})Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
406-406: dupSubExpr: suspicious identical LHS and RHS for == operator
(gocritic)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
x/accounts/defaults/lockup/lockup.go (1)
213-252: Consider adding validation for validator addressThe
Undelegatemethod should validate the validator address format before processing.Add validation before processing:
+ if _, err := bva.addressCodec.StringToBytes(msg.ValidatorAddress); err != nil { + return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid validator address: %s", err) + } + isNewEntry := true skipEntriesIteration := false
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
x/accounts/defaults/lockup/lockup.go(10 hunks)x/accounts/defaults/lockup/periodic_locking_account.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
x/accounts/defaults/lockup/periodic_locking_account.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/accounts/defaults/lockup/lockup.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
x/accounts/defaults/lockup/lockup.go
397-397: dupSubExpr: suspicious identical LHS and RHS for == operator
(gocritic)
🔇 Additional comments (6)
x/accounts/defaults/lockup/periodic_locking_account.go (1)
333-333: LGTM! Verify base lockup query handler integration
The addition of base lockup query handlers is a good improvement that maintains consistency across account types and supports the enhanced unbonding functionality.
Let's verify the integration of base lockup query handlers:
✅ Verification successful
Base lockup query handler integration verified
The integration of base lockup query handlers is correctly implemented and consistent across all lockup account types. The verification shows that:
- All lockup account types (Periodic, Continuous, Delayed, Permanent) properly integrate
BaseLockup.RegisterQueryHandlers - The base implementation in
lockup.goregisters the essential unbonding entries query handler - The pattern follows a consistent inheritance model where each specialized account type adds its specific handlers before delegating to the base implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify base lockup query handler integration
# Check if BaseLockup query handlers are consistently registered across all lockup account types
# Search for RegisterQueryHandlers implementations
ast-grep --pattern 'func ($x $_) RegisterQueryHandlers(builder *accountstd.QueryBuilder) {
$$$
$x.BaseLockup.RegisterQueryHandlers(builder)
$$$
}'
Length of output: 154
Script:
#!/bin/bash
# Let's try a different approach to verify the base lockup query handler integration
# First, let's find all files that might contain lockup-related query handlers
fd -e go | xargs rg -l "RegisterQueryHandlers.*accountstd.QueryBuilder"
# Then, let's examine the actual implementations
fd -e go | xargs rg -A 5 "RegisterQueryHandlers.*accountstd.QueryBuilder"
# Also check for BaseLockup usage
fd -e go | xargs rg "BaseLockup"
Length of output: 14301
x/accounts/defaults/lockup/lockup.go (5)
71-72: LGTM: Well-structured field addition for unbonding entries
The new UnbondEntries field is appropriately added to track validator-specific unbonding entries.
152-156: LGTM: Proper maturity check before delegation
Good addition of CheckUbdEntriesMature call before tracking delegation to ensure accurate delegation amounts.
640-644: LGTM: Proper maturity check in GetNotBondedLockedCoin
Good addition of CheckUbdEntriesMature call to ensure accurate locked coin calculation.
782-784: LGTM: Proper registration of query handlers
The new query handler for unbonding entries is correctly registered.
373-434: 🛠️ Refactor suggestion
Improve safety of entry removal logic
The current implementation modifies the slice while iterating and uses a separate list for removal, which could be error-prone.
Consider using a more robust approach:
-removeKeys := []string{}
+type entryUpdate struct {
+ key string
+ entries *lockuptypes.UnbondingEntries
+ remove bool
+}
+updates := make([]entryUpdate, 0)
err = bva.UnbondEntries.Walk(ctx, nil, func(key string, value lockuptypes.UnbondingEntries) (stop bool, err error) {
+ newEntries := make([]*lockuptypes.UnbondingEntry, 0, len(value.Entries))
for i := 0; i < len(value.Entries); i++ {
entry := value.Entries[i]
if entry.EndTime.After(currentTime) {
+ newEntries = append(newEntries, entry)
continue
}
// ... rest of the maturity check logic ...
- value.Entries = append(value.Entries[:i], value.Entries[i+1:]...)
- i--
}
- if len(value.Entries) == 0 {
- removeKeys = append(removeKeys, key)
+ update := entryUpdate{key: key}
+ if len(newEntries) == 0 {
+ update.remove = true
} else {
- err = bva.UnbondEntries.Set(ctx, key, value)
+ value.Entries = newEntries
+ update.entries = &value
}
+ updates = append(updates, update)
return false, nil
})Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
397-397: dupSubExpr: suspicious identical LHS and RHS for == operator
(gocritic)
x/accounts/defaults/lockup/lockup.go
Outdated
| // check if the entry is still exist in the unbonding entries | ||
| if !skipIteration { | ||
| for _, e := range entries { | ||
| if e.CompletionTime.Equal(entry.EndTime) && entry.CreationHeight == entry.CreationHeight { |
There was a problem hiding this comment.
Fix comparison in unbonding entry check
There's a logical error in the comparison where entry.CreationHeight is being compared with itself.
Apply this fix:
-if e.CompletionTime.Equal(entry.EndTime) && entry.CreationHeight == entry.CreationHeight {
+if e.CompletionTime.Equal(entry.EndTime) && e.CreationHeight == entry.CreationHeight {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if e.CompletionTime.Equal(entry.EndTime) && entry.CreationHeight == entry.CreationHeight { | |
| if e.CompletionTime.Equal(entry.EndTime) && e.CreationHeight == entry.CreationHeight { |
🧰 Tools
🪛 golangci-lint (1.62.2)
397-397: dupSubExpr: suspicious identical LHS and RHS for == operator
(gocritic)
x/accounts/defaults/lockup/lockup.go
Outdated
| entry.Amount = entry.Amount.Add(msg.Amount) | ||
|
|
||
| // update the entry | ||
| entries.Entries[i] = entry |
There was a problem hiding this comment.
Why are you handling this edge case? Equal CreationHeight could happen only with 2 undelegate messages in the same block.
There was a problem hiding this comment.
yeah maybe just edge case where user add 2 msg execute undelegate in one blocks, should we removed this then ?
x/accounts/defaults/lockup/lockup.go
Outdated
| // check if the entry is still exist in the unbonding entries | ||
| if !skipIteration { | ||
| for _, e := range entries { | ||
| if e.CompletionTime.Equal(entry.EndTime) && entry.CreationHeight == entry.CreationHeight { |
There was a problem hiding this comment.
Here you double check that the unbonding entry was removed. I assume this is not a cheap call and the logic in staking should ensure this. Do you have some concrete scenarios in mind that you want to cover?
There was a problem hiding this comment.
hmmm not sure i understand the question here. But this seems to be the only way to check if entries is actually removed from staking ( AKA is handled ) since entry can still be put onhold after matured time
| } | ||
|
|
||
| spendable, hasNeg := sdk.Coins{balance}.SafeSub(notBondedLockedCoin) | ||
| if hasNeg { |
| @@ -186,6 +180,23 @@ func (cva ContinuousLockingAccount) QueryLockupAccountInfo(ctx context.Context, | |||
| return resp, nil | |||
| } | |||
|
|
|||
| func (cva ContinuousLockingAccount) QuerySpendableTokens(ctx context.Context, req *lockuptypes.QuerySpendableAmountRequest) ( | |||
There was a problem hiding this comment.
Would it make sense to have pagination support here, too?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
x/accounts/defaults/lockup/lockup.go (1)
703-720: Consider adding pagination supportFor accounts with many unbonding entries, pagination would improve query performance and reduce response size.
Consider modifying the request and response types to include pagination parameters and results, similar to other Cosmos SDK queries.
x/accounts/defaults/lockup/permanent_locking_account_test.go (2)
80-85: Add intermediate state validation for unbonding entriesThe test should verify the state of unbonding entries before they mature. Consider adding timeline validation to ensure the unbonding process works correctly throughout its lifecycle.
entries, err := acc.UnbondEntries.Get(sdkCtx, "val_address") require.NoError(t, err) require.Len(t, entries.Entries, 1) require.True(t, entries.Entries[0].Amount.Amount.Equal(math.NewInt(1))) require.True(t, entries.Entries[0].ValidatorAddress == "val_address") + +// Verify state before maturity +delLocking, err := acc.DelegatedLocking.Get(ctx, "test") +require.NoError(t, err) +require.True(t, delLocking.Equal(math.NewInt(1))) // Should still be locked
90-91: Improve error assertion specificityThe error check should verify the specific error type or message to ensure the error is due to the entries being processed rather than other potential issues.
_, err = acc.UnbondEntries.Get(sdkCtx, "val_address") -require.Error(t, err) +require.ErrorIs(t, err, lockuptypes.ErrNoUnbondEntries)x/accounts/defaults/lockup/delayed_locking_account_test.go (1)
104-108: Extract common validation logicThe unbonding entries validation logic is duplicated across different test files. Consider extracting this into a helper function to improve maintainability.
+func assertUnbondEntries(t *testing.T, ctx context.Context, acc interface{ + UnbondEntriesGetter interface { + Get(context.Context, string) (*lockuptypes.UnbondingEntries, error) + } +}, validatorAddr string, expectedAmount math.Int) { + t.Helper() + entries, err := acc.UnbondEntries.Get(ctx, validatorAddr) + require.NoError(t, err) + require.Len(t, entries.Entries, 1) + require.True(t, entries.Entries[0].Amount.Amount.Equal(expectedAmount)) + require.True(t, entries.Entries[0].ValidatorAddress == validatorAddr) +}x/accounts/defaults/lockup/continuous_locking_account_test.go (1)
105-109: Make validator address a constantThe string "val_address" is used multiple times across test files. Consider making it a constant in a test helper file.
+const ( + testValidatorAddr = "val_address" +)🧰 Tools
🪛 golangci-lint (1.62.2)
109-109: string
val_addresshas 4 occurrences, make it a constant(goconst)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (5)
x/accounts/defaults/lockup/continuous_locking_account_test.go(1 hunks)x/accounts/defaults/lockup/delayed_locking_account_test.go(1 hunks)x/accounts/defaults/lockup/lockup.go(10 hunks)x/accounts/defaults/lockup/periodic_locking_account_test.go(1 hunks)x/accounts/defaults/lockup/permanent_locking_account_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/accounts/defaults/lockup/periodic_locking_account_test.go
🧰 Additional context used
📓 Path-based instructions (4)
x/accounts/defaults/lockup/permanent_locking_account_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/accounts/defaults/lockup/continuous_locking_account_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/accounts/defaults/lockup/delayed_locking_account_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/accounts/defaults/lockup/lockup.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
x/accounts/defaults/lockup/continuous_locking_account_test.go
109-109: string val_address has 4 occurrences, make it a constant
(goconst)
x/accounts/defaults/lockup/lockup.go
390-390: dupSubExpr: suspicious identical LHS and RHS for == operator
(gocritic)
384-384: File is not gofumpt-ed with -extra
(gofumpt)
🔇 Additional comments (10)
x/accounts/defaults/lockup/lockup.go (8)
37-37: LGTM: New prefix and field declarations are well-structured
The additions follow the existing pattern and are properly documented.
Also applies to: 71-72
152-156: LGTM: Proper sequencing of unbonding check before delegation
The addition of checkUnbondingEntriesMature ensures accurate delegation tracking by processing any matured unbonding entries first.
560-569: LGTM: Well-implemented helper method
The method properly handles the query and error cases.
622-626: LGTM: Proper synchronization of unbonding state
The addition of checkUnbondingEntriesMature ensures accurate locked coin calculation.
722-757: LGTM: Well-implemented spendable tokens calculation
The method properly handles balance checks and error cases.
764-766: LGTM: Proper registration of new query handler
The addition follows the established pattern for query handler registration.
390-391:
Fix incorrect self-comparison in unbonding entry check
The creation height is being compared with itself, which is likely a bug.
Apply this diff:
-if e.CompletionTime.Equal(entry.EndTime) && entry.CreationHeight == entry.CreationHeight {
+if e.CompletionTime.Equal(entry.EndTime) && e.CreationHeight == entry.CreationHeight {Likely invalid or redundant comment.
🧰 Tools
🪛 golangci-lint (1.62.2)
390-390: dupSubExpr: suspicious identical LHS and RHS for == operator
(gocritic)
208-211:
Add bounds check for response array
Before accessing resp[0], add a check to ensure the response array is not empty to prevent potential panic.
Apply this diff:
resp, err := sendMessage(ctx, msgUndelegate)
if err != nil {
return nil, err
}
+if len(resp) == 0 {
+ return nil, sdkerrors.ErrInvalidResponse.Wrap("empty response from MsgUndelegate")
+}
msgUndelegateResp, err := accountstd.UnpackAny[stakingtypes.MsgUndelegateResponse](resp[0])Likely invalid or redundant comment.
x/accounts/defaults/lockup/delayed_locking_account_test.go (1)
113-114: Improve error assertion specificity
Similar to the permanent account test, the error check should verify the specific error type.
x/accounts/defaults/lockup/continuous_locking_account_test.go (1)
114-115: Improve error assertion specificity
Similar to other test files, the error check should verify the specific error type.
| found := false | ||
| // check if the entry is still exist in the unbonding entries | ||
| for _, e := range stakingUnbonding { | ||
| if e.CompletionTime.Equal(entry.EndTime) && entry.CreationHeight == entry.CreationHeight { |
Check warning
Code scanning / CodeQL
Comparison of identical values Warning
Description
TrackUndelegationto update thedelegateLockinganddelegateFreeamount, this is incorrect behavior since legacy vesting account callTrackUndelegationwhen we call x/bankUndelegateCoinswhich is when the unbonding entry already matured. This makedelegateLockingupdated before the ubd amount actually sent back to the account, which make the spendable amount incorrect when we try to sent token from lockup accountTrackUndelegationwith that entry. This check should be run before every execution the account made.TrackUndelegationAuthor 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...
Summary by CodeRabbit
New Features
Bug Fixes
Documentation