feat(auth): allow BaseAccounts to be migrated to x/accounts #21820
Conversation
WalkthroughWalkthroughThe pull request introduces a new account migration feature that enables users to migrate accounts from the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Recent review detailsConfiguration used: .coderabbit.yml Files ignored due to path filters (1)
Files selected for processing (2)
Files skipped from review as they are similar to previous changes (1)
Additional context usedPath-based instructions (1)
Additional comments not posted (3)
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}
| x.AccountInitMsg = value.Message().Interface().(*anypb.Any) | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.auth.v1beta1.MsgMigrateAccount")) |
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.auth.v1beta1.MsgMigrateAccount")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.auth.v1beta1.MsgMigrateAccount does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| x.InitResponse = value.Message().Interface().(*anypb.Any) | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.auth.v1beta1.MsgMigrateAccountResponse")) |
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.auth.v1beta1.MsgMigrateAccountResponse")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.auth.v1beta1.MsgMigrateAccountResponse does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| @@ -4,7 +4,14 @@ | |||
| "context" | |||
| "errors" | |||
| "fmt" | |||
| "reflect" | |||
Check notice
Code scanning / CodeQL
Sensitive package import Note
testinginprod
requested review from
facundomedica,
aaronc,
a team and
sontrinh16
as code owners
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: 11
Outside diff range and nitpick comments (2)
x/auth/types/expected_keepers.go (1)
35-41: LGTM! Consider adding documentation comments.The
MigrateLegacyAccountmethod signature looks good and follows the Golang naming conventions. The parameters are well-defined and serve the purpose of migrating legacy accounts to a new account type.To improve the code readability and maintainability, consider adding documentation comments to explain the purpose of the method and its parameters. This will help future developers understand the functionality of the method at a glance.
UPGRADING.md (1)
979-984: Use consistent unordered list style.For better readability, please use asterisks (
*) consistently for the unordered list items in the example message structure, instead of dashes (-).Tools
LanguageTool
[uncategorized] ~979-~979: Loose punctuation mark.
Context: ...###### Field Descriptions -signer`: The address of the account you want to ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~980-~980: Loose punctuation mark.
Context: ...u want to migrate from. -account_type: The new account type you want to migrat...(UNLIKELY_OPENING_PUNCTUATION)
[style] ~980-~980: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...account_type`: The new account type you want to migrate to (depends on what's installed...(REP_WANT_TO_VB)
[uncategorized] ~981-~981: Loose punctuation mark.
Context: ...lled on the chain). -account_init_msg: The custom initialization message for t...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~982-~982: Loose punctuation mark.
Context: ...ssage for the new account. -@type: Specifies the type of account (in this ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~983-~983: Loose punctuation mark.
Context: .../accounts base account). -pub_key: The public key for the account. You can...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~984-~984: Loose punctuation mark.
Context: ...ic key if desired. -init_sequence: The new sequence number for the account...(UNLIKELY_OPENING_PUNCTUATION)
Markdownlint
979-979: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
980-980: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
981-981: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
982-982: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
983-983: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
984-984: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (4)
api/cosmos/auth/v1beta1/tx_grpc.pb.gois excluded by!**/*.pb.gox/accounts/defaults/base/v1/base.pb.gois excluded by!**/*.pb.gox/auth/types/accounts.pb.gois excluded by!**/*.pb.gox/auth/types/tx.pb.gois excluded by!**/*.pb.go
Files selected for processing (16)
- UPGRADING.md (1 hunks)
- api/cosmos/accounts/defaults/base/v1/base.pulsar.go (14 hunks)
- api/cosmos/auth/v1beta1/accounts.pulsar.go (16 hunks)
- api/cosmos/auth/v1beta1/tx.pulsar.go (6 hunks)
- tests/integration/auth/keeper/accounts_retro_compatibility_test.go (2 hunks)
- tests/integration/auth/keeper/migrate_x_accounts_test.go (1 hunks)
- x/accounts/defaults/base/account.go (4 hunks)
- x/accounts/proto/cosmos/accounts/defaults/base/v1/base.proto (1 hunks)
- x/auth/CHANGELOG.md (1 hunks)
- x/auth/keeper/grpc_query.go (1 hunks)
- x/auth/keeper/msg_server.go (2 hunks)
- x/auth/proto/cosmos/auth/v1beta1/accounts.proto (1 hunks)
- x/auth/proto/cosmos/auth/v1beta1/tx.proto (2 hunks)
- x/auth/testutil/expected_keepers_mocks.go (1 hunks)
- x/auth/types/codec.go (1 hunks)
- x/auth/types/expected_keepers.go (1 hunks)
Files skipped from review due to trivial changes (1)
- x/auth/proto/cosmos/auth/v1beta1/accounts.proto
Additional context used
Path-based instructions (13)
UPGRADING.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"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.api/cosmos/auth/v1beta1/accounts.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.api/cosmos/auth/v1beta1/tx.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.tests/integration/auth/keeper/accounts_retro_compatibility_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"tests/integration/auth/keeper/migrate_x_accounts_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"x/accounts/defaults/base/account.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"x/auth/keeper/grpc_query.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/keeper/msg_server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/testutil/expected_keepers_mocks.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/types/codec.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/auth/types/expected_keepers.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
LanguageTool
UPGRADING.md
[uncategorized] ~979-~979: Loose punctuation mark.
Context: ...###### Field Descriptions -signer`: The address of the account you want to ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~980-~980: Loose punctuation mark.
Context: ...u want to migrate from. -account_type: The new account type you want to migrat...(UNLIKELY_OPENING_PUNCTUATION)
[style] ~980-~980: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...account_type`: The new account type you want to migrate to (depends on what's installed...(REP_WANT_TO_VB)
[uncategorized] ~981-~981: Loose punctuation mark.
Context: ...lled on the chain). -account_init_msg: The custom initialization message for t...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~982-~982: Loose punctuation mark.
Context: ...ssage for the new account. -@type: Specifies the type of account (in this ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~983-~983: Loose punctuation mark.
Context: .../accounts base account). -pub_key: The public key for the account. You can...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~984-~984: Loose punctuation mark.
Context: ...ic key if desired. -init_sequence: The new sequence number for the account...(UNLIKELY_OPENING_PUNCTUATION)
Markdownlint
UPGRADING.md
953-953: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
954-954: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
979-979: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
980-980: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
981-981: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
982-982: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
983-983: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
984-984: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
x/auth/CHANGELOG.md
64-64: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
Additional comments not posted (36)
x/accounts/proto/cosmos/accounts/defaults/base/v1/base.proto (1)
13-14: LGTM!The addition of the
init_sequencefield to theMsgInitmessage is a valuable enhancement. It provides the flexibility to set a custom initial sequence during account initialization.Key points:
- The field type
uint64is appropriate for representing the sequence number.- The field number
2follows the correct sequential ordering.- The comment accurately describes the purpose of the field.
- This is a non-breaking change, maintaining backward compatibility for existing clients.
Well done!
x/auth/types/codec.go (1)
62-62: LGTM!The registration of
&MsgMigrateAccount{}as an implementation ofcoretransaction.Msginterface is consistent with the existing code pattern and follows the Uber Golang style guide.tests/integration/auth/keeper/migrate_x_accounts_test.go (3)
44-52: LGTM!The subtest correctly verifies the behavior when the account to be migrated does not exist. It checks that the response is nil and the error is
sdkerrors.ErrUnknownAddress.
54-62: LGTM!The subtest correctly verifies the behavior when the account type is invalid for migration. It checks that the response is nil and the error contains "only BaseAccount can be migrated".
64-99: LGTM!The subtest correctly verifies the successful migration of a base account. It checks the following:
- The response has the expected type URL and non-nil value.
- The account is removed from the auth keeper and added to the accounts keeper.
- The sequence and public key in the accounts keeper match the init message.
x/auth/keeper/msg_server.go (3)
7-13: Imports look good, but use "reflect" sparingly.The newly added imports are relevant and justified based on the codebase's apparent use of protocol buffers and gRPC. However, be cautious with the "reflect" package as it can make the code harder to understand and maintain if overused. Use it only when absolutely necessary.
79-116: LGTM!The
MigrateAccountmethod is well-structured and follows a clear sequence of steps to handle the account migration process. It performs necessary checks and validations, handles errors appropriately, and updates the state correctly after a successful migration. The logic aligns with the expected behavior and the code is readable and maintainable.
118-135: TheunpackAnyRawfunction looks good, but be mindful of the reflection trade-off.The
unpackAnyRawfunction correctly handles the unpacking ofcodectypes.Anymessages into concrete message types using reflection. It follows a clear sequence of steps and handles errors appropriately. The use of reflection allows for flexibility in handling different message types, but it comes at the cost of making the code harder to understand and maintain. Be mindful of this trade-off and ensure that the flexibility provided by reflection is necessary and justified in this case.tests/integration/auth/keeper/accounts_retro_compatibility_test.go (2)
Line range hint
49-54: LGTM!The field renaming from
InfotoBasein theauthtypes.QueryLegacyAccountResponsestructure is consistent with the updated data model. The code changes look good.
89-89: LGTM!The assertion update to compare
infoResp.Infowithvalid.retroCompat.Baseis consistent with the field renaming in theauthtypes.QueryLegacyAccountResponsestructure. The code changes look good.x/auth/CHANGELOG.md (2)
64-64: LGTM!The changelog entry accurately describes the new account migration feature and follows the proper format. Great job!
Tools
Markdownlint
64-64: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
64-64: Skipping Markdownlint suggestion.The changelog follows a specific format and structure. Surrounding lists with blank lines, while a general Markdown best practice, may not be applicable or necessary here. Applying the suggestion could negatively impact the readability and consistency of the changelog.
Tools
Markdownlint
64-64: null
Lists should be surrounded by blank lines(MD032, blanks-around-lists)
x/auth/testutil/expected_keepers_mocks.go (2)
137-144: LGTM!The
MigrateLegacyAccountmethod is correctly added to theMockAccountsModKeeperstruct to mock the base method for migrating legacy accounts. The function signature matches the expected signature and the mock implementation usinggomockis correct.
146-150: LGTM!The
MigrateLegacyAccountrecorder method is correctly added to theMockAccountsModKeeperMockRecorderstruct. It matches the signature of the mocked method and correctly records the expected call usinggomock.x/auth/keeper/grpc_query.go (1)
234-234: LGTM!The changes in the error handling logic for accounts fetched from the
x/accountsmodule are correct. The shift from checkingxAccount.InfotoxAccount.Basealigns with the updated data structure used to represent account information in thex/accountsmodule.Also applies to: 237-237
x/accounts/defaults/base/account.go (4)
73-78: LGTM!The changes to the
Initmethod look good. The conditional check ensures that the sequence is only set ifmsg.InitSequenceis non-zero, and proper error handling is in place.
267-277: LGTM!The new
QueryPubKeymethod looks good. It follows a clear sequence of steps to load the public key, wrap it in anAnytype, and return it in the response. Proper error handling is in place.
317-317: LGTM!The renaming of the field from
InfotoBasein theQueryLegacyAccountResponsestruct improves consistency with the naming convention used in the codebase. The change does not impact the functionality of theAuthRetroCompatibilitymethod.
332-332: LGTM!The registration of the
QueryPubKeymethod in theRegisterQueryHandlersmethod is necessary to make the new query handler accessible. The registration follows the same pattern as the existing query handlers.api/cosmos/auth/v1beta1/accounts.pulsar.go (2)
375-375: LGTM!The renaming of the
Infofield toBasein theQueryLegacyAccountResponsestruct is consistently applied across the file. The comments have been updated accurately to reflect the new field name.Also applies to: 382-382, 456-458, 479-480, 499-500, 520-521, 593-593, 940-943, 973-975, 996-999, 1037-1037
Line range hint
1-1:api/cosmos/accounts/defaults/base/v1/base.pulsar.go (14)
17-19: Addition of 'InitSequence' Field DescriptorThe field descriptor
fd_MsgInit_init_sequenceis correctly added to support the newInitSequencefield in theMsgInitmessage.
26-26: Initialization of 'fd_MsgInit_init_sequence'Properly initializes
fd_MsgInit_init_sequenceby retrieving theinit_sequencefield frommd_MsgInit.
100-105: Update 'Range' Method to Include 'InitSequence'The
Rangemethod now correctly iterates overInitSequencewhen it is non-zero, ensuring the field is included during reflection.
123-124: Update 'Has' Method for 'InitSequence'The
Hasmethod appropriately checks ifInitSequenceis populated by verifying it is not equal to zero.
143-144: Update 'Clear' Method for 'InitSequence'The
Clearmethod correctly resetsInitSequenceto its zero value, effectively clearing the field.
164-166: Update 'Get' Method for 'InitSequence'The
Getmethod properly retrieves the value ofInitSequenceand returns it as aprotoreflect.Value.
189-190: Update 'Set' Method for 'InitSequence'The
Setmethod correctly assigns a new value toInitSequencefrom the providedprotoreflect.Value.
216-217: Handling 'Mutable' Method for Non-Mutable Field 'InitSequence'Attempting to call
MutableonInitSequencecorrectly results in a panic, as scalar fields are not mutable. This behavior is consistent with the handling of non-mutable fields in the codebase.
234-235: Update 'NewField' Method for 'InitSequence'The
NewFieldmethod appropriately returns a default value forInitSequence, aligning with the expected behavior for scalar fields.
309-311: Update 'size' Function in 'ProtoMethods' to Account for 'InitSequence'The size calculation now includes
InitSequencewhen it is set, ensuring accurate size determination during serialization.
341-345: Update 'marshal' Function in 'ProtoMethods' for 'InitSequence'The
marshalfunction correctly handles serialization ofInitSequence, writing it to the byte buffer when it is non-zero.
445-463: Update 'unmarshal' Function in 'ProtoMethods' for 'InitSequence'The
unmarshalfunction properly deserializesInitSequencefrom the input buffer, correctly interpreting the wire type and field number.
3218-3219: Addition of 'InitSequence' Field to 'MsgInit' StructThe
InitSequencefield is added to theMsgInitstruct with the correct protobuf tags, enabling serialization and deserialization.
3249-3255: Implementation of 'GetInitSequence' MethodThe getter method for
InitSequenceis accurately implemented, providing safe access to the field's value.api/cosmos/auth/v1beta1/tx.pulsar.go (1)
2426-3423: LGTM: Implementation ofMsgMigrateAccountandMsgMigrateAccountResponseThe new message types
MsgMigrateAccountandMsgMigrateAccountResponse, along with their associated reflection methods, are correctly implemented and conform to the Go and protobuf conventions.
| option (cosmos.msg.v1.signer) = "signer"; | ||
|
|
||
| string signer = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"]; | ||
| string account_type = 2; |
There was a problem hiding this comment.
Consider using an enumeration for the account_type field
Instead of using a string for account_type, consider defining an enum to enforce valid account types and prevent invalid values. This enhances type safety and makes the code more robust.
Example:
enum AccountType {
UNKNOWN = 0;
MODULE_ACCOUNT = 1;
CUSTOM_ACCOUNT = 2;
// Add other account types as needed.
}
...
AccountType account_type = 2;| // init_response defines the response returned by the x/account account | ||
| // initialization. |
There was a problem hiding this comment.
Improve the description of init_response field
Clarify the comment for the init_response field to provide better understanding of its contents.
Apply this diff:
// init_response defines the response returned by the x/account account
-// initialization.
+// initialization, containing any data returned by the initialization process of the new account type.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.
| // init_response defines the response returned by the x/account account | |
| // initialization. | |
| // init_response defines the response returned by the x/account account | |
| // initialization, containing any data returned by the initialization process of the new account type. |
| // MsgMigrateAccount defines a message which allows users to migrate from BaseAccount | ||
| // to other x/accounts types. |
There was a problem hiding this comment.
Clarify the description of MsgMigrateAccount
To improve clarity, rephrase the comment describing MsgMigrateAccount to better convey its functionality.
Apply this diff:
// MsgMigrateAccount defines a message which allows users to migrate from BaseAccount
-// to other x/accounts types.
+// to other account types in the `x/accounts` module.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.
| // MsgMigrateAccount defines a message which allows users to migrate from BaseAccount | |
| // to other x/accounts types. | |
| // MsgMigrateAccount defines a message which allows users to migrate from BaseAccount | |
| // to other account types in the `x/accounts` module. |
|
|
||
| string signer = 1 [(cosmos_proto.scalar) = "cosmos.AddressString"]; | ||
| string account_type = 2; | ||
| google.protobuf.Any account_init_msg = 3; |
There was a problem hiding this comment.
Specify accepted interface for account_init_msg field
To ensure type safety and clarity, please specify the (cosmos_proto.accepts_interface) option for the account_init_msg field, indicating the expected interface that the initialization message should implement.
Apply this diff:
google.protobuf.Any account_init_msg = 3
+ [(cosmos_proto.accepts_interface) = "cosmos.accounts.v1beta1.AccountInitMsg"];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.
| google.protobuf.Any account_init_msg = 3; | |
| google.protobuf.Any account_init_msg = 3 | |
| [(cosmos_proto.accepts_interface) = "cosmos.accounts.v1beta1.AccountInitMsg"]; |
| @@ -22,6 +22,9 @@ | |||
|
|
|||
| // NonAtomicExec allows users to submit multiple messages for non-atomic execution. | |||
| rpc NonAtomicExec(MsgNonAtomicExec) returns (MsgNonAtomicExecResponse); | |||
|
|
|||
| // MigrateAccount migrates the account to x/accounts. | |||
There was a problem hiding this comment.
Enhance the documentation of the MigrateAccount RPC method
Consider providing a more detailed comment for the MigrateAccount method to clearly explain its purpose and usage, helping developers understand how to utilize this new functionality.
Apply this diff:
// MigrateAccount migrates the account to x/accounts.
+ // Allows a user to migrate their account from `x/auth` to the `x/accounts` module,
+ // enabling them to switch to a different account type with extended functionalities.
rpc MigrateAccount(MsgMigrateAccount) returns (MsgMigrateAccountResponse);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.
| // MigrateAccount migrates the account to x/accounts. | |
| // MigrateAccount migrates the account to x/accounts. | |
| // Allows a user to migrate their account from `x/auth` to the `x/accounts` module, | |
| // enabling them to switch to a different account type with extended functionalities. | |
| rpc MigrateAccount(MsgMigrateAccount) returns (MsgMigrateAccountResponse); |
| message MsgMigrateAccountResponse { | ||
| // init_response defines the response returned by the x/account account |
There was a problem hiding this comment.
Add amino.name option to MsgMigrateAccountResponse
Including the option (amino.name) in MsgMigrateAccountResponse ensures consistency and supports Amino serialization.
Apply this diff:
message MsgMigrateAccountResponse {
+ option (amino.name) = "cosmos-sdk/MsgMigrateAccountResponse";
// init_response defines the response returned by the x/account accountCommittable 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.
| message MsgMigrateAccountResponse { | |
| // init_response defines the response returned by the x/account account | |
| message MsgMigrateAccountResponse { | |
| option (amino.name) = "cosmos-sdk/MsgMigrateAccountResponse"; | |
| // init_response defines the response returned by the x/account account |
Add message_added_in option to MsgMigrateAccountResponse message
For completeness and to aid documentation, add the option (cosmos_proto.message_added_in) to the MsgMigrateAccountResponse message.
Apply this diff:
message MsgMigrateAccountResponse {
+ option (cosmos_proto.message_added_in) = "cosmos-sdk 0.51";
// init_response defines the response returned by the x/account accountCommittable 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.
| message MsgMigrateAccountResponse { | |
| // init_response defines the response returned by the x/account account | |
| message MsgMigrateAccountResponse { | |
| option (cosmos_proto.message_added_in) = "cosmos-sdk 0.51"; | |
| // init_response defines the response returned by the x/account account |
| message MsgMigrateAccountResponse { | ||
| // init_response defines the response returned by the x/account account | ||
| // initialization. | ||
| google.protobuf.Any init_response = 1; |
There was a problem hiding this comment.
Specify accepted interface for init_response field
Similarly, add the (cosmos_proto.accepts_interface) option to the init_response field to indicate the expected interface of the initialization response.
Apply this diff:
google.protobuf.Any init_response = 1;
+ [(cosmos_proto.accepts_interface) = "cosmos.accounts.v1beta1.AccountInitResponse"];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.
| google.protobuf.Any init_response = 1; | |
| google.protobuf.Any init_response = 1 | |
| [(cosmos_proto.accepts_interface) = "cosmos.accounts.v1beta1.AccountInitResponse"]; |
| message MsgMigrateAccount { | ||
| option (cosmos.msg.v1.signer) = "signer"; |
There was a problem hiding this comment.
Add amino.name option to MsgMigrateAccount
To ensure compatibility with Amino serialization, consider adding the option (amino.name) to MsgMigrateAccount, following the pattern used in other messages.
Apply this diff:
message MsgMigrateAccount {
option (cosmos.msg.v1.signer) = "signer";
+ option (amino.name) = "cosmos-sdk/MsgMigrateAccount";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.
| message MsgMigrateAccount { | |
| option (cosmos.msg.v1.signer) = "signer"; | |
| message MsgMigrateAccount { | |
| option (cosmos.msg.v1.signer) = "signer"; | |
| option (amino.name) = "cosmos-sdk/MsgMigrateAccount"; |
Add message_added_in option to MsgMigrateAccount message
Please include the option (cosmos_proto.message_added_in) in the MsgMigrateAccount message to indicate the version it was added, ensuring consistency across messages.
Apply this diff:
message MsgMigrateAccount {
+ option (cosmos_proto.message_added_in) = "cosmos-sdk 0.51";
option (cosmos.msg.v1.signer) = "signer";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.
| message MsgMigrateAccount { | |
| option (cosmos.msg.v1.signer) = "signer"; | |
| message MsgMigrateAccount { | |
| option (cosmos_proto.message_added_in) = "cosmos-sdk 0.51"; | |
| option (cosmos.msg.v1.signer) = "signer"; |
| // MigrateAccount migrates the account to x/accounts. | ||
| rpc MigrateAccount(MsgMigrateAccount) returns (MsgMigrateAccountResponse); |
There was a problem hiding this comment.
Add method_added_in option to MigrateAccount RPC method
To maintain consistency and aid in version tracking, please add the option (cosmos_proto.method_added_in) to the MigrateAccount RPC method, specifying the version in which it was introduced, similar to the UpdateParams method.
Apply this diff:
rpc MigrateAccount(MsgMigrateAccount) returns (MsgMigrateAccountResponse);
+ {
+ option (cosmos_proto.method_added_in) = "cosmos-sdk 0.51";
+ }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.
| // MigrateAccount migrates the account to x/accounts. | |
| rpc MigrateAccount(MsgMigrateAccount) returns (MsgMigrateAccountResponse); | |
| // MigrateAccount migrates the account to x/accounts. | |
| rpc MigrateAccount(MsgMigrateAccount) returns (MsgMigrateAccountResponse) { | |
| option (cosmos_proto.method_added_in) = "cosmos-sdk 0.51"; | |
| } |
# Conflicts: # api/cosmos/auth/v1beta1/accounts.pulsar.go # x/auth/types/accounts.pb.go
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (4)
proto/cosmos/auth/v1beta1/tx.proto (1)
26-27: LGTM! Consider adding signer option.The new RPC method
MigrateAccountis well-placed and correctly defined. It aligns with the feature description for migrating accounts.Consider adding the
option (cosmos.msg.v1.signer)to the RPC method for consistency with other methods in the service:rpc MigrateAccount(MsgMigrateAccount) returns (MsgMigrateAccountResponse); + option (cosmos.msg.v1.signer) = "signer";UPGRADING.md (3)
949-950: Improve section header formattingThe section header for the account migration guide should be formatted as a proper Markdown header for better readability and structure.
Consider changing the header to:
-#### Account Migration Guide: x/auth to x/accounts +### Account Migration Guide: x/auth to x/accounts
958-968: Improve formatting and clarity of migration process explanationThe explanation of the migration process could be formatted better for improved readability. Additionally, some clarifications could be added to make the process more understandable.
Consider the following improvements:
- Use proper Markdown formatting for headers and lists.
- Clarify the relationship between
auth.MsgMigrateAccountandbasev1.MsgInit.- Explain why users might want to switch to a new public key or reset their sequence number.
Here's a suggested revision:
### Migration Process The migration process allows users to migrate from a legacy `x/auth` `BaseAccount` to any kind of `x/accounts` supported account type. Here's how to migrate to an `x/accounts` `BaseAccount`: 1. Send an `auth.MsgMigrateAccount` message. 2. Within this message, include a `basev1.MsgInit` message, which allows you to: - Switch to a new public key (optional) - Reset your sequence number (optional) > **Important**: If you intend to keep the same public key, ensure you use your current sequence number to prevent potential replay attacks.Tools
Markdownlint
963-963: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
964-964: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
987-997: Improve field descriptions formatting and clarityThe field descriptions could be formatted better for improved readability. Some additional explanations could also be helpful.
Consider the following improvements:
- Use proper Markdown formatting for the list.
- Provide more context for the
account_typefield.- Clarify the purpose and importance of the
init_sequencefield.Here's a suggested revision:
### Field Descriptions * `signer`: The address of the account you want to migrate from. * `account_type`: The new account type you want to migrate to. This depends on what account types are installed on the chain. In this example, it's set to "base" for a base account. * `account_init_msg`: The custom initialization message for the new account. - `@type`: Specifies the type of account (in this case, x/accounts base account). - `pub_key`: The public key for the account. You can migrate to a different public key if desired. - `init_sequence`: The new sequence number for the account. This is crucial for preventing replay attacks. If you're keeping the same public key, use your current sequence number. > **Warning**: If you're keeping the same public key, make sure to use your current sequence number to prevent potential replay attacks.Tools
LanguageTool
[uncategorized] ~989-~989: Loose punctuation mark.
Context: ...###### Field Descriptions -signer`: The address of the account you want to ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~990-~990: Loose punctuation mark.
Context: ...u want to migrate from. -account_type: The new account type you want to migrat...(UNLIKELY_OPENING_PUNCTUATION)
[style] ~990-~990: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...account_type`: The new account type you want to migrate to (depends on what's installed...(REP_WANT_TO_VB)
[uncategorized] ~991-~991: Loose punctuation mark.
Context: ...lled on the chain). -account_init_msg: The custom initialization message for t...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~992-~992: Loose punctuation mark.
Context: ...ssage for the new account. -@type: Specifies the type of account (in this ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~993-~993: Loose punctuation mark.
Context: .../accounts base account). -pub_key: The public key for the account. You can...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~994-~994: Loose punctuation mark.
Context: ...ic key if desired. -init_sequence: The new sequence number for the account...(UNLIKELY_OPENING_PUNCTUATION)
Markdownlint
989-989: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
990-990: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
991-991: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
992-992: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
993-993: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
994-994: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (3)
- UPGRADING.md (1 hunks)
- proto/cosmos/auth/v1beta1/accounts.proto (1 hunks)
- proto/cosmos/auth/v1beta1/tx.proto (2 hunks)
Files skipped from review due to trivial changes (1)
- proto/cosmos/auth/v1beta1/accounts.proto
Additional context used
Path-based instructions (1)
UPGRADING.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
UPGRADING.md
[uncategorized] ~989-~989: Loose punctuation mark.
Context: ...###### Field Descriptions -signer`: The address of the account you want to ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~990-~990: Loose punctuation mark.
Context: ...u want to migrate from. -account_type: The new account type you want to migrat...(UNLIKELY_OPENING_PUNCTUATION)
[style] ~990-~990: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...account_type`: The new account type you want to migrate to (depends on what's installed...(REP_WANT_TO_VB)
[uncategorized] ~991-~991: Loose punctuation mark.
Context: ...lled on the chain). -account_init_msg: The custom initialization message for t...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~992-~992: Loose punctuation mark.
Context: ...ssage for the new account. -@type: Specifies the type of account (in this ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~993-~993: Loose punctuation mark.
Context: .../accounts base account). -pub_key: The public key for the account. You can...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~994-~994: Loose punctuation mark.
Context: ...ic key if desired. -init_sequence: The new sequence number for the account...(UNLIKELY_OPENING_PUNCTUATION)
Markdownlint
UPGRADING.md
963-963: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
964-964: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
989-989: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
990-990: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
991-991: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
992-992: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
993-993: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
994-994: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
Additional comments not posted (5)
proto/cosmos/auth/v1beta1/tx.proto (3)
71-79: LGTM! Well-structured message definition.The
MsgMigrateAccountmessage is well-defined and includes all necessary fields for the account migration feature. The use ofgoogle.protobuf.Anyforaccount_init_msgprovides the required flexibility for different account types.
81-87: LGTM! Clear and flexible response message.The
MsgMigrateAccountResponsemessage is well-defined. The use ofgoogle.protobuf.Anyforinit_responseallows for flexible response types from different account initializations.
Line range hint
25-87: Overall, the changes effectively implement the account migration feature.The additions to this file (new RPC method and message types) are well-structured and consistent with the PR objectives. They provide the necessary flexibility for migrating different account types while maintaining a clear and logical structure.
Key points:
- The new
MigrateAccountRPC method is appropriately placed in theMsgservice.MsgMigrateAccountincludes all necessary fields for initiating a migration.MsgMigrateAccountResponseallows for flexible responses from different account initializations.These changes lay a solid foundation for the account migration feature in the Cosmos SDK.
UPGRADING.md (2)
951-957: LGTM: Clear introduction to account migrationThe introduction provides a concise explanation of the new account migration feature and its current limitations. This is helpful for users to understand the scope of the migration process.
969-986: LGTM: Clear example of x/auth.MsgMigrateAccountThe provided example of the
x/auth.MsgMigrateAccountmessage structure is clear and helpful. It demonstrates the correct format and includes all necessary fields.
julienrbrt
added
the
backport/v0.52.x
| @@ -59,5 +59,6 @@ func RegisterInterfaces(registrar registry.InterfaceRegistrar) { | |||
| registrar.RegisterImplementations((*coretransaction.Msg)(nil), | |||
| &MsgUpdateParams{}, | |||
| &MsgNonAtomicExec{}, | |||
| &MsgMigrateAccount{}, | |||
There was a problem hiding this comment.
we should register it on amino above I think
Co-authored-by: Akhil Kumar P <36399231+akhilkumarpilli@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (1)
- UPGRADING.md (1 hunks)
Additional context used
Path-based instructions (1)
UPGRADING.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
LanguageTool
UPGRADING.md
[uncategorized] ~989-~989: Loose punctuation mark.
Context: ... ``` Field Descriptions -signer: The address of the account you want to ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~990-~990: Loose punctuation mark.
Context: ...u want to migrate from. -account_type: The new account type you want to migrat...(UNLIKELY_OPENING_PUNCTUATION)
[style] ~990-~990: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...account_type`: The new account type you want to migrate to (depends on what's installed...(REP_WANT_TO_VB)
[uncategorized] ~991-~991: Loose punctuation mark.
Context: ...lled on the chain). -account_init_msg: The custom initialization message for t...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~992-~992: Loose punctuation mark.
Context: ...ssage for the new account. -@type: Specifies the type of account (in this ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~993-~993: Loose punctuation mark.
Context: .../accounts base account). -pub_key: The public key for the account. You can...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~994-~994: Loose punctuation mark.
Context: ...ic key if desired. -init_sequence: The new sequence number for the account...(UNLIKELY_OPENING_PUNCTUATION)
Markdownlint
UPGRADING.md
963-963: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
964-964: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
989-989: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
990-990: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
991-991: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
992-992: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
993-993: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
994-994: Expected: asterisk; Actual: dash
Unordered list style(MD004, ul-style)
987-987: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Co-authored-by: Akhil Kumar P <36399231+akhilkumarpilli@users.noreply.github.com> (cherry picked from commit ebbaa0e) # Conflicts: # UPGRADING.md # api/cosmos/accounts/defaults/base/v1/base.pulsar.go # api/cosmos/auth/v1beta1/accounts.pulsar.go # api/cosmos/auth/v1beta1/tx.pulsar.go # api/cosmos/auth/v1beta1/tx_grpc.pb.go
Description
This PR allows for a way to explictly migrate BaseAccounts from x/auth to x/accounts, the choice of target account is left to the message sender.
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
MsgMigrateAccount.MsgInitmessage structure for account initialization, allowing optional sequence number adjustments.MigrateAccountadded to facilitate account migrations.AccountsModKeeperinterface with a method for migrating legacy accounts.Bug Fixes
Documentation