Skip to content
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

Merged
merged 20 commits into from
Dec 8, 2023
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (simulation) [#17911](https://github.com/cosmos/cosmos-sdk/pull/17911) Fix all problems with executing command `make test-sim-custom-genesis-fast` for simulation test.
* (simulation) [#18196](https://github.com/cosmos/cosmos-sdk/pull/18196) Fix the problem of `validator set is empty after InitGenesis` in simulation test.
* (baseapp) [#18551](https://github.com/cosmos/cosmos-sdk/pull/18551) Fix SelectTxForProposal the calculation method of tx bytes size is inconsistent with CometBFT
* (baseapp) [#18653](https://github.com/cosmos/cosmos-sdk/pull/18653) Fix protocompat hybrid handler merging on gogoproto handlers for messages with custom types.

### API Breaking Changes

Expand Down
278 changes: 156 additions & 122 deletions api/cosmos/accounts/v1/query.pulsar.go

Large diffs are not rendered by default.

412 changes: 238 additions & 174 deletions api/cosmos/accounts/v1/tx.pulsar.go

Large diffs are not rendered by default.

18 changes: 14 additions & 4 deletions baseapp/internal/protocompat/protocompat.go
Copy link
Member

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?

Copy link
Contributor Author

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.

Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"reflect"

gogoproto "github.com/cosmos/gogoproto/proto"
"github.com/golang/protobuf/proto" // nolint: staticcheck // needed because gogoproto.Merge does not work consistently. See NOTE: comments.
"google.golang.org/grpc"
proto2 "google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protoreflect"
Expand Down Expand Up @@ -125,14 +126,18 @@ func makeGogoHybridHandler(prefMethod protoreflect.MethodDescriptor, cdc codec.B
}
resp, err := method.Handler(handler, ctx, func(msg any) error {
// merge! ref: https://github.com/cosmos/cosmos-sdk/issues/18003
gogoproto.Merge(msg.(gogoproto.Message), inReq)
// 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(msg.(gogoproto.Message), inReq)
return nil
}, nil)
if err != nil {
return err
}
// merge 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
}, nil
}
Expand Down Expand Up @@ -165,14 +170,19 @@ func makeGogoHybridHandler(prefMethod protoreflect.MethodDescriptor, cdc codec.B
// 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:
Copy link
Contributor

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.

panic("unreachable")
Expand Down
18 changes: 9 additions & 9 deletions codec/unknownproto/unknown_fields.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,7 @@ func RejectUnknownFields(bz []byte, msg proto.Message, allowUnknownNonCriticals
return hasUnknownNonCriticals, nil
}

desc, ok := msg.(descriptorIface)
if !ok {
return hasUnknownNonCriticals, fmt.Errorf("%T does not have a Descriptor() method", msg)
}

fieldDescProtoFromTagNum, _, err := getDescriptorInfo(desc, msg)
fieldDescProtoFromTagNum, _, err := getDescriptorInfo(msg)
if err != nil {
return hasUnknownNonCriticals, err
}
Expand Down Expand Up @@ -345,7 +340,11 @@ func unnestDesc(mdescs []*descriptorpb.DescriptorProto, indices []int) *descript

// Invoking descriptorpb.ForMessage(proto.Message.(Descriptor).Descriptor()) is incredibly slow
// for every single message, thus the need for a hand-rolled custom version that's performant and cacheable.
func extractFileDescMessageDesc(desc descriptorIface) (*descriptorpb.FileDescriptorProto, *descriptorpb.DescriptorProto, error) {
func extractFileDescMessageDesc(msg proto.Message) (*descriptorpb.FileDescriptorProto, *descriptorpb.DescriptorProto, error) {
desc, ok := msg.(descriptorIface)
if !ok {
return nil, nil, fmt.Errorf("%T does not have a Descriptor() method", msg)
}
gzippedPb, indices := desc.Descriptor()

protoFileToDescMu.RLock()
Expand Down Expand Up @@ -391,7 +390,8 @@ var (
)

// getDescriptorInfo retrieves the mapping of field numbers to their respective field descriptors.
func getDescriptorInfo(desc descriptorIface, msg proto.Message) (map[int32]*descriptorpb.FieldDescriptorProto, *descriptorpb.DescriptorProto, error) {
func getDescriptorInfo(msg proto.Message) (map[int32]*descriptorpb.FieldDescriptorProto, *descriptorpb.DescriptorProto, error) {
// we immediately check if the desc is present in the desc
key := reflect.ValueOf(msg).Type()

descprotoCacheMu.RLock()
Expand All @@ -403,7 +403,7 @@ func getDescriptorInfo(desc descriptorIface, msg proto.Message) (map[int32]*desc
}

// Now compute and cache the index.
_, md, err := extractFileDescMessageDesc(desc)
_, md, err := extractFileDescMessageDesc(msg)
if err != nil {
return nil, nil, err
}
Expand Down
8 changes: 3 additions & 5 deletions crypto/keys/secp256k1/keys.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
import "google/protobuf/any.proto";
import "cosmos/accounts/v1/account_abstraction.proto";

option go_package = "cosmossdk.io/x/accounts/interfaces/account_abstraction/v1";

Check failure on line 8 in proto/cosmos/accounts/interfaces/account_abstraction/v1/interface.proto

View workflow job for this annotation

GitHub Actions / break-check

File option "go_package" changed from "" to "cosmossdk.io/x/accounts/interfaces/account_abstraction/v1".

// MsgAuthenticate is a message that an x/account account abstraction implementer
// must handle to authenticate a state transition.
message MsgAuthenticate {
Expand Down
2 changes: 2 additions & 0 deletions proto/cosmos/accounts/testing/counter/v1/counter.proto
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

package cosmos.accounts.testing.counter.v1;

option go_package = "cosmossdk.io/x/accounts/testing/counter/v1";

Check failure on line 5 in proto/cosmos/accounts/testing/counter/v1/counter.proto

View workflow job for this annotation

GitHub Actions / break-check

File option "go_package" changed from "" to "cosmossdk.io/x/accounts/testing/counter/v1".

// MsgInit defines a message which initializes the counter with a given amount.
message MsgInit {
// initial_value is the initial amount to set the counter to.
Expand Down
2 changes: 2 additions & 0 deletions proto/cosmos/accounts/testing/rotation/v1/partial.proto
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

package cosmos.accounts.testing.rotation.v1;

option go_package = "cosmossdk.io/x/accounts/testing/rotation/v1";

Check failure on line 5 in proto/cosmos/accounts/testing/rotation/v1/partial.proto

View workflow job for this annotation

GitHub Actions / break-check

File option "go_package" changed from "" to "cosmossdk.io/x/accounts/testing/rotation/v1".

// MsgInit is the init message used to create a new account
// abstraction implementation that we use for testing, this account
// also allows for rotating the public key.
Expand Down
6 changes: 4 additions & 2 deletions proto/cosmos/accounts/v1/query.proto
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

option go_package = "cosmossdk.io/x/accounts/v1";

import "google/protobuf/any.proto";

// Query defines the Query service for the x/accounts module.
service Query {
// AccountQuery runs an account query.
Expand All @@ -19,13 +21,13 @@
// target defines the account to be queried.
string target = 1;
// request defines the query message being sent to the account.
bytes request = 2;
google.protobuf.Any request = 2;

Check failure on line 24 in proto/cosmos/accounts/v1/query.proto

View workflow job for this annotation

GitHub Actions / break-check

Field "2" on message "AccountQueryRequest" changed type from "bytes" to "message".
}

// AccountQueryResponse is the response type for the Query/AccountQuery RPC method.
message AccountQueryResponse {
// response defines the query response of the account.
bytes response = 1;
google.protobuf.Any response = 1;

Check failure on line 30 in proto/cosmos/accounts/v1/query.proto

View workflow job for this annotation

GitHub Actions / break-check

Field "1" on message "AccountQueryResponse" changed type from "bytes" to "message".
}

// SchemaResponse is the response type for the Query/Schema RPC method.
Expand Down
15 changes: 7 additions & 8 deletions proto/cosmos/accounts/v1/tx.proto
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

option go_package = "cosmossdk.io/x/accounts/v1";

import "google/protobuf/any.proto";
import "cosmos/msg/v1/msg.proto";
import "cosmos/accounts/v1/account_abstraction.proto";

Expand All @@ -30,18 +31,16 @@
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, it's up to the account
// implementation to decide what encoding format should be used to interpret
// this message.
bytes message = 3;
// message is the message to be sent to the account.
google.protobuf.Any message = 3;

Check failure on line 35 in proto/cosmos/accounts/v1/tx.proto

View workflow job for this annotation

GitHub Actions / break-check

Field "3" on message "MsgInit" changed type from "bytes" to "message".
}

// MsgInitResponse defines the Create response type for the Msg/Create RPC method.
message MsgInitResponse {
// account_address is the address of the newly created account.
string account_address = 1;
// response is the response returned by the account implementation.
bytes response = 2;
google.protobuf.Any response = 2;

Check failure on line 43 in proto/cosmos/accounts/v1/tx.proto

View workflow job for this annotation

GitHub Actions / break-check

Field "2" on message "MsgInitResponse" changed type from "bytes" to "message".
}

// MsgExecute defines the Execute request type for the Msg/Execute RPC method.
Expand All @@ -51,14 +50,14 @@
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, it's up to the account
bytes message = 3;
// message is the message to be sent to the account.
google.protobuf.Any message = 3;

Check failure on line 54 in proto/cosmos/accounts/v1/tx.proto

View workflow job for this annotation

GitHub Actions / break-check

Field "3" on message "MsgExecute" changed type from "bytes" to "message".
}

// MsgExecuteResponse defines the Execute response type for the Msg/Execute RPC method.
message MsgExecuteResponse {
// response is the response returned by the account implementation.
bytes response = 1;
google.protobuf.Any response = 1;

Check failure on line 60 in proto/cosmos/accounts/v1/tx.proto

View workflow job for this annotation

GitHub Actions / break-check

Field "1" on message "MsgExecuteResponse" changed type from "bytes" to "message".
}

// -------- Account Abstraction ---------
Expand Down
3 changes: 2 additions & 1 deletion simapp/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -287,9 +287,10 @@ func NewSimApp(
runtime.EventService{},
runtime.BranchService{},
app.AuthKeeper.AddressCodec(),
appCodec.InterfaceRegistry().SigningContext(),
appCodec,
app.MsgServiceRouter(),
app.GRPCQueryRouter(),
appCodec.InterfaceRegistry(),
Comment on lines 287 to +293
Copy link
Contributor

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() from appCodec.InterfaceRegistry().SigningContext() in NewSimApp does not lead to unresolved references or logic errors in other parts of the codebase where SigningContext 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.

accountstd.AddAccount("counter", counter.NewAccount),
accountstd.AddAccount("aa_minimal", account_abstraction.NewMinimalAbstractedAccount),
accountstd.AddAccount("aa_full", account_abstraction.NewFullAbstractedAccount),
Expand Down
Loading
Loading