-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(accounts): use gogoproto API instead of protov2. #18653
Conversation
WalkthroughWalkthroughThe changes across the codebase reflect a significant shift from using generic protobuf types to more specific or custom types, such as Changes
TipsChat with CodeRabbit Bot (
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files ignored due to filter (6)
- crypto/keys/secp256k1/keys.pb.go
- x/accounts/interfaces/account_abstraction/v1/interface.pb.go
- x/accounts/testing/counter/v1/counter.pb.go
- x/accounts/testing/rotation/v1/partial.pb.go
- x/accounts/v1/query.pb.go
- x/accounts/v1/tx.pb.go
Files selected for processing (35)
- api/cosmos/accounts/v1/query.pulsar.go (27 hunks)
- api/cosmos/accounts/v1/tx.pulsar.go (51 hunks)
- baseapp/internal/protocompat/protocompat.go (3 hunks)
- codec/unknownproto/unknown_fields.go (4 hunks)
- proto/cosmos/accounts/interfaces/account_abstraction/v1/interface.proto (1 hunks)
- proto/cosmos/accounts/testing/counter/v1/counter.proto (1 hunks)
- proto/cosmos/accounts/testing/rotation/v1/partial.proto (1 hunks)
- proto/cosmos/accounts/v1/query.proto (2 hunks)
- proto/cosmos/accounts/v1/tx.proto (3 hunks)
- simapp/app.go (1 hunks)
- tests/e2e/accounts/account_abstraction_test.go (11 hunks)
- x/accounts/account_test.go (4 hunks)
- x/accounts/accountstd/exports.go (3 hunks)
- x/accounts/cli/cli.go (4 hunks)
- x/accounts/genesis_test.go (2 hunks)
- x/accounts/internal/implementation/account_test.go (2 hunks)
- x/accounts/internal/implementation/api_builder.go (8 hunks)
- x/accounts/internal/implementation/api_builder_test.go (2 hunks)
- x/accounts/internal/implementation/context.go (5 hunks)
- x/accounts/internal/implementation/context_test.go (3 hunks)
- x/accounts/internal/implementation/encoding.go (1 hunks)
- x/accounts/internal/implementation/implementation.go (4 hunks)
- x/accounts/internal/implementation/implementation_test.go (1 hunks)
- x/accounts/internal/implementation/protoaccount.go (2 hunks)
- x/accounts/keeper.go (11 hunks)
- x/accounts/keeper_account_abstraction.go (10 hunks)
- x/accounts/keeper_test.go (5 hunks)
- x/accounts/msg_server.go (6 hunks)
- x/accounts/msg_server_test.go (2 hunks)
- x/accounts/query_server.go (3 hunks)
- x/accounts/query_server_test.go (2 hunks)
- x/accounts/testing/account_abstraction/full.go (1 hunks)
- x/accounts/testing/account_abstraction/minimal.go (1 hunks)
- x/accounts/testing/counter/counter.go (1 hunks)
- x/accounts/utils_test.go (2 hunks)
Files not summarized due to errors (2)
- api/cosmos/accounts/v1/query.pulsar.go: Error: Message exceeds token limit
- api/cosmos/accounts/v1/tx.pulsar.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (3)
- proto/cosmos/accounts/interfaces/account_abstraction/v1/interface.proto
- proto/cosmos/accounts/testing/counter/v1/counter.proto
- proto/cosmos/accounts/testing/rotation/v1/partial.proto
Additional comments: 146
api/cosmos/accounts/v1/query.pulsar.go (8)
3424-3427: The
AccountQueryRequest
message type correctly defines atarget
field of typestring
and arequest
field of typegoogle.protobuf.Any
. This allows for a flexible request that can contain any type of message.3468-3471: The
AccountQueryResponse
message type correctly defines aresponse
field of typegoogle.protobuf.Any
. This allows for a flexible response that can contain any type of message.3718-3728: The
SchemaRequest
message type correctly defines anaccount_type
field of typestring
. This field is intended to specify the account type for which the schema is being requested.3736-3753: The
SchemaResponse
message type correctly definesinit_schema
,execute_handlers
, andquery_handlers
fields, each designed to hold schema descriptors for different account methods. The use of repeated fields forexecute_handlers
andquery_handlers
allows for multiple handlers to be defined.3753-3756: The
SchemaResponse_Handler
message type correctly definesrequest
andresponse
fields of typestring
. These fields are intended to hold the names that can be used to look up the reflection descriptor.3757-3760: The
AccountTypeRequest
message type correctly defines anaddress
field of typestring
. This field is intended to specify the address for which the account type is being requested.3761-3764: The
AccountTypeResponse
message type correctly defines anaccount_type
field of typestring
. This field is intended to specify the account type associated with the address provided in the request.3764-3794: The service definition
Query
correctly defines RPC methodsAccountQuery
,Schema
, andAccountType
, each with appropriate request and response types. These methods correspond to the message types defined above and provide the necessary functionality for querying account information.api/cosmos/accounts/v1/tx.pulsar.go (6)
3149-3154: The
MsgInit
message correctly defines thesender
,account_type
, andmessage
fields with appropriate protobuf field tags.3203-3206: The
MsgInitResponse
message correctly defines theaccount_address
andresponse
fields with appropriate protobuf field tags.3250-3254: The
MsgExecute
message correctly defines thesender
,target
, andmessage
fields with appropriate protobuf field tags.3304-3305: The
MsgExecuteResponse
message correctly defines theresponse
field with appropriate protobuf field tags.3250-3254: The
MsgExecuteBundle
message correctly defines thebundler
andoperations
fields with appropriate protobuf field tags.3203-3206: The
MsgExecuteBundleResponse
message correctly defines theresponses
field with appropriate protobuf field tags.baseapp/internal/protocompat/protocompat.go (4)
9-9: The summary states that
gogoproto.Merge
is replaced withproto.Merge
from thegithub.com/golang/protobuf/proto
package, which is not part of theprotov2
API. Please verify that this aligns with the PR objective to standardize onprotov2
APIs.129-130: The comments explain that
gogoproto.Merge
does not work consistently, which is whyproto.Merge
is used. This context is important for understanding the rationale behind the changes.126-143: The
makeGogoHybridHandler
function is designed to handlegogoproto
messages, which may be necessary for backward compatibility or gradual migration toprotov2
. Ensure that this is in line with the overall migration strategy.6-12: The import of
github.com/golang/protobuf/proto
is marked with anolint
comment to bypass static analysis checks. This is necessary becausegogoproto.Merge
does not work consistently, andproto.Merge
is used as a workaround. Ensure that this exception to linting rules is acceptable within the project's coding standards.codec/unknownproto/unknown_fields.go (3)
44-50: The summary states that the function
RejectUnknownFields
has been modified to accept only two parameters, but the hunk shows that it still accepts four parameters. This discrepancy should be clarified.341-350: The summary states that the function
extractFileDescMessageDesc
now takes a single parameter, but the hunk shows that it still takes a parameter of typeproto.Message
and returns two additional types. This discrepancy should be clarified.390-397: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [390-409]
The function
getDescriptorInfo
correctly matches the summary, accepting a single parameter of typeproto.Message
.proto/cosmos/accounts/v1/query.proto (4)
5-5: The
go_package
option has been updated to reflect the new package path for the Go code generated from the.proto
files.7-7: The import statement for "google/protobuf/any.proto" has been added, which is necessary for using the
google.protobuf.Any
type in the proto messages.24-24: The
request
field inAccountQueryRequest
has been correctly changed frombytes
togoogle.protobuf.Any
, allowing for more structured data to be used in the request.30-30: The
response
field inAccountQueryResponse
has been correctly changed frombytes
togoogle.protobuf.Any
, which is consistent with the change in the request and allows for more structured data in the response.proto/cosmos/accounts/v1/tx.proto (5)
4-9: The addition of
google/protobuf/any.proto
import and the update of the Go package path are consistent with the PR's objective to standardize the usage ofproto.Merge
and to use more structured message types.31-35: The replacement of
bytes
fields withgoogle.protobuf.Any
in theMsgInit
message definition aligns with the PR's objective to improve message structure and encoding/decoding logic.39-43: The replacement of
bytes
fields withgoogle.protobuf.Any
in theMsgInitResponse
message definition aligns with the PR's objective to improve message structure and encoding/decoding logic.50-54: The replacement of
bytes
fields withgoogle.protobuf.Any
in theMsgExecute
message definition aligns with the PR's objective to improve message structure and encoding/decoding logic.58-60: The replacement of
bytes
fields withgoogle.protobuf.Any
in theMsgExecuteResponse
message definition aligns with the PR's objective to improve message structure and encoding/decoding logic.simapp/app.go (1)
- 287-293: Please ensure that the PR checklist items, such as adding a changelog entry, updating documentation, and confirming CI checks, are completed before finalizing the PR.
tests/e2e/accounts/account_abstraction_test.go (5)
6-22: The changes in import paths align with the PR objectives to transition from
gogoproto
toprotov2
APIs and reflect a reorganization of the codebase.318-326: The
intoAny
function signature has been correctly updated to usecodectypes.Any
instead ofanypb.Any
, aligning with the PR objectives.328-332: The
coins
function signature has been correctly updated to returnsdk.Coins
instead ofv1beta1.Coin
, aligning with the PR objectives.70-74: The usage of
intoAny
function in the test cases is consistent with the updated function signature and the PR objectives.Also applies to: 76-79, 103-107, 109-112, 130-134, 136-139, 159-163, 165-168, 189-193, 195-198, 218-222, 224-227, 247-251, 253-256, 273-276, 300-305, 307-310
- 73-73: The usage of
coins
function in the test cases is consistent with the updated function signature and the PR objectives.Also applies to: 78-78, 106-106, 111-111, 133-133, 138-138, 192-192, 197-197, 221-221, 226-226, 250-250, 255-255, 276-276, 304-304, 309-309
x/accounts/account_test.go (7)
4-10: The import changes from
google.golang.org/protobuf
togithub.com/cosmos/gogoproto
types are consistent with the PR's objective to standardize the usage ofproto.Merge
for message merging.29-35: The usage of
types.Empty
fromgithub.com/cosmos/gogoproto
instead ofemptypb.Empty
is in line with the PR's goal to replacegogoproto
API withprotov2
API.44-50: The replacement of
wrapperspb.StringValue
withtypes.StringValue
and the subsequent usage oftypes.UInt64Value
is consistent with the PR's objective.26-57: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [54-72]
The replacement of
wrapperspb.Int64Value
withtypes.Int64Value
and the use oftypes.Empty
in the error handling logic align with the PR's objective.
86-88: The replacement of
wrapperspb.UInt64Value
withtypes.UInt64Value
andwrapperspb.StringValue
withtypes.StringValue
is consistent with the PR's objective.92-95: The replacement of
wrapperspb.StringValue
withtypes.StringValue
and the subsequent usage oftypes.Int64Value
is consistent with the PR's objective.110-116: The use of
types.DoubleValue
andtypes.UInt64Value
is consistent with the PR's objective to replacegogoproto
API withprotov2
API.x/accounts/accountstd/exports.go (4)
6-11: The removal of
gogoproto
imports and the transition toprotov2
APIs is not directly visible in the provided hunks, but the changes in function signatures fromimplementation.ProtoMsg[Type]
toimplementation.ProtoMsgG[Type]
are consistent with the PR's objective to standardize protobuf message handling.32-63: The changes in function signatures to use
implementation.ProtoMsgG[Type]
instead ofimplementation.ProtoMsg[Type]
and the modification of theAddAccount
function's return type are in line with the PR's objective to improve protobuf message handling. Ensure that all calls to these functions are updated to match the new signatures.78-102: The changes in function signatures for
ExecModule
,QueryModule
,UnpackAny
, andPackAny
to useimplementation.ProtoMsgG
andimplementation.Any
instead ofproto.Message
andanypb.Any
respectively, are consistent with the PR's objective. Ensure that all calls to these functions are updated to match the new signatures.104-111: The
ExecModuleAnys
function signature change to useimplementation.Any
instead ofanypb.Any
is consistent with the PR's objective. Ensure that all calls to this function are updated to match the new signature.x/accounts/cli/cli.go (4)
3-16: The import section has been updated to reflect the transition from
gogoproto
toprotov2
API, which aligns with the PR objectives.138-141: The
GetQueryAccountCmd
function now usesclientCtx.PrintProto(res)
for output, which is a change consistent with the PR objectives to standardize protobuf message handling.161-164: The
handlerMsgBytes
function signature has been updated to return*codectypes.Any
, which aligns with the PR objectives to useproto.Merge
for message merging.175-185: The
encodeJSONToProto
function has been updated to usecodectypes
andgogoproto
packages for message handling and encoding, which is in line with the PR objectives.x/accounts/genesis_test.go (2)
15-32: The updates to the
newKeeper
function signature and the usage oftypes.Empty{}
andtypes.UInt64Value{}
from thegithub.com/cosmos/gogoproto/types
package align with the PR's objective to standardize protobuf message handling.43-51: The updates to the
k.Query
calls and the assertions in the test cases are consistent with the PR's objective to transition fromgoogle.golang.org/protobuf/types/known
togithub.com/cosmos/gogoproto/types
.x/accounts/internal/implementation/account_test.go (2)
3-9: The changes in the import statements are consistent with the PR's objective to replace
gogoproto
API withprotov2
API.27-55: The function signatures and type usages in this hunk are updated to use types from
github.com/cosmos/gogoproto/types
, which aligns with the PR's objective and the summary provided.x/accounts/internal/implementation/api_builder.go (8)
22-25: The change from
any
toProtoMsg
in thehandler
function signature enhances type safety and clarity, aligning with the PR's objective to standardize protocol buffer message handling.33-36: The error handling for the absence of an
init
handler is clear and follows Go's idiomatic approach of returning an error when a function cannot proceed due to missing prerequisites.42-45: The initialization of
handlers
andhandlersSchema
maps with the correct types forProtoMsg
is consistent with the changes made in theInitBuilder
and aligns with the PR's goal of improving type safety.52-55: The update to the
handlers
map to useProtoMsg
instead ofany
is consistent with the changes in theInitBuilder
and is a good practice for type safety and clarity.62-68: The error handling for the case where no execution handlers are registered is appropriate, and the use of a default handler that returns
errNoExecuteHandler
is a good way to provide clear feedback to the caller.75-79: The dynamic dispatch logic to find and call the appropriate handler based on the message name is well-implemented, and the error handling for unregistered messages is clear and informative.
87-87: The
QueryBuilder
initialization usingNewExecuteBuilder
suggests that the query and execution handlers share the same API, which is a good design choice for consistency and maintainability.99-101: The delegation of
makeHandler
inQueryBuilder
toExecuteBuilder
'smakeHandler
is a good example of code reuse and helps to maintain consistency between execution and query handling.x/accounts/internal/implementation/api_builder_test.go (1)
- 4-17: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [4-34]
The changes from
wrapperspb.StringValue
totypes.StringValue
in the function signatures and test statements are consistent with the PR's objective to transition fromgogoproto
toprotov2
APIs.x/accounts/internal/implementation/context_test.go (4)
4-11: The import changes are consistent with the PR objectives to transition from
gogoproto
toprotov2
APIs.27-34: The replacement of
NewImplementation
withnewImplementation
and the change in argument type for theExecute
function align with the PR objectives.43-43: The hardcoded byte array in the assertion should be verified to ensure it reflects the expected outcome after the refactoring.
46-77: The changes in argument types for
ExecModule
,ExecModuleUntyped
,QueryModule
, andMerge
functions are consistent with the PR objectives.x/accounts/internal/implementation/encoding.go (6)
- 8-8: The import statement
github.com/cosmos/gogoproto/proto
seems to contradict the PR's objective to replacegogoproto
withprotov2
. Please verify if this is intentional or an oversight.
The import statement
github.com/cosmos/gogoproto/proto
is consistently used across various files in the codebase, indicating that the usage inx/accounts/internal/implementation/encoding.go
is likely intentional and aligned with the current codebase structure.
15-21: The implementation of
FindMessageByName
correctly uses reflection to instantiate message types by name, which aligns with the PR's objective to improve message handling.33-36: The use of generics in
UnpackAny
is a modern Go feature and should be fine as long as the codebase is using Go 1.18 or later.38-40: The use of
proto.Unmarshal
inUnpackAnyTo
is consistent with the PR's objective to standardize onproto
message handling.53-55: The
Merge
function correctly usesproto.Merge
, aligning with the PR's objective to replacegogoproto.Merge
.57-59: The
Equal
function correctly usesproto.Equal
, aligning with the PR's objective to standardize onproto
message handling.x/accounts/internal/implementation/implementation.go (6)
18-18: The signature change for
AccountCreatorFunc
is correct and aligns with the PR objectives to standardize the usage ofproto.Merge
for message merging.47-47: The renaming of
NewImplementation
tonewImplementation
is correct and aligns with the PR objectives.27-41: The changes within
MakeAccountsMap
function, including the use ofnewImplementation
and the variableaccountInterface
, are correct and align with the PR objectives.69-100: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [89-105]
The structural changes to the
Implementation
struct, including the addition of new schema fields, are correct and align with the PR objectives.
73-86: The logic changes in the
newImplementation
function, including the schema building and assignment to theCollectionsSchema
field, are correct and align with the PR objectives.93-97: The parameter changes in the
Init
,Execute
, andQuery
functions of theImplementation
struct to useProtoMsg
are correct and align with the PR objectives.x/accounts/internal/implementation/implementation_test.go (3)
4-11: The import statements have been updated to reflect the transition from
gogoproto
toprotov2
APIs, aligning with the PR objectives.14-14: The function
newImplementation
is correctly called with the additional argumentcollections.NewSchemaBuilderFromAccessor(OpenKVStore)
, which is consistent with the summary.13-58: The test cases are using the updated
types
fromgithub.com/cosmos/gogoproto/types
and seem to be correctly implemented with no apparent issues.x/accounts/internal/implementation/protoaccount.go (7)
11-11: The summary incorrectly states that the
ProtoMsg
interface is replaced withprotoiface.MessageV1
. The code shows thatProtoMsg
is defined as an alias toprotoiface.MessageV1
, not replaced.13-17: The introduction of the generic interface
ProtoMsgG
is correctly reflected in the code.4-29: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [19-30]
The update to the
RegisterInitHandler
function signature to use the new generic interfaceProtoMsgG
is consistent with the summary.
74-84: The
NewProtoMessageSchema
function is updated to handle the new generic interfaceProtoMsgG
, which is consistent with the summary.40-64: The
RegisterExecuteHandler
function is updated to use the new generic interfaceProtoMsgG
. This change is part of the protobuf message handling improvements and is consistent with the overall PR objectives.66-72: The
RegisterQueryHandler
function delegates toRegisterExecuteHandler
, which is updated to use the new generic interfaceProtoMsgG
. This is consistent with the changes made in the PR.74-84: The
NewProtoMessageSchema
function includes error handling that panics if aprotov2
message is used, which aligns with the PR's objective to move away fromgogoproto
and standardize onproto.Merge
.x/accounts/keeper.go (14)
11-12: The import statements for
gogoproto
andproto
are present, which is consistent with the PR's objective to transition fromgogoproto
toprotov2
APIs.39-42: The
QueryRouter
interface has been updated to useimplementation.ProtoMsg
instead ofprotoiface.MessageV1
, which aligns with the PR's objective to standardize message handling.54-54: The
SignerProvider
interface methodGetMsgV1Signers
now usesgogoproto.Message
instead ofproto.Message
. This change should be verified across the codebase to ensure that all implementations of this interface and calls to this method are updated accordingly.
The verification of the
SignerProvider
interface methodGetMsgV1Signers
confirms that the method signature has been updated to usegogoproto.Message
across various files in the codebase. Implementations and calls to this method have been found inx/group/keeper/proposal_executor.go
,x/gov/keeper/proposal.go
,x/authz/keeper/keeper.go
,x/accounts/utils_test.go
,x/accounts/keeper.go
,codec/proto_codec_test.go
,codec/proto_codec.go
, andcodec/codec.go
. TheCHANGELOG.md
also reflects the introduction of new methods includingGetMsgV1Signers
.
60-62: The
InterfaceRegistry
interface is introduced with methods for registering interfaces and implementations, which is a new addition mentioned in the summary.70-76: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [65-75]
The
NewKeeper
function signature has been updated to include anInterfaceRegistry
parameter, which is consistent with the changes described in the summary.
101-101: The
registerToInterfaceRegistry
function is called withinNewKeeper
to register interfaces and implementations, which is a new function mentioned in the summary.135-139: The
Init
method of theKeeper
type has been updated to accept a parameter of typeimplementation.ProtoMsg
instead ofany
, which aligns with the PR's objective to use specific types for message handling.177-181: The
Execute
method of theKeeper
type has been updated to accept a parameter of typeimplementation.ProtoMsg
instead ofany
, which is consistent with the PR's objective.203-207: The
Query
method of theKeeper
type has been updated to accept a parameter of typeimplementation.ProtoMsg
instead ofany
, which is consistent with the PR's objective.261-270: The
makeAccountContext
function has been updated to useimplementation.ProtoMsg
and includes error handling for execution in a query context, which is consistent with the PR's objective to standardize message handling.297-311: The
sendModuleMessageUntyped
function has been updated to useimplementation.ProtoMsg
and includes logic to fetch the response type from the request message type, which is consistent with the PR's objective.326-377: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [317-334]
The
sendModuleMessage
function includes sender verification and message routing logic, which is consistent with the PR's objective to standardize message handling.
337-349: The
queryModule
function includes logic to handle query routing and execution, which is consistent with the PR's objective to standardize message handling.359-377: The
registerToInterfaceRegistry
function registers interfaces and implementations to the global interface registry, which is a new addition mentioned in the summary and aligns with the PR's objective.x/accounts/keeper_account_abstraction.go (10)
5-11: The changes in package imports and type references align with the PR's objective to transition from
gogoproto
toprotov2
APIs.23-31: The update to the
ExecuteUserOperation
function signature to usev1.UserOperation
is consistent with the PR's objective to standardize the usage ofproto.Merge
.64-70: The update to the
Authenticate
function signature to usev1.UserOperation
is consistent with the PR's objective to standardize the usage ofproto.Merge
.80-85: The update to the
authenticate
function signature to usev1.UserOperation
is consistent with the PR's objective to standardize the usage ofproto.Merge
.104-111: The update to the
OpExecuteMessages
function signature to useimplementation.Any
is consistent with the PR's objective to standardize the usage ofproto.Merge
.120-127: The update to the
opExecuteMessages
function signature to useimplementation.Any
is consistent with the PR's objective to standardize the usage ofproto.Merge
.157-164: The update to the
PayBundler
function signature to useimplementation.Any
is consistent with the PR's objective to standardize the usage ofproto.Merge
.173-180: The update to the
payBundler
function signature to useimplementation.Any
is consistent with the PR's objective to standardize the usage ofproto.Merge
.208-213: The update to the
parsePayBundlerResponse
function to useimplementation.Any
is consistent with the PR's objective to standardize the usage ofproto.Merge
.222-227: The update to the
parseExecuteResponse
function to useimplementation.Any
is consistent with the PR's objective to standardize the usage ofproto.Merge
.x/accounts/keeper_test.go (7)
7-15: The import statements have been updated to reflect the transition from
gogoproto
toprotov2
APIs, which is consistent with the PR objectives and summaries.18-23: The
TestKeeper_Init
function correctly uses the newtypes.Empty
type in place ofemptypb.Empty
, aligning with the PR's goal to standardize protobuf message handling.28-34: The
ok
test case withinTestKeeper_Init
correctly usestypes.Empty
in line with the PR's objective to replaceemptypb.Empty
.53-65: The
ok
test case withinTestKeeper_Execute
correctly usestypes.Empty
, which is consistent with the PR's objective to replaceemptypb.Empty
.94-108: The
ok
test case withinTestKeeper_Query
correctly usestypes.Empty
, aligning with the PR's objective to replaceemptypb.Empty
.125-136: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [116-134]
The
query module
test case withinTestKeeper_Query
correctly usestypes.StringValue
andtypes.Int64Value
, which is consistent with the PR's objective to replacewrapperspb.String
andwrapperspb.Int64
.
- 128-128: The use of
proto.Merge
in thequery module
test case is consistent with the PR's objective to standardize message merging usingproto.Merge
.x/accounts/msg_server.go (5)
7-12: The addition of the new import statement for
"cosmossdk.io/x/accounts/internal/implementation"
aligns with the PR's objective to replacegogoproto
withprotov2
API usage.31-34: The replacement of
impl.InitHandlerSchema.RequestSchema.TxDecode
withimplementation.UnpackAnyRaw
is consistent with the PR's objective to standardize message handling usingprotov2
APIs.61-68: The replacement of
impl.InitHandlerSchema.ResponseSchema.TxEncode
withimplementation.PackAny
is consistent with the PR's objective to standardize message handling usingprotov2
APIs.84-87: The replacement of
impl.DecodeExecuteRequest
withimplementation.UnpackAnyRaw
is consistent with the PR's objective to standardize message handling usingprotov2
APIs.96-102: The replacement of
impl.EncodeExecuteResponse
withimplementation.PackAny
is consistent with the PR's objective to standardize message handling usingprotov2
APIs.x/accounts/msg_server_test.go (6)
6-16: The summary indicates that the import
"google.golang.org/protobuf/types/known/anypb"
was removed, but this change is not reflected in the provided hunks. If this import is no longer used in the file, the removal is correct. Otherwise, ensure that the import is necessary for the current implementation.6-16: The addition of the import
"cosmossdk.io/x/accounts/internal/implementation"
is consistent with the changes in the code whereimplementation.PackAny
is used.20-20: The change in the type of the
resp
parameter fromproto.Message
toimplementation.ProtoMsg
is consistent with the PR's objective to standardize the usage ofproto.Merge
and related protobuf handling.23-23: The type assertion for
resp
has been correctly updated to(proto.Message)
to match the new parameter typeimplementation.ProtoMsg
.30-30: The replacement of
proto.Marshal
withimplementation.PackAny
for theinitMsg
variable aligns with the PR's objective to transition fromgogoproto
toprotov2
APIs.45-45: The replacement of
proto.Marshal
withimplementation.PackAny
for theexecuteMsg
variable aligns with the PR's objective to transition fromgogoproto
toprotov2
APIs.x/accounts/query_server.go (3)
7-9: The addition of the import path
"cosmossdk.io/x/accounts/internal/implementation"
aligns with the PR's objective to transition fromgogoproto
toprotov2
APIs.28-32: The replacement of
impl.DecodeQueryRequest
withimplementation.UnpackAnyRaw
is consistent with the PR's objective to standardize the usage ofproto.Merge
for message merging.39-47: The replacement of
impl.EncodeQueryResponse
withimplementation.PackAny
is consistent with the PR's objective to standardize the usage ofproto.Merge
for message merging.x/accounts/query_server_test.go (1)
- 4-30: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [4-51]
The changes in the
x/accounts/query_server_test.go
file are consistent with the PR objectives and the AI-generated summaries. The code correctly reflects the transition fromgogoproto
toprotov2
APIs, with appropriate replacements ofproto.Message
withimplementation.ProtoMsg
, and the use ofimplementation.PackAny
andimplementation.UnpackAnyRaw
for message handling. The import ofgithub.com/cosmos/gogoproto/types
and the use oftypes.StringValue
are also in line with the described changes.x/accounts/testing/account_abstraction/full.go (2)
7-8: The change in the import path from
"cosmossdk.io/api/cosmos/accounts/interfaces/account_abstraction/v1"
to"cosmossdk.io/x/accounts/interfaces/account_abstraction/v1"
is consistent with the PR objectives and the summary provided.4-9: No further issues or changes are detected in the provided hunks.
x/accounts/testing/account_abstraction/minimal.go (1)
- 7-11: The changes in import paths are consistent with the PR objectives and the summary provided.
x/accounts/testing/counter/counter.go (1)
- 5-11: The import path for
counterv1
has been updated to reflect the new location of the package. Ensure that all references tocounterv1
in this file and any other files that import this package are updated accordingly.
The updated import path for
counterv1
is correctly used inx/accounts/testing/counter/counter.go
and is also reflected in thego_package
option in the corresponding.proto
file. No further action is required.x/accounts/utils_test.go (6)
7-9: The addition of the
gogoproto
import is consistent with the PR's objective to standardize the usage ofproto.Merge
for message merging.39-45: The introduction of the
interfaceRegistry
type and its methods aligns with the PR's objective to improve protobuf message handling.47-85: The function signature changes for
newKeeper
,mockQuery
,mockSigner
, andmockExec
are consistent with the PR's objective to transition fromgogoproto
toprotov2
APIs.47-52: The summary indicates a change in the
newKeeper
function signature, but the hunk shows no change. Please verify if the summary is accurate or if the hunk is missing the intended changes.57-74: The changes to
mockQuery
andmockSigner
method signatures are consistent with the PR's objective and the summary provided.79-85: The changes to the
mockExec
method signature are consistent with the PR's objective and the summary provided.
runtime.EventService{}, | ||
runtime.BranchService{}, | ||
app.AuthKeeper.AddressCodec(), | ||
appCodec.InterfaceRegistry().SigningContext(), | ||
appCodec, | ||
app.MsgServiceRouter(), | ||
app.GRPCQueryRouter(), | ||
appCodec.InterfaceRegistry(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 NOTE
Codebase verification is a beta feature.
The review of the NewSimApp
function changes confirms that the removal of SigningContext()
and the addition of appCodec.InterfaceRegistry()
is consistent with the PR's objectives. However, the SigningContext
method is used in various other parts of the codebase. It is important to ensure that these references are updated or handled appropriately to avoid breaking changes.
- Ensure that the removal of
SigningContext()
fromappCodec.InterfaceRegistry().SigningContext()
inNewSimApp
does not lead to unresolved references or logic errors in other parts of the codebase whereSigningContext
is used.
Analysis chain
The changes in the NewSimApp
function reflect the PR's objective to standardize the usage of proto.Merge
for message merging. The removal of SigningContext()
and the addition of appCodec.InterfaceRegistry()
align with the PR's goal to replace gogoproto
with protov2
API usage. Ensure that the removal of SigningContext()
does not affect any downstream dependencies that might rely on this method.
@@ -86,7 +84,7 @@ | |||
} | |||
|
|||
// QueryModule can be used by an account to execute a module query. | |||
func QueryModule[Resp any, RespProto ProtoMsg[Resp], Req any, ReqProto ProtoMsg[Req]](ctx context.Context, req ReqProto) (RespProto, error) { | |||
func QueryModule[Resp any, RespProto ProtoMsgG[Resp], Req any, ReqProto ProtoMsgG[Req]](ctx context.Context, req ReqProto) (RespProto, error) { | |||
// we do not need to check the sender in a query because it is not a state transition. | |||
// we also unwrap the original context. | |||
v := ctx.Value(contextKey{}).(contextValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And also here, add a type assertion check for safety.
- v := ctx.Value(contextKey{}).(contextValue)
+ v, ok := ctx.Value(contextKey{}).(contextValue)
+ if !ok {
+ // handle the error, perhaps return an error indicating the context was not set up correctly
+ }
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.
v := ctx.Value(contextKey{}).(contextValue) | |
v, ok := ctx.Value(contextKey{}).(contextValue) | |
if !ok { | |
// handle the error, perhaps return an error indicating the context was not set up correctly | |
} |
// we can just call the handler after making a copy of the message, for safety reasons. | ||
resp, err := method.Handler(handler, ctx, func(msg any) error { | ||
// ref: https://github.com/cosmos/cosmos-sdk/issues/18003 | ||
gogoproto.Merge(msg.(gogoproto.Message), m) | ||
asGogoProto := msg.(gogoproto.Message) | ||
// NOTE: using gogoproto.Merge will fail for some reason unknown to me, but | ||
// using proto.Merge with gogo messages seems to work fine. | ||
proto.Merge(asGogoProto, m) | ||
return nil | ||
}, nil) | ||
if err != nil { | ||
return err | ||
} | ||
// merge on the resp, ref: https://github.com/cosmos/cosmos-sdk/issues/18003 | ||
gogoproto.Merge(outResp.(gogoproto.Message), resp.(gogoproto.Message)) | ||
// NOTE: using gogoproto.Merge will fail for some reason unknown to me, but | ||
// using proto.Merge with gogo messages seems to work fine. | ||
proto.Merge(outResp.(gogoproto.Message), resp.(gogoproto.Message)) | ||
return nil | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for handling message merging in makeGogoHybridHandler
is similar to that in makeProtoV2HybridHandler
. Consider refactoring to reduce duplication if it aligns with the project's coding standards and practices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 87-87: The changelog entry mentions a fix related to
protocompat
which aligns with the PR's objective to replacegogoproto
withprotov2
. However, given the extensive changes described in the PR summary, it would be prudent to verify if there are any API breaking changes that should be documented in the changelog under the "API Breaking Changes" section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we maybe extract this in another PR and backport it to v0.50?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@julienrbrt 100%, I'll do so.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/accounts/cli/cli.go (4 hunks)
Additional comments: 4
x/accounts/cli/cli.go (4)
3-17: The import statements reflect the continued use of
gogoproto
andcodectypes
from the Cosmos SDK, which is inconsistent with the PR objective to transition toprotov2
APIs. Please verify if this is intentional or if further changes are needed to align with the PR's goal.140-143: The change to use
clientCtx.PrintProto(res)
simplifies the output logic and is consistent with the summary provided forx/accounts/cli/cli.go
.163-166: The change in the return type of
handlerMsgBytes
to*codectypes.Any
is consistent with the summary provided forx/accounts/cli/cli.go
and reflects a shift towards usingcodectypes
for message handling.177-187: The implementation of
encodeJSONToProto
continues to usegogoproto
andjsonpb
for JSON to proto conversion, which is consistent with the summary forx/accounts/cli/cli.go
but may contradict the PR objective to transition toprotov2
APIs.
* feat: secp256k1 public key constant time (cosmos#18026) Signed-off-by: bizk <[email protected]> * chore: Fixed changelog duplicated items (cosmos#18628) * adr: Un-Ordered Transaction Inclusion (cosmos#18553) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <[email protected]> * docs: lint ADR-070 (cosmos#18634) * fix(baseapp)!: postHandler should run regardless of result (cosmos#18627) * docs: fix typos in adr-007-specialization-groups.md (cosmos#18635) * chore: alphabetize labels (cosmos#18640) * docs(x/circuit): add note on ante handler (cosmos#18637) Co-authored-by: Aleksandr Bezobchuk <[email protected]> * fix: telemetry metric label variable (cosmos#18643) * chore: typos fix (cosmos#18642) * refactor(store/v2): updates from integration (cosmos#18633) * build(deps): Bump actions/setup-go from 4 to 5 (cosmos#18647) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * feat(store/v2): snapshot manager (cosmos#18458) * chore(client/v2): fix typos in the README.md (cosmos#18657) * fix(baseapp): protocompat.go gogoproto.Merge does not work with custom types (cosmos#18654) Co-authored-by: unknown unknown <unknown@unknown> * chore: fix several minor typos (cosmos#18660) * chore(tools/confix/cmd): fix typo in view.go (cosmos#18659) * refactor(x/staking): check duplicate addresses in StakeAuthorization's params (cosmos#18655) * feat(accounts): use gogoproto API instead of protov2. (cosmos#18653) Co-authored-by: unknown unknown <unknown@unknown> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(store/commitment/iavl): honor tree.Remove error firstly (cosmos#18651) * build(deps): Bump actions/stale from 8 to 9 (cosmos#18656) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(docs): fix typos & wording in docs (cosmos#18667) * chore: fix several typos. (cosmos#18666) * feat(telemetry): enable `statsd` and `dogstatsd` telemetry sinks (cosmos#18646) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: marbar3778 <[email protected]> Co-authored-by: Marko <[email protected]> * feat(store/v2): add SetInitialVersion in SC (cosmos#18665) * feat(client/keys): support display discreetly for `keys add` (cosmos#18663) Co-authored-by: Julien Robert <[email protected]> * ci: add misspell action (cosmos#18671) * chore: typos fix by misspell-fixer (cosmos#18683) Co-authored-by: github-merge-queue <[email protected]> Co-authored-by: Julien Robert <[email protected]> * chore: add v0.50.2 changelog to main (cosmos#18682) * build(deps): Bump github.com/jhump/protoreflect from 1.15.3 to 1.15.4 in /tests (cosmos#18678) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * refactor(bank): remove .String() calls (cosmos#18175) Co-authored-by: Facundo <[email protected]> * ci: use codespell instead of misspell-fixer (cosmos#18686) Co-authored-by: Marko <[email protected]> * feat(gov): add proposal types and spam votes (cosmos#18532) * feat(accounts): use account number as state prefix for account state (cosmos#18664) Co-authored-by: unknown unknown <unknown@unknown> * chore: typos fixes by cosmos-sdk bot (cosmos#18689) Co-authored-by: github-merge-queue <[email protected]> Co-authored-by: Julien Robert <[email protected]> Co-authored-by: marbar3778 <[email protected]> * feat(client/keys): support display discreetly for keys mnemonic (cosmos#18688) * refactor: remove panic usage in keeper methods (cosmos#18636) * ci: rename pr name in misspell job (cosmos#18693) Co-authored-by: Marko <[email protected]> * build(deps): Bump github.com/pelletier/go-toml/v2 from 2.1.0 to 2.1.1 in /tools/confix (cosmos#18702) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * feat(client/keys): support display discreetly for keys export (cosmos#18684) * feat(x/gov): better gov genesis validation (cosmos#18707) --------- Signed-off-by: bizk <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Carlos Santiago Yanzon <[email protected]> Co-authored-by: yihuang <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Aleksandr Bezobchuk <[email protected]> Co-authored-by: Facundo Medica <[email protected]> Co-authored-by: Akaonetwo <[email protected]> Co-authored-by: Marko <[email protected]> Co-authored-by: Julien Robert <[email protected]> Co-authored-by: dreamweaverxyz <[email protected]> Co-authored-by: Pioua <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: cool-developer <[email protected]> Co-authored-by: leonarddt05 <[email protected]> Co-authored-by: testinginprod <[email protected]> Co-authored-by: unknown unknown <unknown@unknown> Co-authored-by: Sukey <[email protected]> Co-authored-by: axie <[email protected]> Co-authored-by: Luke Ma <[email protected]> Co-authored-by: Emmanuel T Odeke <[email protected]> Co-authored-by: 0xn4de <[email protected]> Co-authored-by: hattizai <[email protected]> Co-authored-by: Devon Bear <[email protected]> Co-authored-by: Marko <[email protected]> Co-authored-by: Halimao <[email protected]> Co-authored-by: Cosmos SDK <[email protected]> Co-authored-by: github-merge-queue <[email protected]> Co-authored-by: Facundo <[email protected]> Co-authored-by: Likhita Polavarapu <[email protected]>
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
in the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit