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

x/feegrant audit: clean up / add test coverage to types package #9193

Merged
merged 9 commits into from
Apr 28, 2021
Merged

x/feegrant audit: clean up / add test coverage to types package #9193

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
86579f2
Squashed commit of the following:
technicallyty Apr 23, 2021
441a18c
fix imports
technicallyty Apr 23, 2021
434774c
Merge branch 'master' into ty-9115-cleanup_types
technicallyty Apr 26, 2021
3e6b751
fix tests
technicallyty Apr 26, 2021
6bde6b7
rename test field
technicallyty Apr 27, 2021
ce77bbd
Merge branch 'master' into ty-9115-cleanup_types
technicallyty Apr 27, 2021
9ddb406
Merge branch 'master' into ty-9115-cleanup_types
technicallyty Apr 27, 2021
882c3a5
Merge branch 'master' into ty-9115-cleanup_types
blushi Apr 28, 2021
a92d72a
Merge branch 'master' into ty-9115-cleanup_types
aleem1314 Apr 28, 2021
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
41 changes: 22 additions & 19 deletions x/feegrant/client/testutil/suite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -629,7 +629,7 @@ func (s *IntegrationTestSuite) TestFilteredFeeAllowance() {
}
spendLimit := sdk.NewCoin("stake", sdk.NewInt(1000))

allowMsgs := "/cosmos.gov.v1beta1.Msg/SubmitProposal"
allowMsgs := "/cosmos.gov.v1beta1.Msg/SubmitProposal,weighted_vote"

testCases := []struct {
name string
Expand All @@ -639,10 +639,10 @@ func (s *IntegrationTestSuite) TestFilteredFeeAllowance() {
expectedCode uint32
}{
{
"wrong granter",
"invalid granter address",
append(
[]string{
"wrong granter",
"not an address",
"cosmos1nph3cfzk6trsmfxkeu943nvach5qw4vwstnvkl",
fmt.Sprintf("--%s=%s", cli.FlagAllowedMsgs, allowMsgs),
fmt.Sprintf("--%s=%s", cli.FlagSpendLimit, spendLimit.String()),
Expand All @@ -653,11 +653,11 @@ func (s *IntegrationTestSuite) TestFilteredFeeAllowance() {
true, &sdk.TxResponse{}, 0,
},
{
"wrong grantee",
"invalid grantee address",
append(
[]string{
granter.String(),
"wrong grantee",
"not an address",
fmt.Sprintf("--%s=%s", cli.FlagAllowedMsgs, allowMsgs),
fmt.Sprintf("--%s=%s", cli.FlagSpendLimit, spendLimit.String()),
fmt.Sprintf("--%s=%s", flags.FlagFrom, granter),
Expand Down Expand Up @@ -733,22 +733,30 @@ func (s *IntegrationTestSuite) TestFilteredFeeAllowance() {
cases := []struct {
name string
malleate func() (testutil.BufferWriter, error)
expectErr bool
respType proto.Message
expectedCode uint32
}{
{
"valid tx",
"valid proposal tx",
func() (testutil.BufferWriter, error) {
return govtestutil.MsgSubmitProposal(val.ClientCtx, grantee.String(),
"Text Proposal", "No desc", govtypes.ProposalTypeText,
fmt.Sprintf("--%s=%s", flags.FlagFeeAccount, granter.String()),
)
},
false,
&sdk.TxResponse{},
0,
},
{
"valid weighted_vote tx",
func() (testutil.BufferWriter, error) {
return govtestutil.MsgVote(val.ClientCtx, grantee.String(), "0", "yes",
fmt.Sprintf("--%s=%s", flags.FlagFeeAccount, granter.String()),
)
},
&sdk.TxResponse{},
2,
},
{
"should fail with unauthorized msgs",
func() (testutil.BufferWriter, error) {
Expand All @@ -764,7 +772,8 @@ func (s *IntegrationTestSuite) TestFilteredFeeAllowance() {
cmd := cli.NewCmdFeeGrant()
return clitestutil.ExecTestCLICmd(clientCtx, cmd, args)
},
false, &sdk.TxResponse{}, 7,
&sdk.TxResponse{},
7,
},
}

Expand All @@ -773,16 +782,10 @@ func (s *IntegrationTestSuite) TestFilteredFeeAllowance() {

s.Run(tc.name, func() {
out, err := tc.malleate()

if tc.expectErr {
s.Require().Error(err)
} else {
s.Require().NoError(err)
s.Require().NoError(clientCtx.JSONMarshaler.UnmarshalJSON(out.Bytes(), tc.respType), out.String())

txResp := tc.respType.(*sdk.TxResponse)
s.Require().Equal(tc.expectedCode, txResp.Code, out.String())
}
s.Require().NoError(err)
s.Require().NoError(clientCtx.JSONMarshaler.UnmarshalJSON(out.Bytes(), tc.respType), out.String())
txResp := tc.respType.(*sdk.TxResponse)
s.Require().Equal(tc.expectedCode, txResp.Code, out.String())
})
}
}
14 changes: 7 additions & 7 deletions x/feegrant/simulation/operations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,24 +68,24 @@ func SimulateMsgGrantFeeAllowance(ak types.AccountKeeper, bk types.BankKeeper, k
granter, _ := simtypes.RandomAcc(r, accs)
grantee, _ := simtypes.RandomAcc(r, accs)
if grantee.Address.String() == granter.Address.String() {
return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgGrantFeeAllowance, "grantee and granter cannot be same"), nil, nil
return simtypes.NoOpMsg(types.ModuleName, TypeMsgGrantFeeAllowance, "grantee and granter cannot be same"), nil, nil
}

if f, _ := k.GetFeeAllowance(ctx, granter.Address, grantee.Address); f != nil {
return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgGrantFeeAllowance, "fee allowance exists"), nil, nil
return simtypes.NoOpMsg(types.ModuleName, TypeMsgGrantFeeAllowance, "fee allowance exists"), nil, nil
}

account := ak.GetAccount(ctx, granter.Address)

spendableCoins := bk.SpendableCoins(ctx, account.GetAddress())
fees, err := simtypes.RandomFees(r, ctx, spendableCoins)
if err != nil {
return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgGrantFeeAllowance, err.Error()), nil, err
return simtypes.NoOpMsg(types.ModuleName, TypeMsgGrantFeeAllowance, err.Error()), nil, err
}

spendableCoins = spendableCoins.Sub(fees)
if spendableCoins.Empty() {
return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgGrantFeeAllowance, "unable to grant empty coins as SpendLimit"), nil, nil
return simtypes.NoOpMsg(types.ModuleName, TypeMsgGrantFeeAllowance, "unable to grant empty coins as SpendLimit"), nil, nil
}

msg, err := types.NewMsgGrantFeeAllowance(&types.BasicFeeAllowance{
Expand All @@ -94,14 +94,14 @@ func SimulateMsgGrantFeeAllowance(ak types.AccountKeeper, bk types.BankKeeper, k
}, granter.Address, grantee.Address)

if err != nil {
return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgGrantFeeAllowance, err.Error()), nil, err
return simtypes.NoOpMsg(types.ModuleName, TypeMsgGrantFeeAllowance, err.Error()), nil, err
}
txGen := simappparams.MakeTestEncodingConfig().TxConfig
svcMsgClientConn := &msgservice.ServiceMsgClientConn{}
feegrantMsgClient := types.NewMsgClient(svcMsgClientConn)
_, err = feegrantMsgClient.GrantFeeAllowance(context.Background(), msg)
if err != nil {
return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgGrantFeeAllowance, err.Error()), nil, err
return simtypes.NoOpMsg(types.ModuleName, TypeMsgGrantFeeAllowance, err.Error()), nil, err
}
tx, err := helpers.GenTx(
txGen,
Expand Down Expand Up @@ -175,7 +175,7 @@ func SimulateMsgRevokeFeeAllowance(ak types.AccountKeeper, bk types.BankKeeper,
feegrantMsgClient := types.NewMsgClient(svcMsgClientConn)
_, err = feegrantMsgClient.RevokeFeeAllowance(context.Background(), &msg)
if err != nil {
return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgGrantFeeAllowance, err.Error()), nil, err
return simtypes.NoOpMsg(types.ModuleName, TypeMsgGrantFeeAllowance, err.Error()), nil, err
}

tx, err := helpers.GenTx(
Expand Down
46 changes: 16 additions & 30 deletions x/feegrant/types/basic_fee_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,94 +22,84 @@ func TestBasicFeeValidAllow(t *testing.T) {
leftAtom := sdk.NewCoins(sdk.NewInt64Coin("atom", 512))

cases := map[string]struct {
allow *types.BasicFeeAllowance
allowance *types.BasicFeeAllowance
// all other checks are ignored if valid=false
fee sdk.Coins
blockHeight int64
valid bool
accept bool
remove bool
remove bool
remains sdk.Coins
}{
"empty": {
allow: &types.BasicFeeAllowance{},
valid: true,
allowance: &types.BasicFeeAllowance{},
accept: true,
},
"small fee without expire": {
allow: &types.BasicFeeAllowance{
allowance: &types.BasicFeeAllowance{
SpendLimit: atom,
},
valid: true,
fee: smallAtom,
accept: true,
remove: false,
remains: leftAtom,
},
"all fee without expire": {
allow: &types.BasicFeeAllowance{
allowance: &types.BasicFeeAllowance{
SpendLimit: smallAtom,
},
valid: true,
fee: smallAtom,
accept: true,
remove: true,
},
"wrong fee": {
allow: &types.BasicFeeAllowance{
allowance: &types.BasicFeeAllowance{
SpendLimit: smallAtom,
},
valid: true,
fee: eth,
accept: false,
},
"non-expired": {
allow: &types.BasicFeeAllowance{
allowance: &types.BasicFeeAllowance{
SpendLimit: atom,
Expiration: types.ExpiresAtHeight(100),
},
valid: true,
fee: smallAtom,
blockHeight: 85,
accept: true,
remove: false,
remains: leftAtom,
},
"expired": {
allow: &types.BasicFeeAllowance{
allowance: &types.BasicFeeAllowance{
SpendLimit: atom,
Expiration: types.ExpiresAtHeight(100),
},
valid: true,
fee: smallAtom,
blockHeight: 121,
accept: false,
remove: true,
},
"fee more than allowed": {
allow: &types.BasicFeeAllowance{
allowance: &types.BasicFeeAllowance{
SpendLimit: atom,
Expiration: types.ExpiresAtHeight(100),
},
valid: true,
fee: bigAtom,
blockHeight: 85,
accept: false,
},
"with out spend limit": {
allow: &types.BasicFeeAllowance{
allowance: &types.BasicFeeAllowance{
Expiration: types.ExpiresAtHeight(100),
},
valid: true,
fee: bigAtom,
blockHeight: 85,
accept: true,
},
"expired no spend limit": {
allow: &types.BasicFeeAllowance{
allowance: &types.BasicFeeAllowance{
Expiration: types.ExpiresAtHeight(100),
},
valid: true,
fee: bigAtom,
blockHeight: 120,
accept: false,
Expand All @@ -119,26 +109,22 @@ func TestBasicFeeValidAllow(t *testing.T) {
for name, stc := range cases {
tc := stc // to make scopelint happy
t.Run(name, func(t *testing.T) {
err := tc.allow.ValidateBasic()
if !tc.valid {
require.Error(t, err)
return
}
err := tc.allowance.ValidateBasic()
require.NoError(t, err)

ctx := app.BaseApp.NewContext(false, tmproto.Header{}).WithBlockHeight(tc.blockHeight)

// now try to deduct
remove, err := tc.allow.Accept(ctx, tc.fee, []sdk.Msg{})
removed, err := tc.allowance.Accept(ctx, tc.fee, []sdk.Msg{})
if !tc.accept {
require.Error(t, err)
return
}
require.NoError(t, err)

require.Equal(t, tc.remove, remove)
if !remove {
assert.Equal(t, tc.allow.SpendLimit, tc.remains)
require.Equal(t, tc.remove, removed)
if !removed {
assert.Equal(t, tc.allowance.SpendLimit, tc.remains)
}
})
}
Expand Down
9 changes: 0 additions & 9 deletions x/feegrant/types/expiration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,32 +15,27 @@ func TestExpiresAt(t *testing.T) {

cases := map[string]struct {
example types.ExpiresAt
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe rename it to expiresAt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ExpiresAt is a proto message, we usually have these exported

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant rename the struct field name example into expiresAt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, changed it to expires to be consistent with the other tests in the file

valid bool
zero bool
before types.ExpiresAt
after types.ExpiresAt
}{
"basic": {
example: types.ExpiresAtHeight(100),
valid: true,
before: types.ExpiresAtHeight(50),
after: types.ExpiresAtHeight(122),
},
"zero": {
example: types.ExpiresAt{},
zero: true,
valid: true,
before: types.ExpiresAtHeight(1),
},
"match height": {
example: types.ExpiresAtHeight(1000),
valid: true,
before: types.ExpiresAtHeight(999),
after: types.ExpiresAtHeight(1000),
},
"match time": {
example: types.ExpiresAtTime(now),
valid: true,
before: types.ExpiresAtTime(now.Add(-1 * time.Second)),
after: types.ExpiresAtTime(now.Add(1 * time.Second)),
},
Expand All @@ -51,10 +46,6 @@ func TestExpiresAt(t *testing.T) {
t.Run(name, func(t *testing.T) {
err := tc.example.ValidateBasic()
assert.Equal(t, tc.zero, tc.example.Undefined())
if !tc.valid {
require.Error(t, err)
return
}
require.NoError(t, err)

if !tc.before.Undefined() {
Expand Down
3 changes: 1 addition & 2 deletions x/feegrant/types/filtered_fee.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,10 @@ package types
import (
"time"

proto "github.com/gogo/protobuf/proto"

"github.com/cosmos/cosmos-sdk/codec/types"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/gogo/protobuf/proto"
)

// TODO: Revisit this once we have propoer gas fee framework.
Expand Down
Loading