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

test(callbacks): simplified mock contract keeper's processCallback logic #4375

Merged
merged 13 commits into from
Aug 24, 2023
Merged

test(callbacks): simplified mock contract keeper's processCallback logic #4375

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
8123fdb
imp(callbacks/mock/contract_keeeper): improved contract keeper logic
srdtrk Aug 17, 2023
91b7858
imp(callbacks_test): fixed transfer_test.go
srdtrk Aug 17, 2023
17d1892
imp(callbacks_test): fixed ica_test.go
srdtrk Aug 17, 2023
caf6745
imp(callbacks_test): fixed fee_transfer_test.go
srdtrk Aug 17, 2023
25b4612
style(callbacks_test): removed unneeded gas_limit
srdtrk Aug 17, 2023
1ea7a3d
style: ran golangci-lint
srdtrk Aug 17, 2023
75bb8fa
imp(callbacks_test): implemented simplified logic
srdtrk Aug 17, 2023
332f08a
imp(callbacks_test): removed unneeded variable
srdtrk Aug 17, 2023
c99b86a
merge: branch 'main' into serdar/issue#4309-simplify-mock-callbacks-l…
srdtrk Aug 21, 2023
e14aba1
imp(callbacks): implemented some review feedback
srdtrk Aug 23, 2023
abc85a1
merge: branch 'main' into serdar/issue#4309-simplify-mock-callbacks-l…
srdtrk Aug 23, 2023
95b25e5
docs(callbacks/simapp): updated godocs of mock contract keeper functions
srdtrk Aug 23, 2023
05fe054
merge: branch 'main' into serdar/issue#4309-simplify-mock-callbacks-l…
srdtrk Aug 23, 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
49 changes: 37 additions & 12 deletions modules/apps/callbacks/fee_transfer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

sdk "github.com/cosmos/cosmos-sdk/types"

"github.com/cosmos/ibc-go/modules/apps/callbacks/testing/simapp"
"github.com/cosmos/ibc-go/modules/apps/callbacks/types"
feetypes "github.com/cosmos/ibc-go/v7/modules/apps/29-fee/types"
ibctesting "github.com/cosmos/ibc-go/v7/testing"
Expand All @@ -33,19 +34,19 @@ func (s *CallbacksTestSuite) TestIncentivizedTransferCallbacks() {
},
{
"success: dest callback",
fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, callbackAddr),
fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, simapp.SuccessContract),
types.CallbackTypeReceivePacket,
true,
},
{
"success: dest callback with other json fields",
fmt.Sprintf(`{"dest_callback": {"address": "%s"}, "something_else": {}}`, callbackAddr),
fmt.Sprintf(`{"dest_callback": {"address": "%s"}, "something_else": {}}`, simapp.SuccessContract),
types.CallbackTypeReceivePacket,
true,
},
{
"success: dest callback with malformed json",
fmt.Sprintf(`{"dest_callback": {"address": "%s"}, malformed}`, callbackAddr),
fmt.Sprintf(`{"dest_callback": {"address": "%s"}, malformed}`, simapp.SuccessContract),
"none",
true,
},
Expand All @@ -57,19 +58,19 @@ func (s *CallbacksTestSuite) TestIncentivizedTransferCallbacks() {
},
{
"success: source callback",
fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, callbackAddr),
fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.SuccessContract),
types.CallbackTypeAcknowledgementPacket,
true,
},
{
"success: source callback with other json fields",
fmt.Sprintf(`{"src_callback": {"address": "%s"}, "something_else": {}}`, callbackAddr),
fmt.Sprintf(`{"src_callback": {"address": "%s"}, "something_else": {}}`, simapp.SuccessContract),
types.CallbackTypeAcknowledgementPacket,
true,
},
{
"success: source callback with malformed json",
fmt.Sprintf(`{"src_callback": {"address": "%s"}, malformed}`, callbackAddr),
fmt.Sprintf(`{"src_callback": {"address": "%s"}, malformed}`, simapp.SuccessContract),
"none",
true,
},
Expand All @@ -81,13 +82,25 @@ func (s *CallbacksTestSuite) TestIncentivizedTransferCallbacks() {
},
{
"failure: dest callback with low gas (panic)",
fmt.Sprintf(`{"dest_callback": {"address": "%s", "gas_limit": "450000"}}`, callbackAddr),
fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, simapp.OogPanicContract),
types.CallbackTypeReceivePacket,
false,
},
{
"failure: dest callback with low gas (error)",
fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, simapp.OogErrorContract),
types.CallbackTypeReceivePacket,
false,
},
{
"failure: source callback with low gas (panic)",
fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "450000"}}`, callbackAddr),
fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogPanicContract),
types.CallbackTypeSendPacket,
false,
},
{
"failure: source callback with low gas (error)",
fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogErrorContract),
types.CallbackTypeSendPacket,
false,
},
Expand Down Expand Up @@ -127,25 +140,37 @@ func (s *CallbacksTestSuite) TestIncentivizedTransferTimeoutCallbacks() {
},
{
"success: dest callback",
fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, callbackAddr),
fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, simapp.SuccessContract),
"none",
true, // timeouts don't reach destination chain execution
},
{
"success: source callback",
fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, callbackAddr),
fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.SuccessContract),
types.CallbackTypeTimeoutPacket,
true,
},
{
"success: dest callback with low gas (panic)",
fmt.Sprintf(`{"dest_callback": {"address": "%s", "gas_limit": "450000"}}`, callbackAddr),
fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, simapp.OogPanicContract),
"none", // timeouts don't reach destination chain execution
false,
},
{
"failure: source callback with low gas (panic)",
fmt.Sprintf(`{"src_callback": {"address": "%s", "gas_limit": "450000"}}`, callbackAddr),
fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogPanicContract),
types.CallbackTypeSendPacket,
false,
},
{
"success: dest callback with low gas (error)",
fmt.Sprintf(`{"dest_callback": {"address": "%s"}}`, simapp.OogErrorContract),
"none", // timeouts don't reach destination chain execution
false,
},
{
"failure: source callback with low gas (error)",
fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.OogErrorContract),
types.CallbackTypeSendPacket,
false,
},
Expand Down
99 changes: 53 additions & 46 deletions modules/apps/callbacks/ibc_middleware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,9 @@ func (s *CallbacksTestSuite) TestSendPacket() {
channeltypes.ErrChannelNotFound,
},
{
"failure: callback execution fails, sender is not callback address",
"failure: callback execution fails",
func() {
packetData.Sender = simapp.MockCallbackUnauthorizedAddress
packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s"}}`, simapp.ErrorContract)
},
types.CallbackTypeSendPacket,
false,
Expand All @@ -138,11 +138,11 @@ func (s *CallbacksTestSuite) TestSendPacket() {
{
"failure: callback execution reach out of gas, but sufficient gas provided by relayer",
func() {
packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"400000"}}`, callbackAddr)
packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"400000"}}`, simapp.OogPanicContract)
},
types.CallbackTypeSendPacket,
true,
sdk.ErrorOutOfGas{Descriptor: fmt.Sprintf("mock %s callback panic", types.CallbackTypeSendPacket)},
sdk.ErrorOutOfGas{Descriptor: fmt.Sprintf("mock %s callback oog panic", types.CallbackTypeSendPacket)},
},
}

Expand All @@ -156,8 +156,8 @@ func (s *CallbacksTestSuite) TestSendPacket() {
s.Require().True(ok)

packetData = transfertypes.NewFungibleTokenPacketData(
ibctesting.TestCoin.GetDenom(), ibctesting.TestCoin.Amount.String(), callbackAddr,
ibctesting.TestAccAddress, fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, callbackAddr),
ibctesting.TestCoin.GetDenom(), ibctesting.TestCoin.Amount.String(), ibctesting.TestAccAddress,
ibctesting.TestAccAddress, fmt.Sprintf(`{"src_callback": {"address": "%s"}}`, simapp.SuccessContract),
)

chanCap := s.path.EndpointA.Chain.GetChannelCapability(s.path.EndpointA.ChannelConfig.PortID, s.path.EndpointA.ChannelID)
Expand Down Expand Up @@ -200,10 +200,11 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() {
)

var (
packetData transfertypes.FungibleTokenPacketData
packet channeltypes.Packet
ack []byte
ctx sdk.Context
packetData transfertypes.FungibleTokenPacketData
packet channeltypes.Packet
ack []byte
ctx sdk.Context
userGasLimit uint64
)

panicError := fmt.Errorf("panic error")
Expand Down Expand Up @@ -241,7 +242,7 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() {
{
"failure: callback execution reach out of gas, but sufficient gas provided by relayer",
func() {
packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"400000"}}`, callbackAddr)
packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"%d"}}`, simapp.OogPanicContract, userGasLimit)
packet.Data = packetData.GetBytes()
},
callbackFailed,
Expand All @@ -250,15 +251,18 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() {
{
"failure: callback execution panics on insufficient gas provided by relayer",
func() {
packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"%d"}}`, simapp.OogPanicContract, userGasLimit)
packet.Data = packetData.GetBytes()

ctx = ctx.WithGasMeter(sdk.NewGasMeter(300_000))
},
callbackFailed,
panicError,
},
{
"failure: callback execution fails, unauthorized address",
"failure: callback execution fails",
func() {
packetData.Sender = simapp.MockCallbackUnauthorizedAddress
packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s"}}`, simapp.ErrorContract)
packet.Data = packetData.GetBytes()
},
callbackFailed,
Expand All @@ -272,10 +276,10 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() {
s.SetupTransferTest()

// set user gas limit above panic level in mock contract keeper
srdtrk marked this conversation as resolved.
Show resolved Hide resolved
userGasLimit := 600000
userGasLimit = 600000
Copy link
Contributor

Choose a reason for hiding this comment

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

does this get modified? Could probably be defined at the top of the test

Copy link
Member Author

Choose a reason for hiding this comment

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

No, but the malleate needs access to this variable if it needs to change the memo. If you define it at the top, then an unexpected behavior can occur in which the modification of this variable from a test case can leak to the next test case

packetData = transfertypes.NewFungibleTokenPacketData(
ibctesting.TestCoin.GetDenom(), ibctesting.TestCoin.Amount.String(), callbackAddr, ibctesting.TestAccAddress,
fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"%d"}}`, callbackAddr, userGasLimit),
ibctesting.TestCoin.GetDenom(), ibctesting.TestCoin.Amount.String(), ibctesting.TestAccAddress, ibctesting.TestAccAddress,
fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"%d"}}`, simapp.SuccessContract, userGasLimit),
)

packet = channeltypes.Packet{
Expand Down Expand Up @@ -356,13 +360,11 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() {
ctx sdk.Context
)

panicError := fmt.Errorf("panic error")

testCases := []struct {
name string
malleate func()
expResult expResult
expError error
expValue interface{}
}{
{
"success",
Expand Down Expand Up @@ -391,7 +393,7 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() {
{
"failure: callback execution reach out of gas, but sufficient gas provided by relayer",
func() {
packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"400000"}}`, callbackAddr)
packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s", "gas_limit":"400000"}}`, simapp.OogPanicContract)
packet.Data = packetData.GetBytes()
},
callbackFailed,
Expand All @@ -400,15 +402,20 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() {
{
"failure: callback execution panics on insufficient gas provided by relayer",
func() {
packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s"}}`, simapp.OogPanicContract)
packet.Data = packetData.GetBytes()

ctx = ctx.WithGasMeter(sdk.NewGasMeter(300_000))
},
callbackFailed,
panicError,
sdk.ErrorOutOfGas{
Descriptor: fmt.Sprintf("ibc %s callback out of gas; commitGasLimit: %d", types.CallbackTypeTimeoutPacket, maxCallbackGas),
},
},
{
"failure: callback execution fails, unauthorized address",
"failure: callback execution fails",
func() {
packetData.Sender = simapp.MockCallbackUnauthorizedAddress
packetData.Memo = fmt.Sprintf(`{"src_callback": {"address":"%s"}}`, simapp.ErrorContract)
packet.Data = packetData.GetBytes()
},
callbackFailed,
Expand Down Expand Up @@ -455,21 +462,17 @@ func (s *CallbacksTestSuite) TestOnTimeoutPacket() {
return transferStack.OnTimeoutPacket(ctx, packet, s.chainA.SenderAccount.GetAddress())
}

switch tc.expError {
switch expValue := tc.expValue.(type) {
case nil:
err := onTimeoutPacket()
s.Require().Nil(err)

case panicError:
s.Require().PanicsWithValue(sdk.ErrorOutOfGas{
Descriptor: fmt.Sprintf("ibc %s callback out of gas; commitGasLimit: %d", types.CallbackTypeTimeoutPacket, userGasLimit),
}, func() {
case error:
err := onTimeoutPacket()
s.Require().ErrorIs(expValue, err)
default:
s.Require().PanicsWithValue(tc.expValue, func() {
_ = onTimeoutPacket()
})

default:
err := onTimeoutPacket()
s.Require().ErrorIs(tc.expError, err)
}

sourceStatefulCounter := GetSimApp(s.chainA).MockContractKeeper.GetStateEntryCounter(s.chainA.GetContext())
Expand Down Expand Up @@ -506,9 +509,10 @@ func (s *CallbacksTestSuite) TestOnRecvPacket() {
)

var (
packetData transfertypes.FungibleTokenPacketData
packet channeltypes.Packet
ctx sdk.Context
packetData transfertypes.FungibleTokenPacketData
packet channeltypes.Packet
ctx sdk.Context
userGasLimit uint64
)

successAck := channeltypes.NewResultAcknowledgement([]byte{byte(1)})
Expand Down Expand Up @@ -547,7 +551,7 @@ func (s *CallbacksTestSuite) TestOnRecvPacket() {
{
"failure: callback execution reach out of gas, but sufficient gas provided by relayer",
func() {
packetData.Memo = fmt.Sprintf(`{"dest_callback": {"address":"%s", "gas_limit":"400000"}}`, callbackAddr)
packetData.Memo = fmt.Sprintf(`{"dest_callback": {"address":"%s", "gas_limit":"%d"}}`, simapp.OogPanicContract, userGasLimit)
packet.Data = packetData.GetBytes()
},
callbackFailed,
Expand All @@ -556,20 +560,23 @@ func (s *CallbacksTestSuite) TestOnRecvPacket() {
{
"failure: callback execution panics on insufficient gas provided by relayer",
func() {
packetData.Memo = fmt.Sprintf(`{"dest_callback": {"address":"%s", "gas_limit":"%d"}}`, simapp.OogPanicContract, userGasLimit)
packet.Data = packetData.GetBytes()

ctx = ctx.WithGasMeter(sdk.NewGasMeter(300_000))
},
callbackFailed,
panicAck,
},
/*
TODO: https://github.com/cosmos/ibc-go/issues/4309
{
"failure: callback execution fails",
func() {},
callbackFailed,
successAck,
{
"failure: callback execution fails",
func() {
packetData.Memo = fmt.Sprintf(`{"dest_callback": {"address":"%s"}}`, simapp.ErrorContract)
packet.Data = packetData.GetBytes()
},
*/
callbackFailed,
successAck,
},
}

for _, tc := range testCases {
Expand All @@ -578,7 +585,7 @@ func (s *CallbacksTestSuite) TestOnRecvPacket() {
s.SetupTransferTest()

// set user gas limit above panic level in mock contract keeper
userGasLimit := 600_000
userGasLimit = 600_000
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Member Author

Choose a reason for hiding this comment

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

☝️

No, but the malleate needs access to this variable if it needs to change the memo. If you define it at the top, then an unexpected behavior can occur in which the modification of this variable from a test case can leak to the next test case

packetData = transfertypes.NewFungibleTokenPacketData(
ibctesting.TestCoin.GetDenom(), ibctesting.TestCoin.Amount.String(), ibctesting.TestAccAddress, s.chainB.SenderAccount.GetAddress().String(),
fmt.Sprintf(`{"dest_callback": {"address":"%s", "gas_limit":"%d"}}`, ibctesting.TestAccAddress, userGasLimit),
Expand Down
Loading
Loading