Skip to content

Commit

Permalink
test(callbacks): simplified mock contract keeper's processCallback lo…
Browse files Browse the repository at this point in the history 
…gic (#4375)

* imp(callbacks/mock/contract_keeeper): improved contract keeper logic

* imp(callbacks_test): fixed transfer_test.go

* imp(callbacks_test): fixed ica_test.go

* imp(callbacks_test): fixed fee_transfer_test.go

* style(callbacks_test): removed unneeded gas_limit

* style: ran golangci-lint

* imp(callbacks_test): implemented simplified logic

* imp(callbacks_test): removed unneeded variable

* imp(callbacks): implemented some review feedback

* docs(callbacks/simapp): updated godocs of mock contract keeper functions
  • Loading branch information
@srdtrk
srdtrk authored Aug 24, 2023
1 parent 7c5bd1e commit ce3cfc0
Show file tree
Hide file tree
Showing 5 changed files with 225 additions and 119 deletions.
49 changes: 37 additions & 12 deletions modules/apps/callbacks/fee_transfer_test.go
View file
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
100 changes: 53 additions & 47 deletions modules/apps/callbacks/ibc_middleware_test.go
View file
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 @@ -271,11 +275,10 @@ func (s *CallbacksTestSuite) TestOnAcknowledgementPacket() {
s.Run(tc.name, func() {
s.SetupTransferTest()

// set user gas limit above panic level in mock contract keeper
userGasLimit := 600000
userGasLimit = 600000
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 +359,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 +392,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 +401,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 +461,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 +508,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 +550,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 +559,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 +584,7 @@ func (s *CallbacksTestSuite) TestOnRecvPacket() {
s.SetupTransferTest()

// set user gas limit above panic level in mock contract keeper
userGasLimit := 600_000
userGasLimit = 600_000
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

0 comments on commit ce3cfc0

Please sign in to comment.