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

refactor!: turn MsgsV2 into ReflectMessages to make it less confusing #19839

Merged
merged 24 commits into from
May 16, 2024
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
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 @@ -181,6 +181,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i
* Every module has the codec already, passing it created an unneeded dependency.
* Additionally, to reflect this change, the module manager does not take a codec either.
Comment on lines 181 to 182
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjust the indentation of the unordered list items to maintain consistency and readability in the document.

-    * Every module has the codec already, passing it created an unneeded dependency.
-    * Additionally, to reflect this change, the module manager does not take a codec either.
+  * Every module has the codec already, passing it created an unneeded dependency.
+  * Additionally, to reflect this change, the module manager does not take a codec either.

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.

Suggested change
* Every module has the codec already, passing it created an unneeded dependency.
* Additionally, to reflect this change, the module manager does not take a codec either.
* Every module has the codec already, passing it created an unneeded dependency.
* Additionally, to reflect this change, the module manager does not take a codec either.

* (runtime) [#19747](https://github.com/cosmos/cosmos-sdk/pull/19747) `runtime.ValidatorAddressCodec` and `runtime.ConsensusAddressCodec` have been moved to `core`.
* [#19839](https://github.com/cosmos/cosmos-sdk/pull/19839) `Tx.GetMsgsV2` has been replaced with `Tx.GetReflectMessages`, and `Codec.GetMsgV1Signers` and `Codec.GetMsgV2Signers` have been replaced with `GetMsgSigners` and `GetReflectMsgSigners` respectively. These API changes clear up confusion as to the use and purpose of these methods.
* (baseapp) [#19993](https://github.com/cosmos/cosmos-sdk/pull/19993) Indicate pruning with error code "not found" rather than "invalid request".
* (x/consensus) [#20010](https://github.com/cosmos/cosmos-sdk/pull/20010) Move consensus module to be its own go.mod
* (server) [#20140](https://github.com/cosmos/cosmos-sdk/pull/20140) Remove embedded grpc-web proxy in favor of standalone grpc-web proxy. [Envoy Proxy](https://www.envoyproxy.io/docs/envoy/latest/start/start)
Expand Down
14 changes: 7 additions & 7 deletions baseapp/baseapp.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import (
dbm "github.com/cosmos/cosmos-db"
"github.com/cosmos/gogoproto/proto"
"golang.org/x/exp/maps"
protov2 "google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protoreflect"

"cosmossdk.io/core/header"
errorsmod "cosmossdk.io/errors"
Expand Down Expand Up @@ -950,9 +950,9 @@ func (app *BaseApp) runTx(mode execMode, txBytes []byte) (gInfo sdk.GasInfo, res
// Attempt to execute all messages and only update state if all messages pass
// and we're in DeliverTx. Note, runMsgs will never return a reference to a
// Result if any single message fails or does not have a registered Handler.
msgsV2, err := tx.GetMsgsV2()
reflectMsgs, err := tx.GetReflectMessages()
if err == nil {
result, err = app.runMsgs(runMsgCtx, msgs, msgsV2, mode)
result, err = app.runMsgs(runMsgCtx, msgs, reflectMsgs, mode)
}

// Run optional postHandlers (should run regardless of the execution result).
Expand Down Expand Up @@ -998,7 +998,7 @@ func (app *BaseApp) runTx(mode execMode, txBytes []byte) (gInfo sdk.GasInfo, res
// and DeliverTx. An error is returned if any single message fails or if a
// Handler does not exist for a given message route. Otherwise, a reference to a
// Result is returned. The caller must not commit state if an error is returned.
func (app *BaseApp) runMsgs(ctx sdk.Context, msgs []sdk.Msg, msgsV2 []protov2.Message, mode execMode) (*sdk.Result, error) {
func (app *BaseApp) runMsgs(ctx sdk.Context, msgs []sdk.Msg, reflectMsgs []protoreflect.Message, mode execMode) (*sdk.Result, error) {
events := sdk.EmptyEvents()
msgResponses := make([]*codectypes.Any, 0, len(msgs))

Expand All @@ -1020,7 +1020,7 @@ func (app *BaseApp) runMsgs(ctx sdk.Context, msgs []sdk.Msg, msgsV2 []protov2.Me
}

// create message events
msgEvents, err := createEvents(app.cdc, msgResult.GetEvents(), msg, msgsV2[i])
msgEvents, err := createEvents(app.cdc, msgResult.GetEvents(), msg, reflectMsgs[i])
if err != nil {
return nil, errorsmod.Wrapf(err, "failed to create message events; message index: %d", i)
}
Expand Down Expand Up @@ -1066,12 +1066,12 @@ func makeABCIData(msgResponses []*codectypes.Any) ([]byte, error) {
return proto.Marshal(&sdk.TxMsgData{MsgResponses: msgResponses})
}

func createEvents(cdc codec.Codec, events sdk.Events, msg sdk.Msg, msgV2 protov2.Message) (sdk.Events, error) {
func createEvents(cdc codec.Codec, events sdk.Events, msg sdk.Msg, reflectMsg protoreflect.Message) (sdk.Events, error) {
eventMsgName := sdk.MsgTypeURL(msg)
msgEvent := sdk.NewEvent(sdk.EventTypeMessage, sdk.NewAttribute(sdk.AttributeKeyAction, eventMsgName))

// we set the signer attribute as the sender
signers, err := cdc.GetMsgV2Signers(msgV2)
signers, err := cdc.GetReflectMsgSigners(reflectMsg)
if err != nil {
return nil, err
}
Expand Down
26 changes: 14 additions & 12 deletions codec/codec.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package codec
import (
"github.com/cosmos/gogoproto/proto"
"google.golang.org/grpc/encoding"
protov2 "google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protoreflect"

"github.com/cosmos/cosmos-sdk/codec/types"
)
Expand All @@ -24,17 +24,19 @@ type (
InterfaceRegistry() types.InterfaceRegistry

// GetMsgAnySigners returns the signers of the given message encoded in a protobuf Any
// as well as the decoded google.golang.org/protobuf/proto.Message that was used to
// extract the signers so that this can be used in other contexts.
GetMsgAnySigners(msg *types.Any) ([][]byte, protov2.Message, error)

// GetMsgV2Signers returns the signers of the given message.
GetMsgV2Signers(msg protov2.Message) ([][]byte, error)

// GetMsgV1Signers returns the signers of the given message plus the
// decoded google.golang.org/protobuf/proto.Message that was used to extract the
// signers so that this can be used in other contexts.
GetMsgV1Signers(msg proto.Message) ([][]byte, protov2.Message, error)
// as well as the decoded protoreflect.Message that was used to extract the
// signers so that this can be used in other context where proto reflection
// is needed.
GetMsgAnySigners(msg *types.Any) ([][]byte, protoreflect.Message, error)

// GetMsgSigners returns the signers of the given message plus the
// decoded protoreflect.Message that was used to extract the
// signers so that this can be used in other context where proto reflection
// is needed.
GetMsgSigners(msg proto.Message) ([][]byte, protoreflect.Message, error)
aaronc marked this conversation as resolved.
Show resolved Hide resolved

// GetReflectMsgSigners returns the signers of the given reflected proto message.
GetReflectMsgSigners(msg protoreflect.Message) ([][]byte, error)

// mustEmbedCodec requires that all implementations of Codec embed an official implementation from the codec
// package. This allows new methods to be added to the Codec interface without breaking backwards compatibility.
Expand Down
12 changes: 6 additions & 6 deletions codec/proto_codec.go
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ func (pc *ProtoCodec) InterfaceRegistry() types.InterfaceRegistry {
return pc.interfaceRegistry
}

func (pc ProtoCodec) GetMsgAnySigners(msg *types.Any) ([][]byte, proto.Message, error) {
func (pc ProtoCodec) GetMsgAnySigners(msg *types.Any) ([][]byte, protoreflect.Message, error) {
msgv2, err := anyutil.Unpack(&anypb.Any{
TypeUrl: msg.TypeUrl,
Value: msg.Value,
Expand All @@ -309,17 +309,17 @@ func (pc ProtoCodec) GetMsgAnySigners(msg *types.Any) ([][]byte, proto.Message,
}

signers, err := pc.interfaceRegistry.SigningContext().GetSigners(msgv2)
return signers, msgv2, err
return signers, msgv2.ProtoReflect(), err
}

func (pc *ProtoCodec) GetMsgV2Signers(msg proto.Message) ([][]byte, error) {
return pc.interfaceRegistry.SigningContext().GetSigners(msg)
func (pc *ProtoCodec) GetReflectMsgSigners(msg protoreflect.Message) ([][]byte, error) {
return pc.interfaceRegistry.SigningContext().GetSigners(msg.Interface())
}

func (pc *ProtoCodec) GetMsgV1Signers(msg gogoproto.Message) ([][]byte, proto.Message, error) {
func (pc *ProtoCodec) GetMsgSigners(msg gogoproto.Message) ([][]byte, protoreflect.Message, error) {
if msgV2, ok := msg.(proto.Message); ok {
signers, err := pc.interfaceRegistry.SigningContext().GetSigners(msgV2)
return signers, msgV2, err
return signers, msgV2.ProtoReflect(), err
}
a, err := types.NewAnyWithValue(msg)
if err != nil {
Expand Down
8 changes: 4 additions & 4 deletions codec/proto_codec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -191,12 +191,12 @@ func TestGetSigners(t *testing.T) {
msgSendV1 := &countertypes.MsgIncreaseCounter{Signer: testAddrStr, Count: 1}
msgSendV2 := &counterv1.MsgIncreaseCounter{Signer: testAddrStr, Count: 1}

signers, msgSendV2Copy, err := cdc.GetMsgV1Signers(msgSendV1)
signers, msgSendV2Copy, err := cdc.GetMsgSigners(msgSendV1)
require.NoError(t, err)
require.Equal(t, [][]byte{testAddr}, signers)
require.True(t, protov2.Equal(msgSendV2, msgSendV2Copy))
require.True(t, protov2.Equal(msgSendV2, msgSendV2Copy.Interface()))

signers, err = cdc.GetMsgV2Signers(msgSendV2)
signers, err = cdc.GetReflectMsgSigners(msgSendV2.ProtoReflect())
require.NoError(t, err)
require.Equal(t, [][]byte{testAddr}, signers)

Expand All @@ -205,7 +205,7 @@ func TestGetSigners(t *testing.T) {
signers, msgSendV2Copy, err = cdc.GetMsgAnySigners(msgSendAny)
require.NoError(t, err)
require.Equal(t, [][]byte{testAddr}, signers)
require.True(t, protov2.Equal(msgSendV2, msgSendV2Copy))
require.True(t, protov2.Equal(msgSendV2, msgSendV2Copy.Interface()))
}

type testAddressCodec struct{}
Expand Down
6 changes: 3 additions & 3 deletions server/mock/tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import (
"bytes"
"fmt"

protov2 "google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protoreflect"

bankv1beta1 "cosmossdk.io/api/cosmos/bank/v1beta1"
errorsmod "cosmossdk.io/errors"
Expand Down Expand Up @@ -103,8 +103,8 @@ func (msg *KVStoreTx) GetMsgs() []sdk.Msg {
return []sdk.Msg{msg}
}

func (msg *KVStoreTx) GetMsgsV2() ([]protov2.Message, error) {
return []protov2.Message{&bankv1beta1.MsgSend{FromAddress: msg.address.String()}}, nil // this is a hack for tests
func (msg *KVStoreTx) GetReflectMessages() ([]protoreflect.Message, error) {
return []protoreflect.Message{(&bankv1beta1.MsgSend{FromAddress: msg.address.String()}).ProtoReflect()}, nil // this is a hack for tests
}

func (msg *KVStoreTx) GetSignBytes() []byte {
Expand Down
6 changes: 3 additions & 3 deletions types/mempool/mempool_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import (

"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
protov2 "google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protoreflect"

_ "cosmossdk.io/api/cosmos/counter/v1"
_ "cosmossdk.io/api/cosmos/crypto/secp256k1"
Expand Down Expand Up @@ -77,7 +77,7 @@ var (

func (tx testTx) GetMsgs() []sdk.Msg { return nil }

func (tx testTx) GetMsgsV2() ([]protov2.Message, error) { return nil, nil }
func (tx testTx) GetReflectMessages() ([]protoreflect.Message, error) { return nil, nil }

func (tx testTx) ValidateBasic() error { return nil }

Expand All @@ -93,7 +93,7 @@ func (sigErrTx) Size() int64 { return 0 }

func (sigErrTx) GetMsgs() []sdk.Msg { return nil }

func (sigErrTx) GetMsgsV2() ([]protov2.Message, error) { return nil, nil }
func (sigErrTx) GetReflectMessages() ([]protoreflect.Message, error) { return nil, nil }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, the GetReflectMessages method in the sigErrTx struct returns nil, nil as a placeholder. It's important to implement tests that verify the functionality of this method.

Would you like assistance in creating tests for the GetReflectMessages method?


func (sigErrTx) ValidateBasic() error { return nil }

Expand Down
4 changes: 2 additions & 2 deletions types/mempool/signer_extraction_adapater_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import (
"testing"

"github.com/stretchr/testify/require"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protoreflect"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/mempool"
Expand All @@ -20,7 +20,7 @@ func (n nonVerifiableTx) GetMsgs() []sdk.Msg {
panic("not implemented")
}

func (n nonVerifiableTx) GetMsgsV2() ([]proto.Message, error) {
func (n nonVerifiableTx) GetReflectMessages() ([]protoreflect.Message, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method GetReflectMessages is correctly updated to return []protoreflect.Message, aligning with the PR's objectives. However, it's essential to ensure that the panic statement is only a placeholder and that actual implementation or tests are planned.

Would you like assistance in implementing the actual logic for GetReflectMessages or creating meaningful tests for this method?

panic("not implemented")
}

Expand Down
12 changes: 6 additions & 6 deletions types/tx/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package tx
import (
"fmt"

protov2 "google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protoreflect"

errorsmod "cosmossdk.io/errors"

Expand Down Expand Up @@ -97,18 +97,18 @@ func (t *Tx) ValidateBasic() error {
// GetSigners retrieves all the signers of a tx.
// This includes all unique signers of the messages (in order),
// as well as the FeePayer (if specified and not already included).
func (t *Tx) GetSigners(cdc codec.Codec) ([][]byte, []protov2.Message, error) {
func (t *Tx) GetSigners(cdc codec.Codec) ([][]byte, []protoreflect.Message, error) {
var signers [][]byte
seen := map[string]bool{}

var msgsv2 []protov2.Message
var reflectMsgs []protoreflect.Message
for _, msg := range t.Body.Messages {
xs, msgv2, err := cdc.GetMsgAnySigners(msg)
xs, reflectMsg, err := cdc.GetMsgAnySigners(msg)
if err != nil {
return nil, nil, err
}

msgsv2 = append(msgsv2, msgv2)
reflectMsgs = append(reflectMsgs, reflectMsg)

for _, signer := range xs {
if !seen[string(signer)] {
Expand All @@ -133,7 +133,7 @@ func (t *Tx) GetSigners(cdc codec.Codec) ([][]byte, []protov2.Message, error) {
seen[string(feePayerAddr)] = true
}

return signers, msgsv2, nil
return signers, reflectMsgs, nil
}

func (t *Tx) GetGas() uint64 {
Expand Down
12 changes: 8 additions & 4 deletions types/tx_msg.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,8 @@ import (
"fmt"
"strings"

protov2 "google.golang.org/protobuf/proto"

coretransaction "cosmossdk.io/core/transaction"
"google.golang.org/protobuf/reflect/protoreflect"

"github.com/cosmos/cosmos-sdk/codec"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
Expand Down Expand Up @@ -53,8 +52,13 @@ type (
Tx interface {
HasMsgs

// GetMsgsV2 gets the transaction's messages as google.golang.org/protobuf/proto.Message's.
GetMsgsV2() ([]protov2.Message, error)
// GetReflectMessages gets a reflected version of the transaction's messages
// that can be used by dynamic APIs. These messages should not be used for actual
// processing as they cannot be guaranteed to be what handlers are expecting, but
// they can be used for dynamically reading specific fields from the message such
// as signers or validation data. Message processors should ALWAYS use the messages
// returned by GetMsgs.
GetReflectMessages() ([]protoreflect.Message, error)
}

// FeeTx defines the interface to be implemented by Tx to use the FeeDecorators
Expand Down
6 changes: 3 additions & 3 deletions x/accounts/defaults/multisig/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,17 @@ type mockStateCodec struct {
}

// GetMsgAnySigners implements codec.Codec.
func (mockStateCodec) GetMsgAnySigners(msg *types.Any) ([][]byte, protoreflect.ProtoMessage, error) {
func (mockStateCodec) GetMsgAnySigners(msg *types.Any) ([][]byte, protoreflect.Message, error) {
panic("unimplemented")
}

// GetMsgV1Signers implements codec.Codec.
func (mockStateCodec) GetMsgV1Signers(msg gogoproto.Message) ([][]byte, protoreflect.ProtoMessage, error) {
func (mockStateCodec) GetMsgSigners(msg gogoproto.Message) ([][]byte, protoreflect.Message, error) {
panic("unimplemented")
}

// GetMsgV2Signers implements codec.Codec.
func (mockStateCodec) GetMsgV2Signers(msg protoreflect.ProtoMessage) ([][]byte, error) {
func (mockStateCodec) GetReflectMsgSigners(msg protoreflect.Message) ([][]byte, error) {
panic("unimplemented")
}

Expand Down
4 changes: 2 additions & 2 deletions x/accounts/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ func (k Keeper) sendAnyMessages(ctx context.Context, sender []byte, anyMessages
// It should be used when the response type is not known by the caller.
func (k Keeper) SendModuleMessageUntyped(ctx context.Context, sender []byte, msg implementation.ProtoMsg) (implementation.ProtoMsg, error) {
// do sender assertions.
wantSenders, _, err := k.codec.GetMsgV1Signers(msg)
wantSenders, _, err := k.codec.GetMsgSigners(msg)
if err != nil {
return nil, fmt.Errorf("cannot get signers: %w", err)
}
Expand All @@ -348,7 +348,7 @@ func (k Keeper) SendModuleMessageUntyped(ctx context.Context, sender []byte, msg
// is not trying to impersonate another account.
func (k Keeper) sendModuleMessage(ctx context.Context, sender []byte, msg, msgResp implementation.ProtoMsg) error {
// do sender assertions.
wantSenders, _, err := k.codec.GetMsgV1Signers(msg)
wantSenders, _, err := k.codec.GetMsgSigners(msg)
if err != nil {
return fmt.Errorf("cannot get signers: %w", err)
}
Expand Down
8 changes: 4 additions & 4 deletions x/accounts/utils_test.go
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be reverted.

Original file line number Diff line number Diff line change
Expand Up @@ -88,17 +88,17 @@ type bankQueryServer struct {
bankv1beta1.UnimplementedQueryServer
}

type bankMsgServer struct {
bankv1beta1.UnimplementedMsgServer
}

func (b bankQueryServer) Balance(context.Context, *bankv1beta1.QueryBalanceRequest) (*bankv1beta1.QueryBalanceResponse, error) {
return &bankv1beta1.QueryBalanceResponse{Balance: &basev1beta1.Coin{
Denom: "atom",
Amount: "1000",
}}, nil
}

type bankMsgServer struct {
bankv1beta1.UnimplementedMsgServer
}

func (b bankMsgServer) Send(context.Context, *bankv1beta1.MsgSend) (*bankv1beta1.MsgSendResponse, error) {
return &bankv1beta1.MsgSendResponse{}, nil
}
2 changes: 1 addition & 1 deletion x/auth/tx/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func newBuilderFromDecodedTx(addrCodec address.Codec, decoder *decode.Decoder, c
addressCodec: addrCodec,
decoder: decoder,
codec: codec,
msgs: decoded.msgsV1,
msgs: decoded.msgs,
timeoutHeight: decoded.GetTimeoutHeight(),
granter: decoded.FeeGranter(),
payer: payer,
Expand Down
Loading
Loading