refactor(x/account): Enable AutoCLI for x/accounts #22727
Conversation
DongLieu
requested review from
testinginprod,
sontrinh16 and
a team
as code owners
📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the Cosmos SDK's account management module, primarily focusing on the restructuring of message formats and the implementation of AutoCLI functionality. Key changes include the replacement of protobuf message fields with JSON string representations in various message types, such as Changes
Assessment against linked issues
Possibly related issues
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (11)
x/accounts/module.go (2)
99-99: ChangeGetTxCmdmethod to explicitly indicate deprecated statusReturning
nilfromGetTxCmdmay cause confusion. Consider adding a comment to indicate that the transaction command is deprecated or intentionally not implemented.Apply this diff to add a deprecation comment:
func (AppModule) GetTxCmd() *cobra.Command { + // GetTxCmd returns nil as transaction commands are now handled by AutoCLI. return nil }
103-103: ChangeGetQueryCmdmethod to explicitly indicate deprecated statusSimilarly, for
GetQueryCmd, consider adding a comment to clarify that query commands are now handled differently.Apply this diff to add a clarification comment:
func (AppModule) GetQueryCmd() *cobra.Command { + // GetQueryCmd returns nil as query commands are now handled by AutoCLI. return nil }x/accounts/keeper.go (1)
183-183: Ensure proper error handling when encoding JSON to ProtoWhile calling
implementation.EncodeMsgJSONToProto, consider adding a check for the specific type of errors returned, to provide more context or handle certain error conditions differently if necessary.No code changes are necessary, but consider reviewing the error handling strategy for this function.
api/cosmos/accounts/v1/tx.pulsar.go (1)
Line range hint
75-84: Rename 'JsonMessage' to 'JSONMessage' to follow Go naming conventionsAccording to the Uber Go Style Guide, initialisms like "JSON" should be capitalized consistently throughout the code. Consider renaming
JsonMessagetoJSONMessageto improve readability and maintain consistency.Apply this diff to rename the field descriptors:
-fd_MsgInit_json_message protoreflect.FieldDescriptor +fd_MsgInit_JSONMessage protoreflect.FieldDescriptor ... -fd_MsgInit_json_message = md_MsgInit.Fields().ByName("json_message") +fd_MsgInit_JSONMessage = md_MsgInit.Fields().ByName("json_message")x/accounts/msg_server_test.go (1)
6-6: Organize imports according to project guidelinesThe import statements are not ordered according to the project's conventions. Please sort and group the imports properly.
Apply this diff to organize the imports:
import ( "testing" + "cosmossdk.io/x/accounts/accountstd" "github.com/stretchr/testify/require" v1 "cosmossdk.io/x/accounts/v1" )🧰 Tools
🪛 golangci-lint (1.62.2)
6-6: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
x/accounts/internal/implementation/encoding.go (2)
4-4: Organize imports according to project guidelinesThe import statements should be grouped and ordered according to project conventions for better readability.
Apply this diff to organize the imports:
import ( + "bytes" "fmt" "reflect" "strings" + "github.com/cosmos/gogoproto/jsonpb" "github.com/cosmos/gogoproto/proto" "cosmossdk.io/core/transaction" codectypes "github.com/cosmos/cosmos-sdk/codec/types" )Also applies to: 14-14
71-82: Useprotojsoninstead ofjsonpbfor JSON unmarshalingThe
jsonpbpackage is deprecated. Consider usingprotojsonfromgoogle.golang.org/protobuf/encoding/protojsonfor better compatibility with new protobuf APIs.Apply this diff to update the unmarshaling code:
-import ( - "bytes" - "fmt" - "reflect" - "strings" - "github.com/cosmos/gogoproto/jsonpb" - "github.com/cosmos/gogoproto/proto" - "cosmossdk.io/core/transaction" - codectypes "github.com/cosmos/cosmos-sdk/codec/types" -) +import ( + "bytes" + "fmt" + "reflect" + "strings" + "google.golang.org/protobuf/encoding/protojson" + "github.com/cosmos/gogoproto/proto" + "cosmossdk.io/core/transaction" + codectypes "github.com/cosmos/cosmos-sdk/codec/types" +) func EncodeMsgJSONToProto(name, jsonMsg string) (proto.Message, error) { typ := proto.MessageType(name) if typ == nil { return nil, fmt.Errorf("message type %s not found", name) } msg := reflect.New(typ.Elem()).Interface().(proto.Message) - err := jsonpb.Unmarshal(bytes.NewBufferString(jsonMsg), msg) + err := protojson.Unmarshal([]byte(jsonMsg), msg) if err != nil { return nil, fmt.Errorf("failed to unmarshal JSON to proto.Message: %w", err) } return msg, nil }x/accounts/query_server_test.go (2)
8-8: Remove commented-out importThe import statement for
emptypbis commented out. It's best to remove unused imports to keep the codebase clean.Apply this diff to remove the commented-out import:
- // "google.golang.org/protobuf/types/known/emptypb"🧰 Tools
🪛 golangci-lint (1.62.2)
8-8: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
8-8: Organize imports according to project guidelinesThe import statements should be sorted according to the project's conventions for consistency and readability.
After removing unnecessary imports, ensure the remaining imports are organized properly.
🧰 Tools
🪛 golangci-lint (1.62.2)
8-8: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
tests/integration/accounts/multisig/account_test.go (2)
146-151: Consider simplifying the type URL extractionWhile the current implementation works, consider using
strings.TrimPrefix()for cleaner type URL extraction. Also, consider adding a constant for the type URL prefix to avoid magic strings.- split := strings.Split(anyUpdateMsg.TypeUrl, "/") - nameTypeUrl := split[len(split)-1] + const typeURLPrefix = "/" + nameTypeUrl := strings.TrimPrefix(anyUpdateMsg.TypeUrl, typeURLPrefix)
184-211: Consider extracting message preparation logic into a helper functionThere's duplicate code for preparing the MsgExecute. Consider creating a test helper function to reduce duplication and improve maintainability.
func (s *IntegrationTestSuite) prepareExecuteMsg(msg proto.Message, sender, target string) (*accountsv1.MsgExecute, error) { anyMsg := codectypes.UnsafePackAny(msg) typeURL := strings.TrimPrefix(anyMsg.TypeUrl, "/") jsonMsg, err := json.Marshal(msg) if err != nil { return nil, err } return &accountsv1.MsgExecute{ Sender: sender, Target: target, ExecuteMsgTypeUrl: typeURL, JsonMessage: string(jsonMsg), Funds: []sdk.Coin{}, }, nil }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/accounts/v1/query.pb.gois excluded by!**/*.pb.gox/accounts/v1/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (18)
api/cosmos/accounts/v1/query.pulsar.go(16 hunks)api/cosmos/accounts/v1/tx.pulsar.go(31 hunks)tests/integration/accounts/multisig/account_test.go(4 hunks)x/accounts/autocli.go(1 hunks)x/accounts/cli/cli.go(0 hunks)x/accounts/cli/cli_test.go(0 hunks)x/accounts/defaults/multisig/account_test.go(5 hunks)x/accounts/genesis_test.go(1 hunks)x/accounts/internal/implementation/encoding.go(3 hunks)x/accounts/keeper.go(1 hunks)x/accounts/module.go(1 hunks)x/accounts/msg_server.go(1 hunks)x/accounts/msg_server_test.go(2 hunks)x/accounts/proto/cosmos/accounts/v1/query.proto(1 hunks)x/accounts/proto/cosmos/accounts/v1/tx.proto(2 hunks)x/accounts/query_server.go(1 hunks)x/accounts/query_server_test.go(2 hunks)x/accounts/v1/schema.go(1 hunks)
💤 Files with no reviewable changes (2)
- x/accounts/cli/cli_test.go
- x/accounts/cli/cli.go
🧰 Additional context used
📓 Path-based instructions (14)
x/accounts/internal/implementation/encoding.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/accounts/v1/schema.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/accounts/query_server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/accounts/genesis_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/query_server_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/module.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/accounts/msg_server.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/accounts/msg_server_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/multisig/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"
tests/integration/accounts/multisig/account_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/autocli.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/accounts/keeper.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
api/cosmos/accounts/v1/query.pulsar.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
api/cosmos/accounts/v1/tx.pulsar.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
📓 Learnings (1)
x/accounts/msg_server.go (1)
Learnt from: testinginprod
PR: cosmos/cosmos-sdk#18499
File: x/accounts/msg_server.go:123-125
Timestamp: 2024-11-10T03:53:32.474Z
Learning: The `ExecuteBundle` method in `x/accounts/msg_server.go` is intentionally left unimplemented and is expected to be addressed in a follow-up PR.
🪛 golangci-lint (1.62.2)
x/accounts/internal/implementation/encoding.go
14-14: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
x/accounts/query_server_test.go
8-8: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
x/accounts/msg_server_test.go
6-6: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
x/accounts/defaults/multisig/account_test.go
5-5: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
6-6: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
20-20: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
25-25: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
19-19: File is not gofumpt-ed with -extra
(gofumpt)
580-580: ineffectual assignment to err
(ineffassign)
x/accounts/autocli.go
3-3: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
6-6: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
🔇 Additional comments (27)
api/cosmos/accounts/v1/tx.pulsar.go (5)
Line range hint 607-635: LGTM
The changes to the Unmarshal function correctly handle the new JsonMessage field and ensure proper parsing of the JSON string.
1205-1252: LGTM
The implementation of the list structures for handling repeated fields is correct and aligns with protobuf reflection requirements.
1622-1627: LGTM
The updates in the size calculation function appropriately account for the new fields, ensuring accurate message size computation.
Line range hint 1824-1886: LGTM
The modifications in the Unmarshal function for MsgExecute correctly parse the new execute_msg_type_url and json_message fields.
1443-1448: 🛠️ Refactor suggestion
Rename getters to use 'JSONMessage' and 'ExecuteMsgTypeURL'
Update method names to reflect the proper capitalization of initialisms.
Apply this diff:
-func (x *fastReflection_MsgExecute) Get(descriptor protoreflect.FieldDescriptor) protoreflect.Value {
...
-case "cosmos.accounts.v1.MsgExecute.execute_msg_type_url":
+case "cosmos.accounts.v1.MsgExecute.execute_msg_type_URL":
...
-case "cosmos.accounts.v1.MsgExecute.json_message":
+case "cosmos.accounts.v1.MsgExecute.JSON_message":Likely invalid or redundant comment.
api/cosmos/accounts/v1/query.pulsar.go (12)
17-20: New field descriptors added appropriately
The new field descriptors fd_AccountQueryRequest_query_request_type_url and fd_AccountQueryRequest_json_message are defined consistently with existing code conventions.
27-28: Assignment of new field descriptors is correct
The field descriptors for query_request_type_url and json_message are properly assigned using ByName, ensuring correct field mapping.
102-110: Range method updated to include new fields
The Range method now correctly iterates over QueryRequestTypeUrl and JsonMessage fields when they are set, ensuring they are handled during field enumeration.
131-134: Has method correctly checks new fields
The Has method properly checks for the presence of QueryRequestTypeUrl and JsonMessage, maintaining consistency with the handling of existing fields.
153-156: Clear method correctly clears new fields
The Clear method now appropriately handles the new fields by setting QueryRequestTypeUrl and JsonMessage to empty strings.
176-181: Get method retrieves new fields accurately
The Get method now includes cases for QueryRequestTypeUrl and JsonMessage, returning their values correctly.
204-207: Set method assigns values to new fields correctly
The Set method now allows setting the QueryRequestTypeUrl and JsonMessage fields, ensuring they can be updated when needed.
230-233: Mutable method correctly handles new fields
As string fields, QueryRequestTypeUrl and JsonMessage are not mutable, and the Mutable method correctly panics if attempted, consistent with expected behavior.
249-252: NewField method supports new fields
The NewField method returns the default values for QueryRequestTypeUrl and JsonMessage, ensuring they can be initialized properly.
326-331: Size calculation includes new fields
The size function correctly calculates the size of QueryRequestTypeUrl and JsonMessage, which is essential for accurate marshaling.
363-373: Marshaling includes new fields
The marshal function properly handles the serialization of QueryRequestTypeUrl and JsonMessage, ensuring they are marshaled into the output buffer.
Line range hint 467-527: Unmarshaling handles new fields correctly
The unmarshal function is updated to parse QueryRequestTypeUrl and JsonMessage fields from the input buffer, accurately reconstructing the message.
x/accounts/msg_server_test.go (2)
20-20: Initialize JsonMessage with an empty JSON object
The JsonMessage field is correctly set to {}, ensuring the initialization message aligns with the expected JSON format.
27-30: Update MsgExecute to use new JSON fields
The test correctly populates ExecuteMsgTypeUrl and JsonMessage, aligning with the changes in message structure to use JSON representations.
x/accounts/query_server_test.go (3)
24-24: Initialize JsonMessage with an empty JSON object
The JsonMessage field is correctly set to {}, reflecting the new requirement for JSON-based message initialization.
30-32: Update AccountQueryRequest with new JSON fields
The test correctly sets QueryRequestTypeUrl and JsonMessage, aligning with the updated query request structure.
38-42: Assert response type and value correctly
The updated test now asserts the response is of type *types.StringValue and checks its value, ensuring accuracy in the test verification.
x/accounts/msg_server.go (1)
69-71: LGTM: Message encoding change aligns with AutoCLI objectives
The change from UnpackAnyRaw to EncodeMsgJSONToProto successfully moves message encoding responsibility to the module, which is essential for AutoCLI integration. The error handling is properly maintained.
x/accounts/query_server.go (1)
37-40: LGTM: Query handling aligned with new message encoding approach
The change consistently applies the new message encoding pattern across both message and query servers, maintaining proper error handling.
x/accounts/proto/cosmos/accounts/v1/query.proto (1)
26-28: LGTM: Improved message structure for AutoCLI compatibility
The replacement of the generic Any type with explicit query_request_type_url and json_message fields improves type safety and aligns with AutoCLI requirements. The field ordering and types are appropriate for the use case.
x/accounts/genesis_test.go (1)
46-46: LGTM: JsonMessage field correctly represents empty message
The change from protobuf Any to JSON string representation aligns with the PR's objective of moving message encoding responsibility to the module.
tests/integration/accounts/multisig/account_test.go (1)
4-5: LGTM: Required imports added for JSON handling
The addition of json and strings packages supports the new JSON-based message handling approach.
| import ( | ||
| accountsv1 "cosmossdk.io/api/cosmos/accounts/v1" | ||
| autocliv1 "cosmossdk.io/api/cosmos/autocli/v1" | ||
| "fmt" | ||
|
|
||
| "github.com/cosmos/cosmos-sdk/version" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Organize imports according to Go conventions
The import statements are not properly grouped and ordered. According to the Go coding conventions and the Uber Go Style Guide, imports should be grouped in the following order: standard library packages, third-party packages, and project-specific packages, each separated by a blank line.
Apply this diff to organize the imports:
import (
+ "fmt"
+ "github.com/cosmos/cosmos-sdk/version"
accountsv1 "cosmossdk.io/api/cosmos/accounts/v1"
autocliv1 "cosmossdk.io/api/cosmos/autocli/v1"
- "fmt"
- "github.com/cosmos/cosmos-sdk/version"
)📝 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.
| import ( | |
| accountsv1 "cosmossdk.io/api/cosmos/accounts/v1" | |
| autocliv1 "cosmossdk.io/api/cosmos/autocli/v1" | |
| "fmt" | |
| "github.com/cosmos/cosmos-sdk/version" | |
| ) | |
| import ( | |
| "fmt" | |
| "github.com/cosmos/cosmos-sdk/version" | |
| accountsv1 "cosmossdk.io/api/cosmos/accounts/v1" | |
| autocliv1 "cosmossdk.io/api/cosmos/autocli/v1" | |
| ) |
🧰 Tools
🪛 golangci-lint (1.62.2)
3-3: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
6-6: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
| @@ -34,7 +34,7 @@ message MsgInit { | |||
| // account_type is the type of the account to be created. | |||
| string account_type = 2; | |||
| // message is the message to be sent to the account. | |||
| google.protobuf.Any message = 3; | |||
| string json_message = 3; | |||
There was a problem hiding this comment.
Avoid reusing field numbers for different types to maintain backward compatibility
Changing the type of existing fields or reusing field numbers can break backward compatibility in Protocol Buffers. The fields message of type google.protobuf.Any are being replaced with new fields of different types but the same field numbers. This can cause deserialization issues with existing clients.
To maintain backward compatibility, consider assigning new, unused field numbers to the new fields and marking the old fields as reserved or deprecated.
Apply this diff to adjust field numbers:
// MsgInit defines the Create request type for the Msg/Create RPC method.
message MsgInit {
option (cosmos.msg.v1.signer) = "sender";
// sender is the address of the sender of this message.
string sender = 1;
// account_type is the type of the account to be created.
string account_type = 2;
- // message is the message to be sent to the account.
- google.protobuf.Any message = 3;
+ // json_message is the JSON-encoded message to be sent to the account.
+ string json_message = 4;
// funds contains the coins that the account wants to
// send alongside the request.
- repeated cosmos.base.v1beta1.Coin funds = 4
+ repeated cosmos.base.v1beta1.Coin funds = 5
[(gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins", (gogoproto.nullable) = false];
+ // Reserved field numbers to prevent reuse.
+ reserved 3;
}
// MsgExecute defines the Execute request type for the Msg/Execute RPC method.
message MsgExecute {
option (cosmos.msg.v1.signer) = "sender";
// sender is the address of the sender of this message.
string sender = 1;
// target is the address of the account to be executed.
string target = 2;
- // message is the message to be sent to the account.
- google.protobuf.Any message = 3;
+ // execute_msg_type_url specifies the type URL of the execution message.
+ string execute_msg_type_url = 4;
+ // json_message is the JSON-encoded message to be sent to the account.
+ string json_message = 5;
// funds contains the coins that the account wants to
// send alongside the request.
- repeated cosmos.base.v1beta1.Coin funds = 4
+ repeated cosmos.base.v1beta1.Coin funds = 6
[(gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins", (gogoproto.nullable) = false];
+ // Reserved field numbers to prevent reuse.
+ reserved 3;
}This change assigns new field numbers to the new fields and reserves the old field numbers to prevent accidental reuse.
Also applies to: 61-63, 66-66
| "encoding/json" | ||
| "math" | ||
| "strings" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Organize imports according to Go conventions
The import statements are not properly grouped and ordered. Imports should be grouped into standard library packages, third-party packages, and project-specific packages, each separated by a blank line.
Apply this diff to organize the imports:
import (
"context"
+ "encoding/json"
+ "strings"
"math"
"testing"
"time"
- types "github.com/cosmos/gogoproto/types/any"
- "github.com/stretchr/testify/require"
- "cosmossdk.io/core/store"
- "cosmossdk.io/core/transaction"
- "cosmossdk.io/x/accounts/accountstd"
- v1 "cosmossdk.io/x/accounts/defaults/multisig/v1"
- accountsv1 "cosmossdk.io/x/accounts/v1"
+ "bytes"
+ "fmt"
+ "reflect"
+ types "github.com/cosmos/gogoproto/types/any"
+ "github.com/cosmos/gogoproto/jsonpb"
+ gogoproto "github.com/cosmos/gogoproto/proto"
+ "github.com/stretchr/testify/require"
+ codectypes "github.com/cosmos/cosmos-sdk/codec/types"
+
+ "cosmossdk.io/core/store"
+ "cosmossdk.io/core/transaction"
+ "cosmossdk.io/x/accounts/accountstd"
+ v1 "cosmossdk.io/x/accounts/defaults/multisig/v1"
+ accountsv1 "cosmossdk.io/x/accounts/v1"
)Also applies to: 19-26
🧰 Tools
🪛 golangci-lint (1.62.2)
5-5: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
6-6: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
| @@ -567,7 +577,8 @@ | |||
| 0, []byte("multisig_acc"), []byte("addr1"), TestFunds, | |||
| func(ictx context.Context, sender []byte, msg transaction.Msg) (transaction.Msg, error) { | |||
| if execmsg, ok := msg.(*accountsv1.MsgExecute); ok { | |||
| updateCfg, err := accountstd.UnpackAny[v1.MsgUpdateConfig](execmsg.GetMessage()) | |||
| anyExecmsg, err := encodeJSONToProto(execmsg.ExecuteMsgTypeUrl, execmsg.JsonMessage) | |||
There was a problem hiding this comment.
Handle the error returned by encodeJSONToProto
The error returned by encodeJSONToProto is assigned but not checked, which can lead to unhandled errors and unexpected behavior.
Apply this diff to properly handle the error:
anyExecmsg, err := encodeJSONToProto(execmsg.ExecuteMsgTypeUrl, execmsg.JsonMessage)
+ if err != nil {
+ return nil, err
+ }
updateCfg, err := accountstd.UnpackAny[v1.MsgUpdateConfig](anyExecmsg)
if err != nil {
return nil, err
}📝 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.
| anyExecmsg, err := encodeJSONToProto(execmsg.ExecuteMsgTypeUrl, execmsg.JsonMessage) | |
| anyExecmsg, err := encodeJSONToProto(execmsg.ExecuteMsgTypeUrl, execmsg.JsonMessage) | |
| if err != nil { | |
| return nil, err | |
| } | |
| updateCfg, err := accountstd.UnpackAny[v1.MsgUpdateConfig](anyExecmsg) | |
| if err != nil { | |
| return nil, err | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
580-580: ineffectual assignment to err
(ineffassign)
| impl, ok := k.accounts[initMsg.AccountType] | ||
| if !ok { | ||
| return nil, nil, err | ||
| } |
There was a problem hiding this comment.
Initialize the err variable before returning
In the condition where the account type is not found, the error variable err is not defined before it is returned, leading to a potential nil pointer dereference.
Apply this diff to define and return an appropriate error:
impl, ok := k.accounts[initMsg.AccountType]
if !ok {
- return nil, nil, err
+ return nil, nil, fmt.Errorf("%w: %s", errAccountTypeNotFound, initMsg.AccountType)
}📝 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.
| impl, ok := k.accounts[initMsg.AccountType] | |
| if !ok { | |
| return nil, nil, err | |
| } | |
| impl, ok := k.accounts[initMsg.AccountType] | |
| if !ok { | |
| return nil, nil, fmt.Errorf("%w: %s", errAccountTypeNotFound, initMsg.AccountType) | |
| } |
| func (x *MsgExecute) GetJsonMessage() string { | ||
| if x != nil { | ||
| return x.JsonMessage | ||
| } | ||
| return "" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename 'JsonMessage' to 'JSONMessage' in 'MsgExecute'
Update the field name to JSONMessage to follow Go conventions.
Apply this diff:
-func (x *MsgExecute) GetJsonMessage() string {
+func (x *MsgExecute) GetJSONMessage() string {
if x != nil {
- return x.JsonMessage
+ return x.JSONMessage
}
return ""
}Committable suggestion skipped: line range outside the PR's diff.
| func (x *MsgExecute) GetExecuteMsgTypeUrl() string { | ||
| if x != nil { | ||
| return x.Message | ||
| return x.ExecuteMsgTypeUrl | ||
| } | ||
| return nil | ||
| return "" | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename 'ExecuteMsgTypeUrl' to 'ExecuteMsgTypeURL' in 'MsgExecute'
Align with Go naming conventions by capitalizing "URL" in the method name.
Apply this diff:
-func (x *MsgExecute) GetExecuteMsgTypeUrl() string {
+func (x *MsgExecute) GetExecuteMsgTypeURL() string {
if x != nil {
- return x.ExecuteMsgTypeUrl
+ return x.ExecuteMsgTypeURL
}
return ""
}Committable suggestion skipped: line range outside the PR's diff.
| md_MsgExecute protoreflect.MessageDescriptor | ||
| fd_MsgExecute_sender protoreflect.FieldDescriptor | ||
| fd_MsgExecute_target protoreflect.FieldDescriptor | ||
| fd_MsgExecute_execute_msg_type_url protoreflect.FieldDescriptor | ||
| fd_MsgExecute_json_message protoreflect.FieldDescriptor | ||
| fd_MsgExecute_funds protoreflect.FieldDescriptor |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename 'JsonMessage' and 'ExecuteMsgTypeUrl' to follow Go naming conventions
The fields JsonMessage and ExecuteMsgTypeUrl should be renamed to JSONMessage and ExecuteMsgTypeURL respectively to capitalize initialisms.
Apply this diff:
-fd_MsgExecute_execute_msg_type_url protoreflect.FieldDescriptor
-fd_MsgExecute_json_message protoreflect.FieldDescriptor
+fd_MsgExecute_execute_msg_type_URL protoreflect.FieldDescriptor
+fd_MsgExecute_JSONMessage protoreflect.FieldDescriptor📝 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.
| md_MsgExecute protoreflect.MessageDescriptor | |
| fd_MsgExecute_sender protoreflect.FieldDescriptor | |
| fd_MsgExecute_target protoreflect.FieldDescriptor | |
| fd_MsgExecute_execute_msg_type_url protoreflect.FieldDescriptor | |
| fd_MsgExecute_json_message protoreflect.FieldDescriptor | |
| fd_MsgExecute_funds protoreflect.FieldDescriptor | |
| md_MsgExecute protoreflect.MessageDescriptor | |
| fd_MsgExecute_sender protoreflect.FieldDescriptor | |
| fd_MsgExecute_target protoreflect.FieldDescriptor | |
| fd_MsgExecute_execute_msg_type_URL protoreflect.FieldDescriptor | |
| fd_MsgExecute_JSONMessage protoreflect.FieldDescriptor | |
| fd_MsgExecute_funds protoreflect.FieldDescriptor |
| if x.ExecuteMsgTypeUrl != "" { | ||
| value := protoreflect.ValueOfString(x.ExecuteMsgTypeUrl) | ||
| if !f(fd_MsgExecute_execute_msg_type_url, value) { | ||
| return | ||
| } | ||
| } | ||
| if x.JsonMessage != "" { | ||
| value := protoreflect.ValueOfString(x.JsonMessage) | ||
| if !f(fd_MsgExecute_json_message, value) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update field names to capitalize initialisms in 'MsgExecute'
Consistently capitalize "URL" and "JSON" in field names.
Apply this diff:
-if x.ExecuteMsgTypeUrl != "" {
+if x.ExecuteMsgTypeURL != "" {
...
-if x.JsonMessage != "" {
+if x.JSONMessage != "" {Committable suggestion skipped: line range outside the PR's diff.
| } | ||
| return m | ||
| } | ||
|
|
||
| func makeAccountSchema(impl implementation.Implementation) *SchemaResponse { | ||
| func MakeAccountSchema(impl implementation.Implementation) *SchemaResponse { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation comment for exported function MakeAccountSchema
Since MakeAccountSchema is now exported, it should have a documentation comment to explain its purpose and usage.
Apply this diff to add the documentation comment:
+// MakeAccountSchema generates a SchemaResponse for a given account implementation.
func MakeAccountSchema(impl implementation.Implementation) *SchemaResponse {📝 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.
| func MakeAccountSchema(impl implementation.Implementation) *SchemaResponse { | |
| // MakeAccountSchema generates a SchemaResponse for a given account implementation. | |
| func MakeAccountSchema(impl implementation.Implementation) *SchemaResponse { |
|
Closing this then! Let's go then by the first suggestion in the issue. |
Description
Closes: #21953
This PR focuses a modification to the protobuf definition of
AccountQueryRequest,MsgInit,MsgExecuteand related func in the x/accounts module. The primary goal is to shift the responsibility of encoding messages from the client to the module itself, ensuring that the module handles all message transformations internally. And then enableAutoCLIeasily.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
Bug Fixes
Tests
Documentation