feat(accounts/base): give chains the possibility to pick their chosen pubkey types for the base accounts #21466
Conversation
|
Warning Rate limit exceeded@testinginprod has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 18 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughWalkthroughThe changes involve significant modifications to the handling of public keys within the Cosmos SDK account management system. Key message types, Changes
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
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
x/accounts/depinject.go (1)
67-67: Update function call to match new signatureThe function call to
baseaccount.NewAccountintests/integration/auth/keeper/msg_server_test.godoes not include thebaseaccount.WithSecp256K1PubKey()parameter. Please update this instance to ensure consistency with the new function signature.
- File:
tests/integration/auth/keeper/msg_server_test.goAnalysis chain
LGTM! But verify the function usage in the codebase.
The change enhances the account creation process by configuring the account to utilize a specific public key type (Secp256K1). Ensure that all function calls to
baseaccount.NewAccountmatch the new signature.The code changes are approved.
Run the following script to verify the function usage:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `baseaccount.NewAccount` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'baseaccount.NewAccount'Length of output: 1609
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (2)
store/internal/kv/kv.pb.gois excluded by!**/*.pb.gox/accounts/defaults/base/v1/base.pb.gois excluded by!**/*.pb.go
Files selected for processing (8)
- api/cosmos/accounts/defaults/base/v1/base.pulsar.go (25 hunks)
- scripts/protocgen.sh (1 hunks)
- simapp/app.go (1 hunks)
- x/accounts/defaults/base/account.go (6 hunks)
- x/accounts/defaults/base/account_test.go (10 hunks)
- x/accounts/defaults/base/pubkey.go (1 hunks)
- x/accounts/depinject.go (1 hunks)
- x/accounts/proto/cosmos/accounts/defaults/base/v1/base.proto (3 hunks)
Additional context used
Path-based instructions (6)
x/accounts/defaults/base/pubkey.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/accounts/depinject.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/accounts/defaults/base/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/base/account.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.simapp/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.api/cosmos/accounts/defaults/base/v1/base.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Shellcheck
scripts/protocgen.sh
[warning] 32-32: For loops over find output are fragile. Use find -exec or a while read loop.
(SC2044)
Additional comments not posted (48)
x/accounts/proto/cosmos/accounts/defaults/base/v1/base.proto (4)
7-7: LGTM!The import statement for
google/protobuf/any.protois necessary for the changes made to thepub_keyfields.
11-12: LGTM!Changing the
pub_keyfield type frombytestogoogle.protobuf.Anyallows for a more flexible representation of public keys.
22-22: LGTM!Changing the
new_pub_keyfield type frombytestogoogle.protobuf.Anyallows for a more flexible representation of public keys.
38-44: LGTM!The addition of the
QueryPubKeyandQueryPubKeyResponsemessages facilitates querying public keys in a more versatile manner.x/accounts/defaults/base/pubkey.go (4)
31-36: LGTM!The
WithSecp256K1PubKeyfunction correctly implements the validation for secp256k1 public keys.
38-42: LGTM!The
WithPubKeyfunction correctly implements the option for generic public keys without validation.
44-65: LGTM!The
WithPubKeyWithValidationFuncfunction correctly implements the option for generic public keys with a custom validation function.
67-72: LGTM!The
nameFromTypeURLfunction correctly extracts the name from a type URL.x/accounts/defaults/base/account_test.go (5)
30-33: LGTM!The change enhances the robustness of public key handling by ensuring that the provided public key is correctly parsed and validated.
The code changes are approved.
45-45: LGTM!The change ensures that public keys are correctly serialized, which is crucial for maintaining the integrity of account management in the system.
The code changes are approved.
Also applies to: 57-57, 64-64
86-86: LGTM!The change ensures that public keys are correctly serialized, which is crucial for maintaining the integrity of account management in the system.
The code changes are approved.
Also applies to: 103-103, 114-114, 125-125
158-158: LGTM!The change ensures that public keys are correctly serialized, which is crucial for maintaining the integrity of account management in the system.
The code changes are approved.
261-269: LGTM!The function is correctly implemented and enhances the robustness of public key handling in the test cases.
The code changes are approved.
x/accounts/defaults/base/account.go (8)
32-33: LGTM!The addition of the
Optiontype and its usage in theNewAccountfunction enhances the flexibility of account creation by allowing additional configurations.The code changes are approved.
Also applies to: 34-34, 45-47
36-44: LGTM!The changes enhance the flexibility of public key management and improve the robustness of the account management system.
The code changes are approved.
Also applies to: 57-58, 65-66
71-71: LGTM!The change ensures that public keys are correctly saved and validated during account initialization.
The code changes are approved.
79-79: LGTM!The change ensures that public keys are correctly saved and validated during key swapping.
The code changes are approved.
90-90: LGTM!The change ensures that public keys are correctly handled during authentication.
The code changes are approved.
Also applies to: 95-95, 112-112
131-134: LGTM!The change reflects the broader public key handling and improves error handling by providing clearer context for failures.
The code changes are approved.
Also applies to: 140-140, 143-145, 148-150, 155-155
208-230: LGTM!The method is correctly implemented and enhances the robustness of public key handling in the account management system.
The code changes are approved.
232-254: LGTM!The method is correctly implemented and enhances the robustness of public key handling in the account management system.
The code changes are approved.
simapp/app.go (1)
308-308: Ensure the new public key type is supported.The addition of
baseaccount.WithSecp256K1PubKey()as a parameter tobaseaccount.NewAccountis correct. However, ensure that the new public key type is supported throughout the codebase and that any necessary changes are made to accommodate it.Run the following script to verify the usage of
baseaccount.WithSecp256K1PubKeyin the codebase:api/cosmos/accounts/defaults/base/v1/base.pulsar.go (26)
10-10: LGTM!The import statement for
anypbis correct and necessary for the changes made in the file.
92-93: LGTM!The
Rangemethod correctly handles the new*anypb.Anytype forPubKey.
114-114: LGTM!The
Hasmethod correctly handles the new*anypb.Anytype forPubKey.
151-151: LGTM!The
Getmethod correctly handles the new*anypb.Anytype forPubKey.
173-173: LGTM!The
Setmethod correctly handles the new*anypb.Anytype forPubKey.
195-198: LGTM!The
Mutablemethod correctly handles the new*anypb.Anytype forPubKey.
213-214: LGTM!The
NewFieldmethod correctly handles the new*anypb.Anytype forPubKey.
284-285: LGTM!The
Sizemethod correctly handles the new*anypb.Anytype forPubKey.
317-327: LGTM!The
Marshalmethod correctly handles the new*anypb.Anytype forPubKey.
Line range hint
384-413: LGTM!The
Unmarshalmethod correctly handles the new*anypb.Anytype forPubKey.
883-884: LGTM!The
Rangemethod correctly handles the new*anypb.Anytype forNewPubKey.
905-905: LGTM!The
Hasmethod correctly handles the new*anypb.Anytype forNewPubKey.
942-942: LGTM!The
Getmethod correctly handles the new*anypb.Anytype forNewPubKey.
964-964: LGTM!The
Setmethod correctly handles the new*anypb.Anytype forNewPubKey.
986-989: LGTM!The
Mutablemethod correctly handles the new*anypb.Anytype forNewPubKey.
1004-1005: LGTM!The
NewFieldmethod correctly handles the new*anypb.Anytype forNewPubKey.
1075-1076: LGTM!The
Sizemethod correctly handles the new*anypb.Anytype forNewPubKey.
1108-1118: LGTM!The
Marshalmethod correctly handles the new*anypb.Anytype forNewPubKey.
Line range hint
1175-1204: LGTM!The
Unmarshalmethod correctly handles the new*anypb.Anytype forNewPubKey.
2790-2795: LGTM!The
Rangemethod correctly handles the new*anypb.Anytype forPubKey.
2810-2812: LGTM!The
Hasmethod correctly handles the new*anypb.Anytype forPubKey.
2846-2849: LGTM!The
Getmethod correctly handles the new*anypb.Anytype forPubKey.
2869-2871: LGTM!The
Setmethod correctly handles the new*anypb.Anytype forPubKey.
2893-2896: LGTM!The
Mutablemethod correctly handles the new*anypb.Anytype forPubKey.
2909-2912: LGTM!The
NewFieldmethod correctly handles the new*anypb.Anytype forPubKey.
2982-2983: LGTM!The
Sizemethod correctly handles the new*anypb.Anytype forPubKey.
| @@ -29,7 +29,7 @@ for dir in $proto_dirs; do | |||
|
|
|||
| # check if buf.gen.gogo.yaml exists in the proto directory | |||
| if [ -f "buf.gen.gogo.yaml" ]; then | |||
| for file in $(find . -maxdepth 5 -name '*.proto'); do | |||
| for file in $(find . -maxdepth 8 -name '*.proto'); do | |||
There was a problem hiding this comment.
Use find -exec to address the Shellcheck warning.
For loops over find output are fragile. Use find -exec to address the Shellcheck warning.
Apply this diff to fix the issue:
- for file in $(find . -maxdepth 8 -name '*.proto'); do
+ find . -maxdepth 8 -name '*.proto' -exec sh -c '
+ for file; do
+ if grep -q "option go_package" "$file" && grep -H -o -c "option go_package.*cosmossdk.io/api" "$file" | grep -q ":0$"; then
+ buf generate --template buf.gen.gogo.yaml "$file"
+ fi
+ done
+ ' sh {} +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.
| for file in $(find . -maxdepth 8 -name '*.proto'); do | |
| find . -maxdepth 8 -name '*.proto' -exec sh -c ' | |
| for file; do | |
| if grep -q "option go_package" "$file" && grep -H -o -c "option go_package.*cosmossdk.io/api" "$file" | grep -q ":0$"; then | |
| buf generate --template buf.gen.gogo.yaml "$file" | |
| fi | |
| done | |
| ' sh {} + |
Tools
Shellcheck
[warning] 32-32: For loops over find output are fragile. Use find -exec or a while read loop.
(SC2044)
x/accounts/defaults/base/pubkey.go
Outdated
|
|
||
| // PubKey defines a generic pubkey. | ||
| type PubKey interface { | ||
| gogoproto.Message |
There was a problem hiding this comment.
should we make this core tx.Msg to be consistent?
There was a problem hiding this comment.
agreed, let me fix
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- simapp/CHANGELOG.md (1 hunks)
- tests/integration/auth/keeper/msg_server_test.go (1 hunks)
Additional context used
Path-based instructions (2)
simapp/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"tests/integration/auth/keeper/msg_server_test.go (3)
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"
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (2)
simapp/CHANGELOG.md (1)
48-48: LGTM!The changelog entry is well-written and follows the guidelines.
tests/integration/auth/keeper/msg_server_test.go (1)
81-81: LGTM!The modification enhances the account creation process by specifying the use of a Secp256K1 public key. This change is likely to impact cryptographic operations and the overall behavior of the account in subsequent tests.
testinginprod
added
the
backport/v0.52.x
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- x/accounts/defaults/base/pubkey.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/accounts/defaults/base/pubkey.go
| // pub_key defines the secp256k1 pubkey for the account. | ||
| bytes pub_key = 1; | ||
| // pub_key defines a pubkey for the account arbitrary encapsulated. | ||
| google.protobuf.Any pub_key = 1; |
There was a problem hiding this comment.
why do we need an Any here? What would be the improvement over just bytes?
There was a problem hiding this comment.
Probably so that you know from the type url what is the pub key type
Description
It is now possible to plug in different pubkeys in base account.
This allows chains to inject custom pubkeys into the base account type, making the transition to x/accounts for accounts of any kind much more simpler!
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.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
google.protobuf.Any.QueryPubKeyandQueryPubKeyResponsefor querying public keys.Bug Fixes
Documentation
Tests