-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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): lockup account document, Tx and InitGenesis #23397
base: main
Are you sure you want to change the base?
Conversation
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=error msg="Running error: context loading failed: no go files to analyze: running 📝 WalkthroughWalkthroughThis pull request addresses multiple aspects of the accounts module in the Cosmos SDK, focusing on genesis initialization, CLI functionality, and documentation updates. The changes include modifying the initialization order of modules in the simapp configuration, updating the CLI to support funded account creation, revising documentation for lockup accounts, and registering new message types. The modifications aim to improve the accounts module's functionality and clarity in version 0.52. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Finishing Touches
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: 1
🧹 Nitpick comments (2)
x/accounts/cli/cli.go (1)
73-76
: Enhance error handling for funds parsingConsider providing a more descriptive error message when funds parsing fails, to help users understand the expected format.
funds, err := sdk.ParseCoinsNormalized(args[2]) if err != nil { - return err + return fmt.Errorf("failed to parse funds %q: %w. Expected format: <amount><denomination>[,<amount><denomination>...]", args[2], err) }Also applies to: 81-81
x/accounts/defaults/lockup/TUTORIAL.md (1)
216-216
: Fix grammatical errors in documentationThere are grammatical errors in the query documentation.
-`QueryLockupAccountInfoRequest` does not required an input +`QueryLockupAccountInfoRequest` does not require an input -`QueryLockingPeriodsRequest` does not required an input +`QueryLockingPeriodsRequest` does not require an inputAlso applies to: 238-238
🧰 Tools
🪛 LanguageTool
[grammar] ~216-~216: After the auxiliary verb ‘do’, use the base form of a verb. Did you mean “require”?
Context: ...QueryLockupAccountInfoRequest` does not required an input. Account information includin...(AUXILIARY_DO_WITH_INCORRECT_VERB_FORM)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
x/accounts/go.sum
is excluded by!**/*.sum
📒 Files selected for processing (6)
simapp/v2/app_config.go
(1 hunks)x/accounts/README.md
(1 hunks)x/accounts/cli/cli.go
(3 hunks)x/accounts/defaults/lockup/TUTORIAL.md
(7 hunks)x/accounts/go.mod
(3 hunks)x/accounts/module.go
(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
simapp/v2/app_config.go (1)
Pattern **/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/accounts/module.go (1)
Pattern **/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/accounts/README.md (1)
Pattern **/*.md
: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
x/accounts/defaults/lockup/TUTORIAL.md (1)
Pattern **/*.md
: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
x/accounts/cli/cli.go (1)
Pattern **/*.go
: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 LanguageTool
x/accounts/defaults/lockup/TUTORIAL.md
[grammar] ~216-~216: After the auxiliary verb ‘do’, use the base form of a verb. Did you mean “require”?
Context: ...QueryLockupAccountInfoRequest` does not required an input. Account information includin...
(AUXILIARY_DO_WITH_INCORRECT_VERB_FORM)
[grammar] ~238-~238: After the auxiliary verb ‘do’, use the base form of a verb. Did you mean “require”?
Context: ...e QueryLockingPeriodsRequest
does not required an input. Locking periods including: ...
(AUXILIARY_DO_WITH_INCORRECT_VERB_FORM)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (7)
x/accounts/module.go (2)
16-19
: LGTM: Clean import additionsThe new imports for lockup types and SDK types are properly organized.
56-62
: LGTM: Proper message type registrationThe lockup message types are correctly registered with the interface registrar, following the standard pattern for Cosmos SDK message registration.
x/accounts/cli/cli.go (1)
45-45
: LGTM: Clear command usage updateThe command usage and argument count are properly updated to include the funds parameter.
Also applies to: 47-47
simapp/v2/app_config.go (1)
159-159
: Verify module initialization orderThe accounts module is now included in InitGenesis. Please verify that this position in the initialization order is correct, particularly in relation to dependencies like auth and bank modules.
✅ Verification successful
Module initialization order is correct
The position of the accounts module in InitGenesis is valid. Recent framework changes have loosened the requirements for module initialization order, only requiring specific ordering for modules that implement certain interfaces. The accounts module's position relative to auth and bank modules is consistent with the codebase's test implementations and module relationships.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any explicit dependencies in the accounts module initialization rg -A 5 "InitGenesis.*accounts" rg -B 5 "accounts.*InitGenesis"Length of output: 1810
Script:
#!/bin/bash # Check for module dependencies and initialization patterns echo "=== Checking accounts module dependencies ===" rg -l "accounts" --type go | xargs rg "Depends|Required" -B 2 -A 2 echo -e "\n=== Checking module initialization patterns ===" fd -e go -e md . | xargs rg "ModuleOrder|InitOrder|OrderBeginBlock|OrderEndBlock" -A 3 echo -e "\n=== Looking for dependency documentation ===" fd "README.md|DEPENDENCIES.md" | xargs rg "module.*depend|initialization.*order" -iLength of output: 17656
Script:
#!/bin/bash # Check accounts module implementation and dependencies echo "=== Checking accounts module interfaces ===" ast-grep --pattern 'type $_ struct { $$$ } func (m $_) BeginBlock($_) $_ { $$$ }' echo -e "\n=== Checking accounts module relationship with auth/bank ===" rg -B 3 -A 3 "accounts.*\.(auth|bank)|auth.*\.accounts|bank.*\.accounts" --type go echo -e "\n=== Looking for genesis dependencies ===" rg "InitGenesis.*accounts" -B 5 -A 5 --type goLength of output: 33165
x/accounts/defaults/lockup/TUTORIAL.md (1)
107-107
: LGTM: Consistent version updates in message type URLsThe message type URLs are correctly updated to include the v1 version, maintaining consistency across all message types.
Also applies to: 128-128, 149-149, 166-166, 186-186
x/accounts/go.mod (1)
11-11
: Verify dependency versions and compatibility.Several dependencies have been updated to specific pseudo-versions. Let's verify their compatibility and check for any security advisories.
Also applies to: 43-45
✅ Verification successful
Dependencies verified - local module development pattern confirmed
The pseudo-versions are consistent across modules and the replace directives correctly point to local development paths, which is the expected pattern for monorepo module development. No security vulnerabilities were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for security advisories and latest versions of key dependencies # Check for security advisories for bank and staking modules gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: GO, package: "cosmossdk.io/x/bank") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: GO, package: "cosmossdk.io/x/staking") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 705
Script:
#!/bin/bash # Check the full go.mod content and related documentation echo "=== Checking go.mod content ===" cat x/accounts/go.mod echo -e "\n=== Checking for README or docs with version info ===" fd -t f "README.md|CHANGELOG.md|VERSION" x/accounts/ echo -e "\n=== Checking other cosmos-sdk related dependencies ===" rg "cosmossdk.io" x/accounts/go.modLength of output: 9560
x/accounts/README.md (1)
548-553
: Documentation updates look good!The changes to the account initialization documentation are clear and accurate:
- Field name change from
account_type
tolockup_account_type
improves clarity- Message type update to include version (
v1
) follows best practices- Time values are now specific and well-formatted
@@ -1,14 +1,14 @@ | |||
module cosmossdk.io/x/accounts | |||
|
|||
go 1.23.3 | |||
go 1.23.4 |
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.
Invalid Go version specified.
The Go version 1.23.4
is incorrect as it does not exist. The latest stable version of Go is 1.22.x.
-go 1.23.4
+go 1.22.1
📝 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.
go 1.23.4 | |
go 1.22.1 |
cosmossdk.io/store v1.10.0-rc.1 // indirect | ||
cosmossdk.io/x/staking v0.0.0-00010101000000-000000000000 // indirect | ||
cosmossdk.io/store v1.10.0-rc.1.0.20241218084712-ca559989da43 // indirect | ||
cosmossdk.io/x/accounts/defaults/lockup v0.2.0-rc.1 |
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.
any chance we can avoid this import?
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.
Msgs for lockup account were added in v0.2.0.
I think this code is needed to import the type from cosmossdk.io.
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.
If you can register the type in another way, you may replace it.
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.
We can register them in the app.go directly maybe. Could be something we add to the x/accounts documentation.
Description
Closes: #23312
init_account_msgs
bug in the genesis.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
Documentation
Improvements
Bug Fixes