-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(x/accounts/default/lockup) Lockup account track undelegation when unbonding entry is mature #22254
base: main
Are you sure you want to change the base?
Conversation
x.ValidatorAddress = value.Interface().(string) | ||
default: | ||
if fd.IsExtension() { | ||
panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.accounts.defaults.lockup.UnbondingEntry")) |
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.UnbondingEntry")) | ||
} | ||
panic(fmt.Errorf("message cosmos.accounts.defaults.lockup.UnbondingEntry does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
maybe user should manually track this delegation through an execution since matured ubd doesnot mean that the fund has returned to the account yet |
switch fd.FullName() { | ||
default: | ||
if fd.IsExtension() { | ||
panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.accounts.defaults.lockup.QueryUnbondingEntriesRequest")) |
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.QueryUnbondingEntriesRequest")) | ||
} | ||
panic(fmt.Errorf("message cosmos.accounts.defaults.lockup.QueryUnbondingEntriesRequest does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
x.UnbondingEntries = *clv.list | ||
default: | ||
if fd.IsExtension() { | ||
panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.accounts.defaults.lockup.QueryUnbondingEntriesResponse")) |
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.QueryUnbondingEntriesResponse")) | ||
} | ||
panic(fmt.Errorf("message cosmos.accounts.defaults.lockup.QueryUnbondingEntriesResponse does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
x.Id = value.Uint() | ||
default: | ||
if fd.IsExtension() { | ||
panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.accounts.defaults.lockup.MsgTrackUndelegation")) |
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.MsgTrackUndelegation")) | ||
} | ||
panic(fmt.Errorf("message cosmos.accounts.defaults.lockup.MsgTrackUndelegation does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
@sontrinh16 your pull request is missing a changelog! |
📝 WalkthroughWalkthroughThe changes in this pull request introduce new message types and functionalities to the lockup package in the Cosmos SDK. A new message type Changes
Possibly related PRs
Suggested labels
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
UnbondingEntry
message 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
UnbondingEntry
The
UnbondingEntry
message 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 handlingUnbondingEntry
instances is included. Specifically, consider implementing:
- Functions to create and update
UnbondingEntry
instances.- Handlers to process matured entries and execute account operations based on them.
- Integration points where
UnbondingEntry
is utilized within the account management workflow.🔗 Analysis chain
Line range hint
1-37
: Implementation details needed for complete solution.The addition of the
UnbondingEntry
message 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
UnbondingEntry
instances.- 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
UnbondingEntry
instances 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
queryUnbondingEntries
method 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
setupStakingParams
method 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
QueryUnbondingEntriesRequest
andQueryUnbondingEntriesResponse
messages 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
TrackUndelegationEntry
handler 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
TrackUndelegationEntry
next toUndelegate
since 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)), }, nil
x/accounts/proto/cosmos/accounts/defaults/lockup/tx.proto (1)
110-119
: LGTM! Consider adding a comment for theid
field.The new
MsgTrackUndelegation
message is well-structured and consistent with other messages in the file. It appropriately uses thecosmos.AddressString
scalar type for thesender
field and includes necessary options.For improved clarity and consistency with other messages in the file, consider adding a comment for the
id
field 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-1
to 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.Equal
for 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
: AssertDelegatedFree
amount after undelegation trackingAfter tracking the undelegation, you assert that
DelegatedLocking
is zero. It's important to also check theDelegatedFree
amount 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.go
is misspelled. It should becontinuous_lockup_test_suite.go
to 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 verifyDelegatedFree
amount after tracking undelegation.Currently, the test checks that
DelegatedLocking
amount is zero after tracking undelegation. To fully validate the state, consider adding an assertion to verify that theDelegatedFree
amount 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
, andValidatorAddress
to 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
QueryLockupAccountInfoResponse
and 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
.proto
files 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.go
is excluded by!**/*.pb.go
x/accounts/defaults/lockup/types/query.pb.go
is excluded by!**/*.pb.go
x/accounts/defaults/lockup/types/tx.pb.go
is 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.proto
andgoogle/protobuf/timestamp.proto
are correctly added to support the newUnbondingEntry
message 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.go
improve the testing framework for the lockup module. The new methodsqueryUnbondingEntries
andsetupStakingParams
allow 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
QueryUnbondingEntriesRequest
message 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
QueryUnbondingEntriesResponse
message is well-structured for returning a list of unbonding entries. However, please ensure that theUnbondingEntry
type is properly defined and imported.Run the following script to verify the UnbondingEntry type definition:
✅ Verification successful
Let's attempt to locate the
UnbondingEntry
type definition by searching within all.proto
files.
Verified: UnbondingEntry type is properly defined
The
UnbondingEntry
type is defined inx/accounts/proto/cosmos/accounts/defaults/lockup/lockup.proto
, ensuring it is properly defined and accessible for theQueryUnbondingEntriesResponse
message.🏁 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
QueryUnbondingEntries
handler is consistent with the PR objectives and follows the existing naming convention.Please ensure that the
QueryUnbondingEntries
method is properly implemented in thePermanentLockingAccount
struct. 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
newMockContext
function 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
TestDelayedAccountUndelegate
function 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
QueryUnbondingEntries
query handler registration is appropriate and aligns with the PR objectives. This enhancement will allow theContinuousLockingAccount
to handle queries related to unbonding entries.To ensure completeness, let's verify the implementation of the
QueryUnbondingEntries
method:✅ Verification successful
Implementation of
QueryUnbondingEntries
Verified SuccessfullyThe
QueryUnbondingEntries
method is correctly implemented inx/accounts/defaults/lockup/lockup.go
. The registration incontinuous_locking_account.go
properly 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
QueryUnbondingEntries
query 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
QueryUnbondingEntries
method? 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
delLocking
equals 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
setupStakingParams
is 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_QueryUnbondingEntriesRequest
are 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_QueryUnbondingEntriesResponse
methods, 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
MsgTrackUndelegation
and 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.NoError
consistently 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 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
QueryUnbondingEntries
method is implemented within theDelayedLockingAccount
struct. - 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
msgUndelegateResp, err := accountstd.UnpackAny[stakingtypes.MsgUndelegateResponse](resp[0]) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check response length before accessing resp[0]
Before accessing the first element of resp
, ensure that the response slice is not empty to prevent a potential index out-of-range panic.
Apply this diff to add a length check:
resp, err := sendMessage(ctx, msgUndelegate)
if err != nil {
return nil, err
}
+if len(resp) == 0 {
+ return nil, sdkerrors.ErrInvalidResponse.Wrap("no response received from MsgUndelegate")
+}
msgUndelegateResp, err := accountstd.UnpackAny[stakingtypes.MsgUndelegateResponse](resp[0])
if err != nil {
return nil, err
}
This ensures that you safely handle cases where the response might be empty.
📝 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.
msgUndelegateResp, err := accountstd.UnpackAny[stakingtypes.MsgUndelegateResponse](resp[0]) | |
if err != nil { | |
if len(resp) == 0 { | |
return nil, sdkerrors.ErrInvalidResponse.Wrap("no response received from MsgUndelegate") | |
} | |
msgUndelegateResp, err := accountstd.UnpackAny[stakingtypes.MsgUndelegateResponse](resp[0]) | |
if err != nil { |
entry, err := bva.UnbondEntries.Get(ctx, msg.Id) | ||
if err != nil { | ||
return nil, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 | |
} |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
}
Description
TrackUndelegation
to update thedelegateLocking
anddelegateFree
amount, this is incorrect behavior since legacy vesting account callTrackUndelegation
when we call x/bankUndelegateCoins
which is when the unbonding entry already matured. This makedelegateLocking
updated before the ubd amount actually sent back to the account, which make the spendable amount incorrect when we try to sent token from lockup accountTrackUndelegation
with that entry. This check should be run before every execution the account made.TrackUndelegation
Author 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.md
Reviewers 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
UnbondingEntry
for tracking unbonding operations.QueryUnbondingEntriesRequest
andQueryUnbondingEntriesResponse
.MsgTrackUndelegation
for tracking undelegation operations.Bug Fixes
Tests