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

Clean Any interface #8167

Merged
merged 14 commits into from
Dec 18, 2020
12 changes: 2 additions & 10 deletions client/tx/tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/client/flags"
"github.com/cosmos/cosmos-sdk/client/input"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -263,22 +262,15 @@ func BuildSimTx(txf Factory, msgs ...sdk.Msg) ([]byte, error) {
},
Sequence: txf.Sequence(),
}

if err := txb.SetSignatures(sig); err != nil {
return nil, err
}

any, ok := txb.(codectypes.IntoAny)
if !ok {
return nil, fmt.Errorf("cannot simulate tx that cannot be wrapped into any")
}
cached := any.AsAny().GetCachedValue()
protoTx, ok := cached.(*tx.Tx)
protoProvider, ok := txb.(tx.ProtoTxProvider)
if !ok {
return nil, fmt.Errorf("cannot simulate amino tx")
}

simReq := tx.SimulateRequest{Tx: protoTx}
simReq := tx.SimulateRequest{Tx: protoProvider.GetProtoTx()}

return simReq.Marshal()
}
Expand Down
67 changes: 24 additions & 43 deletions codec/types/any.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,18 @@ type Any struct {
// returns an error if that value couldn't be packed. This also caches
// the packed value so that it can be retrieved from GetCachedValue without
// unmarshaling
func NewAnyWithValue(value proto.Message) (*Any, error) {
any := &Any{}

err := any.Pack(value)
if err != nil {
return nil, err
func NewAnyWithValue(v proto.Message) (*Any, error) {
if v == nil {
return nil, sdkerrors.Wrap(sdkerrors.ErrPackAny, "Expecting non nil value to create a new Any")
// TODO, second option
// return nil, nil
robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved
}

return any, nil
bz, err := proto.Marshal(v)
return &Any{
TypeUrl: "/" + proto.MessageName(v),
robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved
Value: bz,
cachedValue: v,
}, err
}

// NewAnyWithCustomTypeURL same as NewAnyWithValue, but sets a custom type url, instead
Expand All @@ -82,22 +85,6 @@ func NewAnyWithCustomTypeURL(v proto.Message, typeURL string) (*Any, error) {
}, err
}

// Pack packs the value x in the Any or returns an error. This also caches
// the packed value so that it can be retrieved from GetCachedValue without
// unmarshaling
func (any *Any) Pack(x proto.Message) error {
any.TypeUrl = "/" + proto.MessageName(x)
bz, err := proto.Marshal(x)
if err != nil {
return err
}

any.Value = bz
any.cachedValue = x

return nil
}

// UnsafePackAny packs the value x in the Any and instead of returning the error
// in the case of a packing failure, keeps the cached value. This should only
// be used in situations where compatibility is needed with amino. Amino-only
Expand All @@ -113,21 +100,20 @@ func UnsafePackAny(x interface{}) *Any {
return &Any{cachedValue: x}
}

// PackAny is a checked and safe version of UnsafePackAny. It assures that
// `x` implements the proto.Message interface and uses it to serialize `x`.
// [DEPRECATED]: should be moved away: https://github.com/cosmos/cosmos-sdk/issues/7479
func PackAny(x interface{}) (*Any, error) {
if x == nil {
return nil, nil
}
if intoany, ok := x.(IntoAny); ok {
return intoany.AsAny(), nil
}
protoMsg, ok := x.(proto.Message)
if !ok {
return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "Expecting %T to implement proto.Message", x)
// pack packs the value x in the Any or returns an error. This also caches
// the packed value so that it can be retrieved from GetCachedValue without
// unmarshaling
func (any *Any) pack(x proto.Message) error {
any.TypeUrl = "/" + proto.MessageName(x)
bz, err := proto.Marshal(x)
if err != nil {
return err
}
return NewAnyWithValue(protoMsg)

any.Value = bz
any.cachedValue = x

return nil
}

// GetCachedValue returns the cached value from the Any if present
Expand All @@ -139,8 +125,3 @@ func (any *Any) GetCachedValue() interface{} {
func (any *Any) ClearCachedValue() {
any.cachedValue = nil
}

// IntoAny represents a type that can be wrapped into an Any.
type IntoAny interface {
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
AsAny() *Any
}
6 changes: 2 additions & 4 deletions codec/types/compat.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,7 @@ func (a AminoUnpacker) UnpackAny(any *Any, iface interface{}) error {
return err
}
if m, ok := val.(proto.Message); ok {
err := any.Pack(m)
if err != nil {
if err = any.pack(m); err != nil {
return err
}
} else {
Expand Down Expand Up @@ -148,8 +147,7 @@ func (a AminoJSONUnpacker) UnpackAny(any *Any, iface interface{}) error {
return err
}
if m, ok := val.(proto.Message); ok {
err := any.Pack(m)
if err != nil {
if err = any.pack(m); err != nil {
return err
}
} else {
Expand Down
21 changes: 8 additions & 13 deletions codec/types/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,24 +22,19 @@ func TestPackUnpack(t *testing.T) {
registry := testdata.NewTestInterfaceRegistry()

spot := &testdata.Dog{Name: "Spot"}
any := types.Any{}
err := any.Pack(spot)
require.NoError(t, err)

require.Equal(t, spot, any.GetCachedValue())

// without cache
any.ClearCachedValue()
var animal testdata.Animal
err = registry.UnpackAny(&any, &animal)
require.NoError(t, err)
require.Equal(t, spot, animal)

// with cache
err = any.Pack(spot)
any, err := types.NewAnyWithValue(spot)
require.NoError(t, err)
require.Equal(t, spot, any.GetCachedValue())
err = registry.UnpackAny(any, &animal)
require.NoError(t, err)
err = registry.UnpackAny(&any, &animal)
require.Equal(t, spot, animal)

// without cache
any.ClearCachedValue()
err = registry.UnpackAny(any, &animal)
require.NoError(t, err)
require.Equal(t, spot, animal)
}
Expand Down
2 changes: 1 addition & 1 deletion simapp/test_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func SetupWithGenesisValSet(t *testing.T, valSet *tmtypes.ValidatorSet, genAccs
for _, val := range valSet.Validators {
pk, err := cryptocodec.FromTmPubKeyInterface(val.PubKey)
require.NoError(t, err)
pkAny, err := codectypes.PackAny(pk)
pkAny, err := codectypes.NewAnyWithValue(pk)
require.NoError(t, err)
validator := stakingtypes.Validator{
OperatorAddress: sdk.ValAddress(val.Address).String(),
Expand Down
5 changes: 5 additions & 0 deletions types/tx/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -197,3 +197,8 @@ func RegisterInterfaces(registry codectypes.InterfaceRegistry) {
func isServiceMsg(typeURL string) bool {
return strings.Count(typeURL, "/") >= 2
}

// ProtoTxProvider is a type which can provide a proto transaction.
type ProtoTxProvider interface {
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
GetProtoTx() *Tx
}
19 changes: 8 additions & 11 deletions x/auth/client/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/cosmos/cosmos-sdk/client"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/tx"
)

// QueryTxsByEvents performs a search for transactions for a given set of events
Expand Down Expand Up @@ -134,26 +135,22 @@ func getBlocksForTxResults(clientCtx client.Context, resTxs []*ctypes.ResultTx)
}

func formatTxResult(txConfig client.TxConfig, resTx *ctypes.ResultTx, resBlock *ctypes.ResultBlock) (*sdk.TxResponse, error) {
anyTx, err := parseTx(txConfig, resTx.Tx)
anyTx, err := txToAny(txConfig, resTx.Tx)
if err != nil {
return nil, err
}

return sdk.NewResponseResultTx(resTx, anyTx.AsAny(), resBlock.Block.Time.Format(time.RFC3339)), nil
return sdk.NewResponseResultTx(resTx, anyTx, resBlock.Block.Time.Format(time.RFC3339)), nil
}

func parseTx(txConfig client.TxConfig, txBytes []byte) (codectypes.IntoAny, error) {
var tx sdk.Tx

tx, err := txConfig.TxDecoder()(txBytes)
func txToAny(txConfig client.TxConfig, txBytes []byte) (*codectypes.Any, error) {
txb, err := txConfig.TxDecoder()(txBytes)
if err != nil {
return nil, err
}

anyTx, ok := tx.(codectypes.IntoAny)
p, ok := txb.(tx.ProtoTxProvider)
Copy link
Contributor

@amaury1093 amaury1093 Dec 15, 2020

Choose a reason for hiding this comment

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

I think the edge case is when using a TxConfig that's a StdTxConfig, this will fail because StdTx doesn't implement ProtoTxProvider.

iirc, I believe that's why I initially chose the IntoAny hack, because Any is something that both wrapper and StdTx understand.

In practice, I think no-one creates a client using StdTxConfig, so maybe it's fine to drop support for StdTxConfig in this case? Or maybe that's a bigger discussion in #7489.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for note. So what we need is a Tx (interface) provider. I will have a look into it tomorrow.

if !ok {
return nil, fmt.Errorf("tx cannot be packed into Any")
return nil, fmt.Errorf("expecting a proto transaction builder, got %T", txb)
}

return anyTx, nil
return codectypes.NewAnyWithValue(p.GetProtoTx())
}
12 changes: 3 additions & 9 deletions x/auth/legacy/legacytx/stdtx.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,9 @@ import (

// Interface implementation checks
var (
_ sdk.Tx = (*StdTx)(nil)
_ codectypes.IntoAny = (*StdTx)(nil)
_ sdk.TxWithMemo = (*StdTx)(nil)
_ sdk.FeeTx = (*StdTx)(nil)
_ sdk.Tx = (*StdTx)(nil)
_ sdk.TxWithMemo = (*StdTx)(nil)
_ sdk.FeeTx = (*StdTx)(nil)
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
)

// StdFee includes the amount of coins paid in fees and the maximum
Expand Down Expand Up @@ -172,11 +171,6 @@ func (tx StdTx) ValidateBasic() error {
return nil
}

// AsAny implements IntoAny.AsAny.
func (tx *StdTx) AsAny() *codectypes.Any {
return codectypes.UnsafePackAny(tx)
}

// GetSigners returns the addresses that must sign the transaction.
// Addresses are returned in a deterministic order.
// They are accumulated from the GetSigners method for each Msg
Expand Down
9 changes: 3 additions & 6 deletions x/auth/tx/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ var (
_ client.TxBuilder = &wrapper{}
_ ante.HasExtensionOptionsTx = &wrapper{}
_ ExtensionOptionsTxBuilder = &wrapper{}
_ codectypes.IntoAny = &wrapper{}
_ tx.ProtoTxProvider = &wrapper{}
)

// ExtensionOptionsTxBuilder defines a TxBuilder that can also set extensions.
Expand Down Expand Up @@ -315,11 +315,8 @@ func (w *wrapper) GetTx() authsigning.Tx {
return w
}

// GetProtoTx returns the tx as a proto.Message.
func (w *wrapper) AsAny() *codectypes.Any {
// We're sure here that w.tx is a proto.Message, so this will call
// codectypes.NewAnyWithValue under the hood.
return codectypes.UnsafePackAny(w.tx)
func (w *wrapper) GetProtoTx() *tx.Tx {
return w.tx
}

// WrapTx creates a TxBuilder wrapper around a tx.Tx proto message.
Expand Down
17 changes: 3 additions & 14 deletions x/auth/tx/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/client/flags"
clienttx "github.com/cosmos/cosmos-sdk/client/tx"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
"github.com/cosmos/cosmos-sdk/testutil/network"
"github.com/cosmos/cosmos-sdk/testutil/testdata"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -456,20 +455,10 @@ func (s IntegrationTestSuite) mkTxBuilder() client.TxBuilder {

// txBuilderToProtoTx converts a txBuilder into a proto tx.Tx.
func txBuilderToProtoTx(txBuilder client.TxBuilder) (*tx.Tx, error) { // nolint
intoAnyTx, ok := txBuilder.(codectypes.IntoAny)
protoProvider, ok := txBuilder.(tx.ProtoTxProvider)
if !ok {
return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "expected %T, got %T", (codectypes.IntoAny)(nil), intoAnyTx)
return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "expected proto tx builder, got %T", txBuilder)
}

any := intoAnyTx.AsAny().GetCachedValue()
if any == nil {
return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "any's cached value is empty")
}

protoTx, ok := any.(*tx.Tx)
if !ok {
return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "expected %T, got %T", (codectypes.IntoAny)(nil), intoAnyTx)
}

return protoTx, nil
return protoProvider.GetProtoTx(), nil
}
7 changes: 6 additions & 1 deletion x/auth/types/account.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,12 @@ func (acc BaseAccount) GetPubKey() (pk cryptotypes.PubKey) {

// SetPubKey - Implements sdk.AccountI.
func (acc *BaseAccount) SetPubKey(pubKey cryptotypes.PubKey) error {
any, err := codectypes.PackAny(pubKey)
// TODO: decied if we handle "valid nil" case here or in NewAnyWithValue
if pubKey == nil {
acc.PubKey = nil
return nil
}
any, err := codectypes.NewAnyWithValue(pubKey)
if err != nil {
return err
}
Expand Down
4 changes: 2 additions & 2 deletions x/staking/genesis_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,10 @@ func TestInitGenesis(t *testing.T) {
validators := make([]types.Validator, 2)
var delegations []types.Delegation

pk0, err := codectypes.PackAny(PKs[0])
pk0, err := codectypes.NewAnyWithValue(PKs[0])
require.NoError(t, err)

pk1, err := codectypes.PackAny(PKs[1])
pk1, err := codectypes.NewAnyWithValue(PKs[1])
require.NoError(t, err)

// initialize the validators
Expand Down
2 changes: 1 addition & 1 deletion x/staking/legacy/v040/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func Migrate(stakingState v038staking.GenesisState) *v040staking.GenesisState {

newValidators := make([]v040staking.Validator, len(stakingState.Validators))
for i, oldValidator := range stakingState.Validators {
pkAny, err := codectypes.PackAny(oldValidator.ConsPubKey)
pkAny, err := codectypes.NewAnyWithValue(oldValidator.ConsPubKey)
if err != nil {
panic(fmt.Sprintf("Can't pack validator consensus PK as Any: %s", err))
}
Expand Down
2 changes: 1 addition & 1 deletion x/staking/types/data_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ var (

func init() {
var err error
pk1Any, err = codectypes.PackAny(pk1)
pk1Any, err = codectypes.NewAnyWithValue(pk1)
if err != nil {
panic(fmt.Sprintf("Can't pack pk1 %t as Any", pk1))
}
Expand Down
14 changes: 9 additions & 5 deletions x/staking/types/msg.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,16 @@ var (
// NewMsgCreateValidator creates a new MsgCreateValidator instance.
// Delegator address and validator address are the same.
func NewMsgCreateValidator(
valAddr sdk.ValAddress, pubKey cryptotypes.PubKey, selfDelegation sdk.Coin,
description Description, commission CommissionRates, minSelfDelegation sdk.Int,
valAddr sdk.ValAddress, pubKey cryptotypes.PubKey, //nolint:interfacer
selfDelegation sdk.Coin, description Description, commission CommissionRates, minSelfDelegation sdk.Int,
) (*MsgCreateValidator, error) {
pkAny, err := codectypes.PackAny(pubKey)
if err != nil {
return nil, err
// TODO: update if we decide to allow nil in NewAnyWithValue
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
var pkAny *codectypes.Any
if pubKey != nil {
var err error
if pkAny, err = codectypes.NewAnyWithValue(pubKey); err != nil {
robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved
return nil, err
}
}
return &MsgCreateValidator{
Description: description,
Expand Down
2 changes: 1 addition & 1 deletion x/staking/types/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ var _ ValidatorI = Validator{}
// NewValidator constructs a new Validator
//nolint:interfacer
func NewValidator(operator sdk.ValAddress, pubKey cryptotypes.PubKey, description Description) (Validator, error) {
pkAny, err := codectypes.PackAny(pubKey)
pkAny, err := codectypes.NewAnyWithValue(pubKey)
if err != nil {
return Validator{}, err
}
Expand Down