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

chore: refactor transfer authz tests to use table tests #2998

Merged
merged 15 commits into from
Jan 16, 2023
Merged

chore: refactor transfer authz tests to use table tests #2998

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
0034dc3
moving authz protos to transfer.v1
damiannolan Jan 9, 2023
ac2f8f4
running make format
damiannolan Jan 9, 2023
b048ef4
rename allow list and allocation protobuf types
damiannolan Jan 9, 2023
6b680ae
resolving conflicts
damiannolan Jan 9, 2023
b18fdd7
use fully qualified interface name in authz.proto
damiannolan Jan 10, 2023
6c62a4f
resolving conflicts
damiannolan Jan 10, 2023
eda45ed
adding validate basic tests
damiannolan Jan 10, 2023
ae86469
if allow list is empty, permit any receiver
damiannolan Jan 10, 2023
5d744aa
Merge branch 'feat/authz-ics20' into damian/2960-authz-validate-basic
damiannolan Jan 11, 2023
5b89314
test formatting
damiannolan Jan 11, 2023
5e97377
Merge branch 'feat/authz-ics20' into damian/2960-authz-validate-basic
damiannolan Jan 11, 2023
cb68bd2
test setup for transfer authz accept
damiannolan Jan 11, 2023
2a661e8
refactoring unit tests
damiannolan Jan 11, 2023
87af310
Merge branch 'feat/authz-ics20' into damian/2961-authz-transfer-testing
damiannolan Jan 12, 2023
255281e
apply suggestions from review
damiannolan Jan 16, 2023
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
253 changes: 141 additions & 112 deletions modules/apps/transfer/types/transfer_authorization_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,132 +1,161 @@
package types_test

import (
"testing"

"github.com/stretchr/testify/require"
tmproto "github.com/tendermint/tendermint/proto/tendermint/types"

"github.com/cosmos/cosmos-sdk/simapp"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/authz"

"github.com/cosmos/ibc-go/v6/modules/apps/transfer/types"
clienttypes "github.com/cosmos/ibc-go/v6/modules/core/02-client/types"
ibctesting "github.com/cosmos/ibc-go/v6/testing"
"github.com/cosmos/ibc-go/v6/testing/mock"
)

var (
sourcePort = "port"
sourceChannel = "channel-100"
sourcePort2 = "port2"
sourceChannel2 = "channel-101"
coins1000 = sdk.Coins{sdk.NewCoin("stake", sdk.NewInt(1000))}
coins500 = sdk.Coins{sdk.NewCoin("stake", sdk.NewInt(500))}
coin1000 = sdk.NewCoin("stake", sdk.NewInt(1000))
coin500 = sdk.NewCoin("stake", sdk.NewInt(500))
fromAddr = sdk.AccAddress("_____from _____")
toAddr = sdk.AccAddress("_______to________")
timeoutHeight = clienttypes.NewHeight(0, 10)
)
func (suite *TypesTestSuite) TestTransferAuthorizationAccept() {
var (
msgTransfer types.MsgTransfer
transferAuthz types.TransferAuthorization
)

func TestTransferAuthorization(t *testing.T) {
app := simapp.Setup(t, false)
ctx := app.BaseApp.NewContext(false, tmproto.Header{})
allocation := types.Allocation{
SourcePort: sourcePort,
SourceChannel: sourceChannel,
SpendLimit: coins1000,
AllowList: []string{toAddr.String()},
}
authorization := types.NewTransferAuthorization(allocation)

t.Log("verify authorization returns valid method name")
require.Equal(t, authorization.MsgTypeURL(), "/ibc.applications.transfer.v1.MsgTransfer")
require.NoError(t, authorization.ValidateBasic())
transfer := types.NewMsgTransfer(sourcePort, sourceChannel, coin1000, fromAddr.String(), toAddr.String(), timeoutHeight, 0, "")
require.NoError(t, authorization.ValidateBasic())

t.Log("verify updated authorization returns nil")
resp, err := authorization.Accept(ctx, transfer)
require.NoError(t, err)
require.True(t, resp.Delete)
require.Nil(t, resp.Updated)

t.Log("verify updated authorization returns remaining spent limit")
authorization = types.NewTransferAuthorization(allocation)
require.Equal(t, authorization.MsgTypeURL(), "/ibc.applications.transfer.v1.MsgTransfer")
require.NoError(t, authorization.ValidateBasic())
transfer = types.NewMsgTransfer(sourcePort, sourceChannel, coin500, fromAddr.String(), toAddr.String(), timeoutHeight, 0, "")
require.NoError(t, authorization.ValidateBasic())
resp, err = authorization.Accept(ctx, transfer)
require.NoError(t, err)
require.False(t, resp.Delete)
require.NotNil(t, resp.Updated)

allocation = types.Allocation{
SourcePort: sourcePort,
SourceChannel: sourceChannel,
SpendLimit: coins500,
AllowList: []string{toAddr.String()},
}
sendAuth := types.NewTransferAuthorization(allocation)
require.Equal(t, sendAuth.String(), resp.Updated.String())

t.Log("expect updated authorization nil after spending remaining amount")
resp, err = resp.Updated.Accept(ctx, transfer)
require.NoError(t, err)
require.True(t, resp.Delete)
require.Nil(t, resp.Updated)

t.Log("expect error when spend limit for specific port and channel is not set")
allocation = types.Allocation{
SourcePort: sourcePort,
SourceChannel: sourceChannel,
SpendLimit: coins1000,
AllowList: []string{toAddr.String()},
}
authorization = types.NewTransferAuthorization(allocation)
transfer = types.NewMsgTransfer(sourcePort2, sourceChannel2, coin500, fromAddr.String(), toAddr.String(), timeoutHeight, 0, "")
_, err = authorization.Accept(ctx, transfer)
require.Error(t, err)
testCases := []struct {
name string
malleate func()
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] for the malleate function, we use func() in almost all our tests I think, but what do you think about limiting the scope of what we can change and removing the test suite level variables

var (
		msgTransfer   types.MsgTransfer
		transferAuthz types.TransferAuthorization
	)

altogether?

Something like

malleate func(msgTransfer types.MsgTransfer, transferAuthz types.TransferAuthorization)

I think this makes the intent far more clear than when no arguments are supplied and eliminates the potential for one test to accidentally effect another.

expPass bool
expResult func(res authz.AcceptResponse)
Copy link
Contributor

Choose a reason for hiding this comment

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

[nit] the name expResult to me indicates that a function should return the result which will later be used in some sort of assertion, but this function is actually doing the assertions. Maybe assertResult or similar is more appropriate WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

As a follow up, if this function is renamed, we no longer really need the boolean expPass as that assertion could be done in the function too, in this case maybe the method signature could change to func(res authz.AcceptResponse, err error)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've applied the suggested changes here, but I think I'll leave the malleate func as is for now. I agree it would be good to limit the scope of what can be mutated in the malleate function but for now I'll just keep it as is to be consistent with other tests

}{
{
"success",
func() {},
true,
func(res authz.AcceptResponse) {
suite.Require().True(res.Accept)
suite.Require().True(res.Delete)
suite.Require().Nil(res.Updated)
},
},
{
"success: with spend limit updated",
func() {
msgTransfer.Token = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(50))
},
true,
func(res authz.AcceptResponse) {
suite.Require().True(res.Accept)
suite.Require().False(res.Delete)

t.Log("expect removing only 1 allocation if spend limit is finalized for the port")
updatedAuthz, ok := res.Updated.(*types.TransferAuthorization)
suite.Require().True(ok)

allocations := []types.Allocation{
isEqual := updatedAuthz.Allocations[0].SpendLimit.IsEqual(sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(50))))
suite.Require().True(isEqual)
},
},
{
SourcePort: sourcePort,
SourceChannel: sourceChannel,
SpendLimit: coins1000,
AllowList: []string{toAddr.String()},
"success: with empty allow list",
func() {
transferAuthz.Allocations[0].AllowList = []string{}
},
true,
func(res authz.AcceptResponse) {
suite.Require().True(res.Accept)
suite.Require().True(res.Delete)
suite.Require().Nil(res.Updated)
},
},
{
SourcePort: sourcePort2,
SourceChannel: sourceChannel2,
SpendLimit: coins1000,
AllowList: []string{toAddr.String()},
"success: with multiple allocations",
func() {
alloc := types.Allocation{
SourcePort: ibctesting.MockPort,
SourceChannel: "channel-9",
SpendLimit: ibctesting.TestCoins,
}

transferAuthz.Allocations = append(transferAuthz.Allocations, alloc)
},
true,
func(res authz.AcceptResponse) {
suite.Require().True(res.Accept)
suite.Require().False(res.Delete)

updatedAuthz, ok := res.Updated.(*types.TransferAuthorization)
suite.Require().True(ok)

// assert spent spendlimit is removed from the list
suite.Require().Len(updatedAuthz.Allocations, 1)
},
},
{
"no spend limit set for MsgTransfer port/channel",
func() {
msgTransfer.SourcePort = ibctesting.MockPort
msgTransfer.SourceChannel = "channel-9"
},
false,
func(res authz.AcceptResponse) {},
},
{
"requested transfer amount is more than the spend limit",
func() {
msgTransfer.Token = sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(1000))
},
false,
func(res authz.AcceptResponse) {},
},
{
"receiver address not permitted via allow list",
func() {
msgTransfer.Receiver = suite.chainB.SenderAccount.GetAddress().String()
},
false,
func(res authz.AcceptResponse) {},
},
}
authorization = types.NewTransferAuthorization(allocations...)
transfer = types.NewMsgTransfer(sourcePort, sourceChannel, coin1000, fromAddr.String(), toAddr.String(), timeoutHeight, 0, "")
resp, err = authorization.Accept(ctx, transfer)
require.NoError(t, err)
require.NotNil(t, resp.Updated)
require.Equal(t, resp.Updated, types.NewTransferAuthorization(allocations[1]))
require.False(t, resp.Delete)

t.Log("expect error when transferring to not allowed address")
allocation = types.Allocation{
SourcePort: sourcePort,
SourceChannel: sourceChannel,
SpendLimit: coins1000,
AllowList: []string{fromAddr.String()},

for _, tc := range testCases {
suite.Run(tc.name, func() {
suite.SetupTest()

path := NewTransferPath(suite.chainA, suite.chainB)
suite.coordinator.Setup(path)

transferAuthz = types.TransferAuthorization{
Allocations: []types.Allocation{
{
SourcePort: path.EndpointA.ChannelConfig.PortID,
SourceChannel: path.EndpointA.ChannelID,
SpendLimit: ibctesting.TestCoins,
AllowList: []string{ibctesting.TestAccAddress},
},
},
}

msgTransfer = types.MsgTransfer{
SourcePort: path.EndpointA.ChannelConfig.PortID,
SourceChannel: path.EndpointA.ChannelID,
Token: ibctesting.TestCoin,
Sender: suite.chainA.SenderAccount.GetAddress().String(),
Receiver: ibctesting.TestAccAddress,
TimeoutHeight: suite.chainB.GetTimeoutHeight(),
}

tc.malleate()

res, err := transferAuthz.Accept(suite.chainA.GetContext(), &msgTransfer)
if tc.expPass {
suite.Require().NoError(err)
tc.expResult(res)
} else {
suite.Require().Error(err)
}
})
}
authorization = types.NewTransferAuthorization(allocation)
transfer = types.NewMsgTransfer(sourcePort, sourceChannel, coin500, fromAddr.String(), toAddr.String(), timeoutHeight, 0, "")
_, err = authorization.Accept(ctx, transfer)
require.Error(t, err)
}

func TestTransferAuthorizationValidateBasic(t *testing.T) {
func (suite *TypesTestSuite) TestTransferAuthorizationMsgTypeURL() {
var transferAuthz types.TransferAuthorization
suite.Require().Equal(sdk.MsgTypeURL(&types.MsgTransfer{}), transferAuthz.MsgTypeURL(), "invalid type url for transfer authorization")
}
Comment on lines +154 to +157
Copy link
Contributor

Choose a reason for hiding this comment

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

nice addtion!


func (suite *TypesTestSuite) TestTransferAuthorizationValidateBasic() {
var transferAuthz types.TransferAuthorization

testCases := []struct {
Expand Down Expand Up @@ -212,7 +241,7 @@ func TestTransferAuthorizationValidateBasic(t *testing.T) {
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
suite.Run(tc.name, func() {
transferAuthz = types.TransferAuthorization{
Allocations: []types.Allocation{
{
Expand All @@ -229,9 +258,9 @@ func TestTransferAuthorizationValidateBasic(t *testing.T) {
err := transferAuthz.ValidateBasic()

if tc.expPass {
require.NoError(t, err)
suite.Require().NoError(err)
} else {
require.Error(t, err)
suite.Require().Error(err)
}
})
}
Expand Down
11 changes: 11 additions & 0 deletions modules/apps/transfer/types/ack_test.go → modules/apps/transfer/types/types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

"github.com/stretchr/testify/suite"

"github.com/cosmos/ibc-go/v6/modules/apps/transfer/types"
ibctesting "github.com/cosmos/ibc-go/v6/testing"
)

Expand All @@ -24,6 +25,16 @@ func (suite *TypesTestSuite) SetupTest() {
suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(2))
}

func NewTransferPath(chainA, chainB *ibctesting.TestChain) *ibctesting.Path {
path := ibctesting.NewPath(chainA, chainB)
path.EndpointA.ChannelConfig.PortID = types.PortID
path.EndpointB.ChannelConfig.PortID = types.PortID
path.EndpointA.ChannelConfig.Version = types.Version
path.EndpointB.ChannelConfig.Version = types.Version

return path
}

func TestTypesTestSuite(t *testing.T) {
suite.Run(t, new(TypesTestSuite))
}