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 all 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
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +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/18654) Fixes an issue in which gogoproto.Merge does not work with gogoproto messages with custom types.
* (baseapp) [#18654](https://github.com/cosmos/cosmos-sdk/pull/18654) Fixes an issue in which gogoproto.Merge does not work with gogoproto messages with custom types.

### API Breaking Changes

Expand Down
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: 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
64 changes: 27 additions & 37 deletions tests/e2e/accounts/account_abstraction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,19 @@ import (
"context"
"testing"

rotationv1 "cosmossdk.io/api/cosmos/accounts/testing/rotation/v1"
accountsv1 "cosmossdk.io/api/cosmos/accounts/v1"
bankv1beta1 "cosmossdk.io/api/cosmos/bank/v1beta1"
v1beta1 "cosmossdk.io/api/cosmos/base/v1beta1"
nftv1beta1 "cosmossdk.io/api/cosmos/nft/v1beta1"
stakingv1beta1 "cosmossdk.io/api/cosmos/staking/v1beta1"
"cosmossdk.io/simapp"
"cosmossdk.io/x/accounts"
rotationv1 "cosmossdk.io/x/accounts/testing/rotation/v1"
accountsv1 "cosmossdk.io/x/accounts/v1"
"cosmossdk.io/x/bank/testutil"
banktypes "cosmossdk.io/x/bank/types"
"cosmossdk.io/x/nft"
"github.com/cosmos/cosmos-proto/anyutil"
stakingtypes "cosmossdk.io/x/staking/types"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
sdk "github.com/cosmos/cosmos-sdk/types"
gogoproto "github.com/cosmos/gogoproto/proto"
"github.com/stretchr/testify/require"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/types/known/anypb"
)

var (
Expand Down Expand Up @@ -70,13 +67,13 @@ func TestAccountAbstraction(t *testing.T) {
AuthenticationMethod: "secp256k1",
AuthenticationData: []byte("signature"),
AuthenticationGasLimit: 10000,
BundlerPaymentMessages: intoAny(t, &bankv1beta1.MsgSend{
BundlerPaymentMessages: intoAny(t, &banktypes.MsgSend{
FromAddress: aaAddrStr,
ToAddress: bundlerAddrStr,
Amount: coins(t, "1stake"), // the sender is the AA, so it has the coins and wants to pay the bundler for the gas
}),
BundlerPaymentGasLimit: 50000,
ExecutionMessages: intoAny(t, &bankv1beta1.MsgSend{
ExecutionMessages: intoAny(t, &banktypes.MsgSend{
FromAddress: aaAddrStr,
ToAddress: aliceAddrStr,
Amount: coins(t, "2000stake"), // as the real action the sender wants to send coins to alice
Expand All @@ -103,13 +100,13 @@ func TestAccountAbstraction(t *testing.T) {
AuthenticationMethod: "secp256k1",
AuthenticationData: []byte("signature"),
AuthenticationGasLimit: 10000,
BundlerPaymentMessages: intoAny(t, &bankv1beta1.MsgSend{
BundlerPaymentMessages: intoAny(t, &banktypes.MsgSend{
FromAddress: bundlerAddrStr, // abstracted account tries to send money from bundler to itself.
ToAddress: aaAddrStr,
Amount: coins(t, "1stake"),
}),
BundlerPaymentGasLimit: 50000,
ExecutionMessages: intoAny(t, &bankv1beta1.MsgSend{
ExecutionMessages: intoAny(t, &banktypes.MsgSend{
FromAddress: aaAddrStr,
ToAddress: aliceAddrStr,
Amount: coins(t, "2000stake"), // as the real action the sender wants to send coins to alice
Expand All @@ -130,13 +127,13 @@ func TestAccountAbstraction(t *testing.T) {
AuthenticationMethod: "secp256k1",
AuthenticationData: []byte("signature"),
AuthenticationGasLimit: 10000,
BundlerPaymentMessages: intoAny(t, &bankv1beta1.MsgSend{
BundlerPaymentMessages: intoAny(t, &banktypes.MsgSend{
FromAddress: aaAddrStr,
ToAddress: bundlerAddrStr,
Amount: coins(t, "1stake"),
}),
BundlerPaymentGasLimit: 50000,
ExecutionMessages: intoAny(t, &bankv1beta1.MsgSend{
ExecutionMessages: intoAny(t, &banktypes.MsgSend{
FromAddress: aliceAddrStr, // abstracted account attempts to send money from alice to itself
ToAddress: aaAddrStr,
Amount: coins(t, "2000stake"),
Expand All @@ -159,13 +156,13 @@ func TestAccountAbstraction(t *testing.T) {
AuthenticationMethod: "invalid",
AuthenticationData: []byte("signature"),
AuthenticationGasLimit: 10000,
BundlerPaymentMessages: intoAny(t, &bankv1beta1.MsgSend{
BundlerPaymentMessages: intoAny(t, &banktypes.MsgSend{
FromAddress: aaAddrStr,
ToAddress: bundlerAddrStr,
Amount: coins(t, "1stake"),
}),
BundlerPaymentGasLimit: 50000,
ExecutionMessages: intoAny(t, &bankv1beta1.MsgSend{
ExecutionMessages: intoAny(t, &banktypes.MsgSend{
FromAddress: aliceAddrStr, // abstracted account attempts to send money from alice to itself
ToAddress: aaAddrStr,
Amount: coins(t, "2000stake"),
Expand All @@ -189,13 +186,13 @@ func TestAccountAbstraction(t *testing.T) {
AuthenticationMethod: "secp256k1",
AuthenticationData: []byte("signature"),
AuthenticationGasLimit: 10000,
BundlerPaymentMessages: intoAny(t, &bankv1beta1.MsgSend{
BundlerPaymentMessages: intoAny(t, &banktypes.MsgSend{
FromAddress: aaAddrStr,
ToAddress: bundlerAddrStr,
Amount: coins(t, "1atom"), // abstracted account does not have enough money to pay the bundler, since it does not hold atom
}),
BundlerPaymentGasLimit: 50000,
ExecutionMessages: intoAny(t, &bankv1beta1.MsgSend{
ExecutionMessages: intoAny(t, &banktypes.MsgSend{
FromAddress: aliceAddrStr, // abstracted account attempts to send money from alice to itself
ToAddress: aaAddrStr,
Amount: coins(t, "2000stake"),
Expand All @@ -218,13 +215,13 @@ func TestAccountAbstraction(t *testing.T) {
AuthenticationMethod: "secp256k1",
AuthenticationData: []byte("signature"),
AuthenticationGasLimit: 10000,
BundlerPaymentMessages: intoAny(t, &bankv1beta1.MsgSend{
BundlerPaymentMessages: intoAny(t, &banktypes.MsgSend{
FromAddress: aaAddrStr,
ToAddress: bundlerAddrStr,
Amount: coins(t, "1stake"),
}),
BundlerPaymentGasLimit: 50000,
ExecutionMessages: intoAny(t, &bankv1beta1.MsgSend{
ExecutionMessages: intoAny(t, &banktypes.MsgSend{
FromAddress: aaAddrStr,
ToAddress: aliceAddrStr,
Amount: coins(t, "2000atom"), // abstracted account does not have enough money to pay alice, since it does not hold atom
Expand All @@ -247,13 +244,13 @@ func TestAccountAbstraction(t *testing.T) {
AuthenticationMethod: "secp256k1",
AuthenticationData: []byte("signature"),
AuthenticationGasLimit: 10000,
BundlerPaymentMessages: intoAny(t, &bankv1beta1.MsgSend{
BundlerPaymentMessages: intoAny(t, &banktypes.MsgSend{
FromAddress: aaFullAddrStr,
ToAddress: bundlerAddrStr,
Amount: coins(t, "1stake"), // we expect this to fail since the account is implement in such a way not to allow bank sends.
}),
BundlerPaymentGasLimit: 50000,
ExecutionMessages: intoAny(t, &bankv1beta1.MsgSend{
ExecutionMessages: intoAny(t, &banktypes.MsgSend{
FromAddress: aaFullAddrStr,
ToAddress: aliceAddrStr,
Amount: coins(t, "2000stake"),
Expand All @@ -273,7 +270,7 @@ func TestAccountAbstraction(t *testing.T) {
AuthenticationGasLimit: 10000,
BundlerPaymentMessages: nil,
BundlerPaymentGasLimit: 50000,
ExecutionMessages: intoAny(t, &stakingv1beta1.MsgDelegate{
ExecutionMessages: intoAny(t, &stakingtypes.MsgDelegate{
DelegatorAddress: aaFullAddrStr,
ValidatorAddress: "some-validator",
Amount: coins(t, "2000stake")[0],
Expand All @@ -300,14 +297,14 @@ func TestAccountAbstraction(t *testing.T) {
AuthenticationMethod: "secp256k1",
AuthenticationData: []byte("signature"),
AuthenticationGasLimit: 10000,
BundlerPaymentMessages: intoAny(t, &nftv1beta1.MsgSend{
BundlerPaymentMessages: intoAny(t, &nft.MsgSend{
ClassId: "omega-rare",
Id: "the-most-rare",
Sender: aaFullAddrStr,
Receiver: bundlerAddrStr,
}),
BundlerPaymentGasLimit: 50000,
ExecutionMessages: intoAny(t, &bankv1beta1.MsgSend{
ExecutionMessages: intoAny(t, &banktypes.MsgSend{
FromAddress: aaFullAddrStr,
ToAddress: aliceAddrStr,
Amount: coins(t, "2000stake"),
Expand All @@ -318,28 +315,21 @@ func TestAccountAbstraction(t *testing.T) {
})
}

func intoAny(t *testing.T, msgs ...proto.Message) (anys []*anypb.Any) {
func intoAny(t *testing.T, msgs ...gogoproto.Message) (anys []*codectypes.Any) {
t.Helper()
for _, msg := range msgs {
any, err := anyutil.New(msg)
any, err := codectypes.NewAnyWithValue(msg)
require.NoError(t, err)
anys = append(anys, any)
}
return
}

func coins(t *testing.T, s string) []*v1beta1.Coin {
func coins(t *testing.T, s string) sdk.Coins {
t.Helper()
coins, err := sdk.ParseCoinsNormalized(s)
require.NoError(t, err)
coinsv2 := make([]*v1beta1.Coin, len(coins))
for i, coin := range coins {
coinsv2[i] = &v1beta1.Coin{
Denom: coin.Denom,
Amount: coin.Amount.String(),
}
}
return coinsv2
return coins
}

func balanceIs(t *testing.T, ctx context.Context, app *simapp.SimApp, addr sdk.AccAddress, s string) {
Expand Down
13 changes: 0 additions & 13 deletions tests/e2e/accounts/setup_test.go
View file
Open in desktop

This file was deleted.

Loading
Loading