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

Jump to bottom

fix: implement Amino serialization for x/authz and x/feegrant #11224

Merged
merged 22 commits into from
Feb 22, 2022
Merged

fix: implement Amino serialization for x/authz and x/feegrant #11224

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
78c16d2
fix: Add RegisterLegacyAminoCodec for authz/feegrant
amaury1093 Feb 17, 2022
d88839f
add module name
amaury1093 Feb 17, 2022
9a42a24
Fix GetSignByes, add tests
amaury1093 Feb 17, 2022
0c4a4f1
removed module names from registered messages to match other modules
RiccardoM Feb 18, 2022
8b2a7dc
added interfaces and concrete types registration
RiccardoM Feb 18, 2022
d3422c9
unseal amino instances to allow external grant and authorization regi…
RiccardoM Feb 18, 2022
bc169a0
fixed messages tests
RiccardoM Feb 18, 2022
e963aac
allow to register external types into authz modulecdc
RiccardoM Feb 18, 2022
04a2ff1
Merge branch 'master' into riccardo/fix-authz-feegrant-amino-encoding
RiccardoM Feb 18, 2022
c73b2a0
use legacy.Cdc instead of ModuleCdc inside x/authz
RiccardoM Feb 18, 2022
63f5b97
move the legacy.Cdc initialization outside init function
RiccardoM Feb 18, 2022
e9c91fc
Merge branch 'master' of github.com:cosmos/cosmos-sdk into riccardo/f…
RiccardoM Feb 18, 2022
09d2446
added serialization docs
RiccardoM Feb 18, 2022
db13bd5
Update docs/core/encoding.md
RiccardoM Feb 18, 2022
be8bc73
added the Ledger specification
RiccardoM Feb 21, 2022
d4b7104
Merge remote-tracking branch 'desmos/riccardo/fix-authz-feegrant-amin…
RiccardoM Feb 21, 2022
da242c1
Merge branch 'master' into riccardo/fix-authz-feegrant-amino-encoding
RiccardoM Feb 21, 2022
bac8689
Merge branch 'master' of github.com:cosmos/cosmos-sdk into riccardo/f…
RiccardoM Feb 21, 2022
a7bfed7
fixed tests
RiccardoM Feb 21, 2022
76b449e
Merge remote-tracking branch 'desmos/riccardo/fix-authz-feegrant-amin…
RiccardoM Feb 21, 2022
060fb4c
Merge branch 'master' into riccardo/fix-authz-feegrant-amino-encoding
RiccardoM Feb 21, 2022
ef26d33
Merge branch 'master' into riccardo/fix-authz-feegrant-amino-encoding
RiccardoM Feb 22, 2022
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [\#11011](https://github.com/cosmos/cosmos-sdk/pull/11011) Remove burning of deposits when qourum is not reached on a governance proposal and when the deposit is not fully met.
* [\#11019](https://github.com/cosmos/cosmos-sdk/pull/11019) Add `MsgCreatePermanentLockedAccount` and CLI method for creating permanent locked account
* (x/feegrant) [\#10830](https://github.com/cosmos/cosmos-sdk/pull/10830) Expired allowances will be pruned from state.
* (x/authz,x/feegrant) [\#11214](https://github.com/cosmos/cosmos-sdk/pull/11214) Fix Amino JSON encoding of authz and feegrant Msgs to be consistent with other modules.

### Deprecated

Expand Down
3 changes: 1 addition & 2 deletions codec/legacy/codec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,9 @@ import (
// has all Tendermint crypto and evidence types registered.
//
// TODO: Deprecated - remove this global.
var Cdc *codec.LegacyAmino
var Cdc = codec.NewLegacyAmino()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just to make sure the Cdc is initialized before running the various init methods. Since the initialization order is the following:

  1. package variables (as declared)
  2. init methods (based on the file structure)

It is safer to have the Cdc being initialized as a package variable rather than inside the init method.

Copy link

Choose a reason for hiding this comment

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

Initialization order is not necessarily defined in this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't it? I found this order inside the Go specs (here)

Copy link

Choose a reason for hiding this comment

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

You're right that vars are evaluated before init funcs, my mistake there. But this is very subtle behavior to depend on.

Copy link
Contributor

@alexanderbez alexanderbez Apr 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This honestly wasn't necessary IMO. I actually think keeping it in the init is cleaner as that's also where we do the initialization.


func init() {
Cdc = codec.NewLegacyAmino()
cryptocodec.RegisterCrypto(Cdc)
codec.RegisterEvidences(Cdc)
}
Expand Down
5 changes: 5 additions & 0 deletions x/auth/types/codec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package types

import (
"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/codec/legacy"
"github.com/cosmos/cosmos-sdk/codec/types"
cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
"github.com/cosmos/cosmos-sdk/x/auth/migrations/legacytx"
Expand Down Expand Up @@ -45,4 +46,8 @@ var (
func init() {
RegisterLegacyAminoCodec(amino)
cryptocodec.RegisterCrypto(amino)

// Register all Amino interfaces and concrete types on the global Amino codec so that this can later be
// used to properly serialize x/authz MsgExec instances
RegisterLegacyAminoCodec(legacy.Cdc)
}
18 changes: 18 additions & 0 deletions x/authz/codec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,24 @@
package authz

import (
"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/codec/legacy"
types "github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/msgservice"
)

// RegisterLegacyAminoCodec registers the necessary x/authz interfaces and concrete types
// on the provided LegacyAmino codec. These types are used for Amino JSON serialization.
func RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) {
cdc.RegisterConcrete(&MsgGrant{}, "cosmos-sdk/MsgGrant", nil)
cdc.RegisterConcrete(&MsgRevoke{}, "cosmos-sdk/MsgRevoke", nil)
cdc.RegisterConcrete(&MsgExec{}, "cosmos-sdk/MsgExec", nil)

cdc.RegisterInterface((*Authorization)(nil), nil)
cdc.RegisterConcrete(&GenericAuthorization{}, "cosmos-sdk/GenericAuthorization", nil)
Copy link
Contributor

@amaury1093 amaury1093 Feb 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We still have a problem here. GenericAuthorization allows any msg TypeURL, and then you can MsgExec with that arbitrary Msg. We would need to register authz.RegisterConcrete for every msg for every module.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know how to solve this problem right now. We can try to think of something before merging.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OMG, and we need to use the right type name...

}

// RegisterInterfaces registers the interfaces types with the interface registry
func RegisterInterfaces(registry types.InterfaceRegistry) {
registry.RegisterImplementations((*sdk.Msg)(nil),
Expand All @@ -22,3 +35,8 @@ func RegisterInterfaces(registry types.InterfaceRegistry) {

msgservice.RegisterMsgServiceDesc(registry, MsgServiceDesc())
}
func init() {
// Register all Amino interfaces and concrete types on the global Amino codec so that this can later be
// used to properly serialize MsgGrant and MsgExec instances
RegisterLegacyAminoCodec(legacy.Cdc)
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
}
4 changes: 3 additions & 1 deletion x/authz/module/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,9 @@ func (am AppModule) RegisterServices(cfg module.Configurator) {
}

// RegisterLegacyAminoCodec registers the authz module's types for the given codec.
func (AppModuleBasic) RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) {}
func (AppModuleBasic) RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) {
authz.RegisterLegacyAminoCodec(cdc)
}

// RegisterInterfaces registers the authz module's interface types
func (AppModuleBasic) RegisterInterfaces(registry cdctypes.InterfaceRegistry) {
Expand Down
2 changes: 1 addition & 1 deletion x/authz/msgs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
package authz

import (
"github.com/cosmos/cosmos-sdk/codec/legacy"
"time"

"github.com/gogo/protobuf/proto"

"github.com/cosmos/cosmos-sdk/codec/legacy"
cdctypes "github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
Expand Down
64 changes: 64 additions & 0 deletions x/authz/msgs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
package authz_test

import (
stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
"testing"
"time"

"github.com/stretchr/testify/require"

cdctypes "github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/auth/migrations/legacytx"
"github.com/cosmos/cosmos-sdk/x/authz"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
)
Expand Down Expand Up @@ -120,3 +122,65 @@ func TestMsgGrantGetAuthorization(t *testing.T) {
require.NoError(err)
require.Equal(a, &g)
}

func TestAminoJSON(t *testing.T) {
tx := legacytx.StdTx{}
var msg legacytx.LegacyMsg
someDate := time.Date(1, 1, 1, 1, 1, 1, 1, time.UTC)
msgSend := banktypes.MsgSend{FromAddress: "cosmos1ghi", ToAddress: "cosmos1jkl"}
typeURL := sdk.MsgTypeURL(&msgSend)
msgSendAny, err := cdctypes.NewAnyWithValue(&msgSend)
require.NoError(t, err)
grant, err := authz.NewGrant(someDate, authz.NewGenericAuthorization(typeURL), someDate.Add(time.Hour))
require.NoError(t, err)
sendGrant, err := authz.NewGrant(someDate, banktypes.NewSendAuthorization(sdk.NewCoins(sdk.NewCoin("stake", sdk.NewInt(1000)))), someDate.Add(time.Hour))
require.NoError(t, err)
valAddr, err := sdk.ValAddressFromBech32("cosmosvaloper1xcy3els9ua75kdm783c3qu0rfa2eples6eavqq")
require.NoError(t, err)
stakingAuth, err := stakingtypes.NewStakeAuthorization([]sdk.ValAddress{valAddr}, nil, stakingtypes.AuthorizationType_AUTHORIZATION_TYPE_DELEGATE, &sdk.Coin{Denom: "stake", Amount: sdk.NewInt(1000)})
require.NoError(t, err)
delegateGrant, err := authz.NewGrant(someDate, stakingAuth, someDate.Add(time.Hour))
require.NoError(t, err)

// Amino JSON encoding has changed in authz since v0.46.
// Before, it was outputting something like:
// `{"account_number":"1","chain_id":"foo","fee":{"amount":[],"gas":"0"},"memo":"memo","msgs":[{"grant":{"authorization":{"msg":"/cosmos.bank.v1beta1.MsgSend"},"expiration":"0001-01-01T02:01:01.000000001Z"},"grantee":"cosmos1def","granter":"cosmos1abc"}],"sequence":"1","timeout_height":"1"}`
//
// This was a bug. Now, it's as below, See how there's `type` & `value` fields.
// ref: https://github.com/cosmos/cosmos-sdk/issues/11190
// ref: https://github.com/cosmos/cosmjs/issues/1026
msg = &authz.MsgGrant{Granter: "cosmos1abc", Grantee: "cosmos1def", Grant: grant}
tx.Msgs = []sdk.Msg{msg}
require.Equal(t,
`{"account_number":"1","chain_id":"foo","fee":{"amount":[],"gas":"0"},"memo":"memo","msgs":[{"type":"cosmos-sdk/MsgGrant","value":{"grant":{"authorization":{"type":"cosmos-sdk/GenericAuthorization","value":{"msg":"/cosmos.bank.v1beta1.MsgSend"}},"expiration":"0001-01-01T02:01:01.000000001Z"},"grantee":"cosmos1def","granter":"cosmos1abc"}}],"sequence":"1","timeout_height":"1"}`,
string(legacytx.StdSignBytes("foo", 1, 1, 1, legacytx.StdFee{}, []sdk.Msg{msg}, "memo", nil)),
)

msg = &authz.MsgGrant{Granter: "cosmos1abc", Grantee: "cosmos1def", Grant: sendGrant}
tx.Msgs = []sdk.Msg{msg}
require.Equal(t,
`{"account_number":"1","chain_id":"foo","fee":{"amount":[],"gas":"0"},"memo":"memo","msgs":[{"type":"cosmos-sdk/MsgGrant","value":{"grant":{"authorization":{"type":"cosmos-sdk/SendAuthorization","value":{"spend_limit":[{"amount":"1000","denom":"stake"}]}},"expiration":"0001-01-01T02:01:01.000000001Z"},"grantee":"cosmos1def","granter":"cosmos1abc"}}],"sequence":"1","timeout_height":"1"}`,
string(legacytx.StdSignBytes("foo", 1, 1, 1, legacytx.StdFee{}, []sdk.Msg{msg}, "memo", nil)),
)

msg = &authz.MsgGrant{Granter: "cosmos1abc", Grantee: "cosmos1def", Grant: delegateGrant}
tx.Msgs = []sdk.Msg{msg}
require.Equal(t,
`{"account_number":"1","chain_id":"foo","fee":{"amount":[],"gas":"0"},"memo":"memo","msgs":[{"type":"cosmos-sdk/MsgGrant","value":{"grant":{"authorization":{"type":"cosmos-sdk/StakeAuthorization","value":{"Validators":{"type":"cosmos-sdk/StakeAuthorization/AllowList","value":{"allow_list":{"address":["cosmosvaloper1xcy3els9ua75kdm783c3qu0rfa2eples6eavqq"]}}},"authorization_type":1,"max_tokens":{"amount":"1000","denom":"stake"}}},"expiration":"0001-01-01T02:01:01.000000001Z"},"grantee":"cosmos1def","granter":"cosmos1abc"}}],"sequence":"1","timeout_height":"1"}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

`"Validators"

yeah, I don't think there's a way to have snake_case, unless patching Amino, which I really don't want to do

string(legacytx.StdSignBytes("foo", 1, 1, 1, legacytx.StdFee{}, []sdk.Msg{msg}, "memo", nil)),
)

msg = &authz.MsgRevoke{Granter: "cosmos1abc", Grantee: "cosmos1def", MsgTypeUrl: typeURL}
tx.Msgs = []sdk.Msg{msg}
require.Equal(t,
`{"account_number":"1","chain_id":"foo","fee":{"amount":[],"gas":"0"},"memo":"memo","msgs":[{"type":"cosmos-sdk/MsgRevoke","value":{"grantee":"cosmos1def","granter":"cosmos1abc","msg_type_url":"/cosmos.bank.v1beta1.MsgSend"}}],"sequence":"1","timeout_height":"1"}`,
string(legacytx.StdSignBytes("foo", 1, 1, 1, legacytx.StdFee{}, []sdk.Msg{msg}, "memo", nil)),
)

msg = &authz.MsgExec{Grantee: "cosmos1def", Msgs: []*cdctypes.Any{msgSendAny}}
tx.Msgs = []sdk.Msg{msg}
require.Equal(t,
`{"account_number":"1","chain_id":"foo","fee":{"amount":[],"gas":"0"},"memo":"memo","msgs":[{"type":"cosmos-sdk/MsgExec","value":{"grantee":"cosmos1def","msgs":[{"type":"cosmos-sdk/MsgSend","value":{"amount":[],"from_address":"cosmos1ghi","to_address":"cosmos1jkl"}}]}}],"sequence":"1","timeout_height":"1"}`,
string(legacytx.StdSignBytes("foo", 1, 1, 1, legacytx.StdFee{}, []sdk.Msg{msg}, "memo", nil)),
)
}
6 changes: 6 additions & 0 deletions x/bank/types/codec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package types

import (
"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/codec/legacy"
"github.com/cosmos/cosmos-sdk/codec/types"
cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand All @@ -14,6 +15,7 @@ import (
func RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) {
cdc.RegisterConcrete(&MsgSend{}, "cosmos-sdk/MsgSend", nil)
cdc.RegisterConcrete(&MsgMultiSend{}, "cosmos-sdk/MsgMultiSend", nil)
cdc.RegisterConcrete(&SendAuthorization{}, "cosmos-sdk/SendAuthorization", nil)
}

func RegisterInterfaces(registry types.InterfaceRegistry) {
Expand Down Expand Up @@ -45,4 +47,8 @@ func init() {
RegisterLegacyAminoCodec(amino)
cryptocodec.RegisterCrypto(amino)
amino.Seal()

// Register all Amino interfaces and concrete types on the global Amino codec so that this can later be
// used to properly serialize x/authz MsgExec instances
RegisterLegacyAminoCodec(legacy.Cdc)
}
5 changes: 5 additions & 0 deletions x/crisis/types/codec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package types

import (
"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/codec/legacy"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -38,4 +39,8 @@ func init() {
RegisterLegacyAminoCodec(amino)
cryptocodec.RegisterCrypto(amino)
amino.Seal()

// Register all Amino interfaces and concrete types on the global Amino codec so that this can later be
// used to properly serialize x/authz MsgExec instances
RegisterLegacyAminoCodec(legacy.Cdc)
}
5 changes: 5 additions & 0 deletions x/distribution/types/codec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package types

import (
"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/codec/legacy"
"github.com/cosmos/cosmos-sdk/codec/types"
cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -51,4 +52,8 @@ func init() {
RegisterLegacyAminoCodec(amino)
cryptocodec.RegisterCrypto(amino)
amino.Seal()

// Register all Amino interfaces and concrete types on the global Amino codec so that this can later be
// used to properly serialize x/authz MsgExec instances
RegisterLegacyAminoCodec(legacy.Cdc)
}
5 changes: 5 additions & 0 deletions x/evidence/types/codec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package types

import (
"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/codec/legacy"
"github.com/cosmos/cosmos-sdk/codec/types"
cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -44,4 +45,8 @@ func init() {
RegisterLegacyAminoCodec(amino)
cryptocodec.RegisterCrypto(amino)
amino.Seal()

// Register all Amino interfaces and concrete types on the global Amino codec so that this can later be
// used to properly serialize x/authz MsgExec instances
RegisterLegacyAminoCodec(legacy.Cdc)
}
34 changes: 34 additions & 0 deletions x/feegrant/codec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,25 @@
package feegrant

import (
"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/codec/legacy"
"github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/msgservice"
)

// RegisterLegacyAminoCodec registers the necessary x/feegrant interfaces and concrete types
// on the provided LegacyAmino codec. These types are used for Amino JSON serialization.
func RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) {
cdc.RegisterConcrete(&MsgGrantAllowance{}, "cosmos-sdk/MsgGrantAllowance", nil)
cdc.RegisterConcrete(&MsgRevokeAllowance{}, "cosmos-sdk/MsgRevokeAllowance", nil)

cdc.RegisterInterface((*FeeAllowanceI)(nil), nil)
cdc.RegisterConcrete(&BasicAllowance{}, "cosmos-sdk/BasicAllowance", nil)
cdc.RegisterConcrete(&PeriodicAllowance{}, "cosmos-sdk/PeriodicAllowance", nil)
cdc.RegisterConcrete(&AllowedMsgAllowance{}, "cosmos-sdk/AllowedMsgAllowance", nil)
}

// RegisterInterfaces registers the interfaces types with the interface registry
func RegisterInterfaces(registry types.InterfaceRegistry) {
registry.RegisterImplementations((*sdk.Msg)(nil),
Expand All @@ -23,3 +37,23 @@ func RegisterInterfaces(registry types.InterfaceRegistry) {

msgservice.RegisterMsgServiceDesc(registry, &_Msg_serviceDesc)
}

var (
amino = codec.NewLegacyAmino()

// ModuleCdc references the global x/feegrant module codec. Note, the codec should
// ONLY be used in certain instances of tests and for JSON encoding as Amino is
// still used for that purpose.
//
// The actual codec used for serialization should be provided to x/feegrant and
// defined at the application level.
ModuleCdc = codec.NewAminoCodec(amino)
)
Comment on lines +40 to +51
Copy link
Contributor

@amaury1093 amaury1093 Feb 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
var (
amino = codec.NewLegacyAmino()
// ModuleCdc references the global x/feegrant module codec. Note, the codec should
// ONLY be used in certain instances of tests and for JSON encoding as Amino is
// still used for that purpose.
//
// The actual codec used for serialization should be provided to x/feegrant and
// defined at the application level.
ModuleCdc = codec.NewAminoCodec(amino)
)

Is that correct? Come to think about it, we can remove ModuleCdc globals from all modules, right? (we can do in a separate PR, so that this one focused on authz/feegrant)

Edit: we can do this in #11232


func init() {
RegisterLegacyAminoCodec(amino)

// Register all Amino interfaces and concrete types on the global Amino codec so that this can later be
// used to properly serialize x/authz MsgExec instances
RegisterLegacyAminoCodec(legacy.Cdc)
}
1 change: 1 addition & 0 deletions x/feegrant/module/module.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ func (am AppModule) RegisterServices(cfg module.Configurator) {

// RegisterLegacyAminoCodec registers the feegrant module's types for the given codec.
func (AppModuleBasic) RegisterLegacyAminoCodec(cdc *codec.LegacyAmino) {
feegrant.RegisterLegacyAminoCodec(cdc)
}

// RegisterInterfaces registers the feegrant module's interface types
Expand Down
5 changes: 2 additions & 3 deletions x/feegrant/msgs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package feegrant
import (
"github.com/gogo/protobuf/proto"

"github.com/cosmos/cosmos-sdk/codec/legacy"
"github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
Expand Down Expand Up @@ -73,7 +72,7 @@ func (msg MsgGrantAllowance) Route() string {

// GetSignBytes implements the LegacyMsg.GetSignBytes method.
func (msg MsgGrantAllowance) GetSignBytes() []byte {
return sdk.MustSortJSON(legacy.Cdc.MustMarshalJSON(&msg))
return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&msg))
}

// GetFeeAllowanceI returns unpacked FeeAllowance
Expand Down Expand Up @@ -133,5 +132,5 @@ func (msg MsgRevokeAllowance) Route() string {

// GetSignBytes implements the LegacyMsg.GetSignBytes method.
func (msg MsgRevokeAllowance) GetSignBytes() []byte {
return sdk.MustSortJSON(legacy.Cdc.MustMarshalJSON(&msg))
return sdk.MustSortJSON(ModuleCdc.MustMarshalJSON(&msg))
}
29 changes: 29 additions & 0 deletions x/feegrant/msgs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/cosmos/cosmos-sdk/codec"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/auth/migrations/legacytx"
"github.com/cosmos/cosmos-sdk/x/feegrant"
)

Expand Down Expand Up @@ -132,3 +133,31 @@ func TestMsgRevokeAllowance(t *testing.T) {
}
}
}

func TestAminoJSON(t *testing.T) {
tx := legacytx.StdTx{}
var msg legacytx.LegacyMsg
allowanceAny, err := codectypes.NewAnyWithValue(&feegrant.BasicAllowance{SpendLimit: sdk.NewCoins(sdk.NewCoin("foo", sdk.NewInt(100)))})
require.NoError(t, err)

// Amino JSON encoding has changed in feegrant since v0.46.
// Before, it was outputting something like:
// `{"account_number":"1","chain_id":"foo","fee":{"amount":[],"gas":"0"},"memo":"memo","msgs":[{"allowance":{"spend_limit":[{"amount":"100","denom":"foo"}]},"grantee":"cosmos1def","granter":"cosmos1abc"}],"sequence":"1","timeout_height":"1"}`
//
// This was a bug. Now, it's as below, See how there's `type` & `value` fields.
// ref: https://github.com/cosmos/cosmos-sdk/issues/11190
// ref: https://github.com/cosmos/cosmjs/issues/1026
msg = &feegrant.MsgGrantAllowance{Granter: "cosmos1abc", Grantee: "cosmos1def", Allowance: allowanceAny}
tx.Msgs = []sdk.Msg{msg}
require.Equal(t,
`{"account_number":"1","chain_id":"foo","fee":{"amount":[],"gas":"0"},"memo":"memo","msgs":[{"type":"cosmos-sdk/MsgGrantAllowance","value":{"allowance":{"type":"cosmos-sdk/BasicAllowance","value":{"spend_limit":[{"amount":"100","denom":"foo"}]}},"grantee":"cosmos1def","granter":"cosmos1abc"}}],"sequence":"1","timeout_height":"1"}`,
string(legacytx.StdSignBytes("foo", 1, 1, 1, legacytx.StdFee{}, []sdk.Msg{msg}, "memo", nil)),
)

msg = &feegrant.MsgRevokeAllowance{Granter: "cosmos1abc", Grantee: "cosmos1def"}
tx.Msgs = []sdk.Msg{msg}
require.Equal(t,
`{"account_number":"1","chain_id":"foo","fee":{"amount":[],"gas":"0"},"memo":"memo","msgs":[{"type":"cosmos-sdk/MsgRevokeAllowance","value":{"grantee":"cosmos1def","granter":"cosmos1abc"}}],"sequence":"1","timeout_height":"1"}`,
string(legacytx.StdSignBytes("foo", 1, 1, 1, legacytx.StdFee{}, []sdk.Msg{msg}, "memo", nil)),
)
}
5 changes: 5 additions & 0 deletions x/gov/types/v1beta1/codec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package v1beta1

import (
"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/codec/legacy"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/msgservice"
Expand Down Expand Up @@ -47,4 +48,8 @@ func RegisterProposalTypeCodec(o interface{}, name string) {

func init() {
RegisterLegacyAminoCodec(types.ModuleCdc.LegacyAmino)

// Register all Amino interfaces and concrete types on the global Amino codec so that this can later be
// used to properly serialize x/authz MsgExec instances
RegisterLegacyAminoCodec(legacy.Cdc)
}
Loading