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.

Sign up for GitHub

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

feat(accounts): use gogoproto API instead of protov2. #18653

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
381c68c
RejectUknownFields is agnostic to how the descriptor is fetched
Dec 1, 2023
06abade
move description extraction to a later stage
Dec 1, 2023
4cba9cb
cleanups
Dec 4, 2023
30aed7a
add protov2 any unknown proto support
Dec 5, 2023
142a23c
Merge branch 'main' into tip/codec/unknown_any
Dec 5, 2023
5a02492
use protobuf.Any in x/accounts instead of golang any
Dec 5, 2023
6561acc
tmp commit praying to god
Dec 6, 2023
a575014
continue fixing with very much pain
Dec 6, 2023
f60f005
close to finish fixing all
Dec 6, 2023
eb78e39
fix all
Dec 7, 2023
262bc0b
revert interface registry changes
Dec 7, 2023
47aaba7
cleanup AddAccount logic and make it more lean
Dec 7, 2023
2b6a2f3
add interface registry
Dec 7, 2023
c8f9692
revert unknown proto changes
Dec 7, 2023
1b9d735
make linter happy
Dec 7, 2023
ce7b4e0
cli fixes
Dec 7, 2023
4054de8
CHANGELOG.md
Dec 7, 2023
58a3b20
lint gods are happy
Dec 7, 2023
6de1484
Update x/accounts/account_test.go
testinginprod Dec 7, 2023
a70d3fe
Merge branch 'main' into tip/accounts/use_any
testinginprod Dec 8, 2023
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
278 changes: 156 additions & 122 deletions api/cosmos/accounts/v1/query.pulsar.go
View file
Open in desktop

Large diffs are not rendered by default.

412 changes: 238 additions & 174 deletions api/cosmos/accounts/v1/tx.pulsar.go
View file
Open in desktop

Large diffs are not rendered by default.

18 changes: 14 additions & 4 deletions baseapp/internal/protocompat/protocompat.go
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop

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

2 changes: 2 additions & 0 deletions proto/cosmos/accounts/interfaces/account_abstraction/v1/interface.proto
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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