From c8245d1d0cebf67d001b2bf4eb664f4adc699c9c Mon Sep 17 00:00:00 2001 From: Shogo Hyodo Date: Mon, 17 Oct 2022 16:05:54 +0900 Subject: [PATCH 1/7] Remove potential runtime panic --- x/feegrant/filtered_fee.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/x/feegrant/filtered_fee.go b/x/feegrant/filtered_fee.go index 290e111914..3545096807 100644 --- a/x/feegrant/filtered_fee.go +++ b/x/feegrant/filtered_fee.go @@ -53,11 +53,14 @@ func (a *AllowedMsgAllowance) GetAllowance() (FeeAllowanceI, error) { // SetAllowance sets allowed fee allowance. func (a *AllowedMsgAllowance) SetAllowance(allowance FeeAllowanceI) error { var err error - a.Allowance, err = types.NewAnyWithValue(allowance.(proto.Message)) - if err != nil { + protoAllowance, ok := allowance.(proto.Message) + if !ok || protoAllowance == nil { return sdkerrors.Wrapf(sdkerrors.ErrPackAny, "cannot proto marshal %T", allowance) } - + a.Allowance, err = types.NewAnyWithValue(protoAllowance) + if err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrPackAny, "cannot proto marshal %T", protoAllowance) + } return nil } From e8472e006658c9d4647668dfb947a57d4aeafbc8 Mon Sep 17 00:00:00 2001 From: Shogo Hyodo Date: Mon, 17 Oct 2022 16:31:11 +0900 Subject: [PATCH 2/7] Update `CHANGELOG.md` --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index dcf04b11c2..b2e9620b2d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -98,6 +98,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/foundation) [\#687](https://github.com/line/lbm-sdk/pull/687) fix bugs on aborting x/foundation proposals * (global) [\#694](https://github.com/line/lbm-sdk/pull/694) replace deprecated functions since go 1.16 or 1.17 * (x/bankplus) [\#705](https://github.com/line/lbm-sdk/pull/705) add missing blockedAddr checking in bankplus +* (x/feegrant) [\#720](https://github.com/line/lbm-sdk/pull/720) remove potential runtime panic in x/feegrant ### Breaking Changes * (proto) [\#564](https://github.com/line/lbm-sdk/pull/564) change gRPC path to original cosmos path From 0527c7471fa0c2b02475271a7b090c3f23f454a2 Mon Sep 17 00:00:00 2001 From: Shogo Hyodo Date: Mon, 17 Oct 2022 19:16:51 +0900 Subject: [PATCH 3/7] Fix for codecov --- x/feegrant/filtered_fee.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/feegrant/filtered_fee.go b/x/feegrant/filtered_fee.go index 3545096807..378c8f7bd7 100644 --- a/x/feegrant/filtered_fee.go +++ b/x/feegrant/filtered_fee.go @@ -54,7 +54,7 @@ func (a *AllowedMsgAllowance) GetAllowance() (FeeAllowanceI, error) { func (a *AllowedMsgAllowance) SetAllowance(allowance FeeAllowanceI) error { var err error protoAllowance, ok := allowance.(proto.Message) - if !ok || protoAllowance == nil { + if !ok { return sdkerrors.Wrapf(sdkerrors.ErrPackAny, "cannot proto marshal %T", allowance) } a.Allowance, err = types.NewAnyWithValue(protoAllowance) From ee66ae7582dd8082f656b7ae0a0f9cee5b947fe1 Mon Sep 17 00:00:00 2001 From: Shogo Hyodo Date: Tue, 18 Oct 2022 15:13:03 +0900 Subject: [PATCH 4/7] Add test --- x/feegrant/filtered_fee_test.go | 45 +++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/x/feegrant/filtered_fee_test.go b/x/feegrant/filtered_fee_test.go index 6aee743a44..b4e8b3672a 100644 --- a/x/feegrant/filtered_fee_test.go +++ b/x/feegrant/filtered_fee_test.go @@ -188,3 +188,48 @@ func TestFilteredFeeValidAllow(t *testing.T) { }) } } + +type invalidAllowance struct { +} + +// compilation time interface implementation check +var _ feegrant.FeeAllowanceI = (*invalidAllowance)(nil) + +func (i invalidAllowance) Accept(ctx sdk.Context, fee sdk.Coins, msgs []sdk.Msg) (remove bool, err error) { + return false, nil +} + +func (i invalidAllowance) ValidateBasic() error { + return nil +} + +func TestSetAllowance(t *testing.T) { + cases := map[string]struct { + allowance feegrant.FeeAllowanceI + valid bool + }{ + "valid allowance": { + allowance: &feegrant.BasicAllowance{}, + valid: true, + }, + "invalid allowance": { + allowance: &invalidAllowance{}, + valid: false, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + allowance := &feegrant.BasicAllowance{} + msgs := []string{sdk.MsgTypeURL(&banktypes.MsgSend{})} + allowed, err := feegrant.NewAllowedMsgAllowance(allowance, msgs) + require.NoError(t, err) + require.NotNil(t, allowed) + if tc.valid { + require.NoError(t, allowed.SetAllowance(tc.allowance)) + } else { + require.Error(t, allowed.SetAllowance(tc.allowance)) + } + }) + } +} From 2a80a60a74485f8aa0ef6255a977c27bdf12ea79 Mon Sep 17 00:00:00 2001 From: Shogo Hyodo Date: Tue, 18 Oct 2022 18:51:02 +0900 Subject: [PATCH 5/7] Fix for reviews --- x/feegrant/filtered_fee_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/x/feegrant/filtered_fee_test.go b/x/feegrant/filtered_fee_test.go index b4e8b3672a..102c4ba354 100644 --- a/x/feegrant/filtered_fee_test.go +++ b/x/feegrant/filtered_fee_test.go @@ -189,6 +189,7 @@ func TestFilteredFeeValidAllow(t *testing.T) { } } +// invalidAllowance does not implement proto.Message. type invalidAllowance struct { } @@ -225,10 +226,11 @@ func TestSetAllowance(t *testing.T) { allowed, err := feegrant.NewAllowedMsgAllowance(allowance, msgs) require.NoError(t, err) require.NotNil(t, allowed) + err = allowed.SetAllowance(tc.allowance) if tc.valid { - require.NoError(t, allowed.SetAllowance(tc.allowance)) + require.NoError(t, err) } else { - require.Error(t, allowed.SetAllowance(tc.allowance)) + require.Error(t, err) } }) } From ff6d3fd6b8a712e9873d4ef0d05fa09d02fe7bea Mon Sep 17 00:00:00 2001 From: Shogo Hyodo Date: Wed, 19 Oct 2022 11:42:22 +0900 Subject: [PATCH 6/7] Add test case --- x/feegrant/filtered_fee_test.go | 35 +++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/x/feegrant/filtered_fee_test.go b/x/feegrant/filtered_fee_test.go index 102c4ba354..8b3b413abf 100644 --- a/x/feegrant/filtered_fee_test.go +++ b/x/feegrant/filtered_fee_test.go @@ -4,6 +4,7 @@ import ( "testing" "time" + proto "github.com/gogo/protobuf/proto" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -189,7 +190,7 @@ func TestFilteredFeeValidAllow(t *testing.T) { } } -// invalidAllowance does not implement proto.Message. +// invalidAllowance does not implement proto.Message type invalidAllowance struct { } @@ -204,6 +205,32 @@ func (i invalidAllowance) ValidateBasic() error { return nil } +// invalidAllowance2 does not have field +type invalidAllowance2 struct { +} + +// compilation time interface implementation check +var _ feegrant.FeeAllowanceI = (*invalidAllowance2)(nil) +var _ proto.Message = (*invalidAllowance2)(nil) + +func (i invalidAllowance2) Accept(ctx sdk.Context, fee sdk.Coins, msgs []sdk.Msg) (remove bool, err error) { + return false, nil +} + +func (i invalidAllowance2) ValidateBasic() error { + return nil +} + +func (i invalidAllowance2) Reset() { +} + +func (i invalidAllowance2) String() string { + return "" +} + +func (i invalidAllowance2) ProtoMessage() { +} + func TestSetAllowance(t *testing.T) { cases := map[string]struct { allowance feegrant.FeeAllowanceI @@ -213,10 +240,14 @@ func TestSetAllowance(t *testing.T) { allowance: &feegrant.BasicAllowance{}, valid: true, }, - "invalid allowance": { + "allowance without proto.Message": { allowance: &invalidAllowance{}, valid: false, }, + "allowance without fields": { + allowance: (*invalidAllowance2)(nil), + valid: false, + }, } for name, tc := range cases { From d3573e9b9b98078c2917001ec7f85b17699acbb7 Mon Sep 17 00:00:00 2001 From: Shogo Hyodo Date: Wed, 19 Oct 2022 14:31:25 +0900 Subject: [PATCH 7/7] Fix names --- x/feegrant/filtered_fee_test.go | 41 ++++++++++++++------------------- 1 file changed, 17 insertions(+), 24 deletions(-) diff --git a/x/feegrant/filtered_fee_test.go b/x/feegrant/filtered_fee_test.go index 8b3b413abf..bffce51a1c 100644 --- a/x/feegrant/filtered_fee_test.go +++ b/x/feegrant/filtered_fee_test.go @@ -190,45 +190,38 @@ func TestFilteredFeeValidAllow(t *testing.T) { } } -// invalidAllowance does not implement proto.Message -type invalidAllowance struct { +// invalidInterfaceAllowance does not implement proto.Message +type invalidInterfaceAllowance struct { } // compilation time interface implementation check -var _ feegrant.FeeAllowanceI = (*invalidAllowance)(nil) +var _ feegrant.FeeAllowanceI = (*invalidInterfaceAllowance)(nil) -func (i invalidAllowance) Accept(ctx sdk.Context, fee sdk.Coins, msgs []sdk.Msg) (remove bool, err error) { +func (i invalidInterfaceAllowance) Accept(ctx sdk.Context, fee sdk.Coins, msgs []sdk.Msg) (remove bool, err error) { return false, nil } -func (i invalidAllowance) ValidateBasic() error { +func (i invalidInterfaceAllowance) ValidateBasic() error { return nil } -// invalidAllowance2 does not have field -type invalidAllowance2 struct { +// invalidProtoAllowance can not run proto.Marshal +type invalidProtoAllowance struct { + invalidInterfaceAllowance } // compilation time interface implementation check -var _ feegrant.FeeAllowanceI = (*invalidAllowance2)(nil) -var _ proto.Message = (*invalidAllowance2)(nil) +var _ feegrant.FeeAllowanceI = (*invalidProtoAllowance)(nil) +var _ proto.Message = (*invalidProtoAllowance)(nil) -func (i invalidAllowance2) Accept(ctx sdk.Context, fee sdk.Coins, msgs []sdk.Msg) (remove bool, err error) { - return false, nil -} - -func (i invalidAllowance2) ValidateBasic() error { - return nil -} - -func (i invalidAllowance2) Reset() { +func (i invalidProtoAllowance) Reset() { } -func (i invalidAllowance2) String() string { +func (i invalidProtoAllowance) String() string { return "" } -func (i invalidAllowance2) ProtoMessage() { +func (i invalidProtoAllowance) ProtoMessage() { } func TestSetAllowance(t *testing.T) { @@ -240,12 +233,12 @@ func TestSetAllowance(t *testing.T) { allowance: &feegrant.BasicAllowance{}, valid: true, }, - "allowance without proto.Message": { - allowance: &invalidAllowance{}, + "invalid interface allowance": { + allowance: &invalidInterfaceAllowance{}, valid: false, }, - "allowance without fields": { - allowance: (*invalidAllowance2)(nil), + "empty allowance": { + allowance: (*invalidProtoAllowance)(nil), valid: false, }, }