Skip to content

Commit

Permalink
fix: ics29: switch source with destintion for chan/port IDs (#961)
Browse files Browse the repository at this point in the history
* fix: switch source with destintion for chan/port IDs

* fix: blunder

* test: adding tests in case of incorrect channel/port id

* test: moving check to WriteAcknowledgement

* add test case for Get/Set counterparty address

* nit: path name

* Update modules/apps/29-fee/keeper/msg_server_test.go
  • Loading branch information
seantking authored Feb 25, 2022
1 parent 9c508d2 commit 7991f79
Show file tree
Hide file tree
Showing 7 changed files with 60 additions and 16 deletions.
14 changes: 12 additions & 2 deletions modules/apps/29-fee/fee_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,17 @@ type FeeTestSuite struct {

chainA *ibctesting.TestChain
chainB *ibctesting.TestChain
chainC *ibctesting.TestChain

path *ibctesting.Path
path *ibctesting.Path
pathAToC *ibctesting.Path
}

func (suite *FeeTestSuite) SetupTest() {
suite.coordinator = ibctesting.NewCoordinator(suite.T(), 2)
suite.coordinator = ibctesting.NewCoordinator(suite.T(), 3)
suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(1))
suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(2))
suite.chainC = suite.coordinator.GetChain(ibctesting.GetChainID(3))

path := ibctesting.NewPath(suite.chainA, suite.chainB)
mockFeeVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: ibcmock.Version}))
Expand All @@ -35,6 +38,13 @@ func (suite *FeeTestSuite) SetupTest() {
path.EndpointA.ChannelConfig.PortID = ibctesting.MockFeePort
path.EndpointB.ChannelConfig.PortID = ibctesting.MockFeePort
suite.path = path

path = ibctesting.NewPath(suite.chainA, suite.chainC)
path.EndpointA.ChannelConfig.Version = mockFeeVersion
path.EndpointB.ChannelConfig.Version = mockFeeVersion
path.EndpointA.ChannelConfig.PortID = ibctesting.MockFeePort
path.EndpointB.ChannelConfig.PortID = ibctesting.MockFeePort
suite.pathAToC = path
}

func TestIBCFeeTestSuite(t *testing.T) {
Expand Down
4 changes: 2 additions & 2 deletions modules/apps/29-fee/ibc_module.go
Original file line number Diff line number Diff line change
Expand Up @@ -198,12 +198,12 @@ func (im IBCModule) OnRecvPacket(

// incase of async aknowledgement (ack == nil) store the relayer address for use later during async WriteAcknowledgement
if ack == nil {
im.keeper.SetRelayerAddressForAsyncAck(ctx, channeltypes.NewPacketId(packet.GetSourceChannel(), packet.GetSourcePort(), packet.GetSequence()), relayer.String())
im.keeper.SetRelayerAddressForAsyncAck(ctx, channeltypes.NewPacketId(packet.GetDestChannel(), packet.GetDestPort(), packet.GetSequence()), relayer.String())
return nil
}

// if forwardRelayer is not found we refund recv_fee
forwardRelayer, _ := im.keeper.GetCounterpartyAddress(ctx, relayer.String(), packet.GetSourceChannel())
forwardRelayer, _ := im.keeper.GetCounterpartyAddress(ctx, relayer.String(), packet.GetDestChannel())

return types.NewIncentivizedAcknowledgement(forwardRelayer, ack.Acknowledgement(), ack.Success())
}
Expand Down
21 changes: 18 additions & 3 deletions modules/apps/29-fee/ibc_module_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -498,12 +498,12 @@ func (suite *FeeTestSuite) TestOnRecvPacket() {
tc := tc
suite.Run(tc.name, func() {
suite.SetupTest()
// setup pathAToC (chainA -> chainC) first in order to have different channel IDs for chainA & chainB
suite.coordinator.Setup(suite.pathAToC)
// setup path for chainA -> chainB
suite.coordinator.Setup(suite.path)

// set up a different channel to make sure that the test will error if the destination channel of the packet is not fee enabled
suite.path.EndpointB.ChannelID = "channel-1"
suite.chainB.GetSimApp().IBCFeeKeeper.SetFeeEnabled(suite.chainB.GetContext(), suite.path.EndpointB.ChannelConfig.PortID, suite.path.EndpointB.ChannelID)
suite.chainB.GetSimApp().IBCFeeKeeper.DeleteFeeEnabled(suite.chainB.GetContext(), suite.path.EndpointB.ChannelConfig.PortID, "channel-0")

packet := suite.CreateMockPacket()

Expand All @@ -522,11 +522,26 @@ func (suite *FeeTestSuite) TestOnRecvPacket() {
result := cbs.OnRecvPacket(suite.chainB.GetContext(), packet, suite.chainA.SenderAccount.GetAddress())

switch {
case tc.name == "success":
forwardAddr, _ := suite.chainB.GetSimApp().IBCFeeKeeper.GetCounterpartyAddress(suite.chainB.GetContext(), suite.chainA.SenderAccount.GetAddress().String(), suite.path.EndpointB.ChannelID)

expectedAck := types.IncentivizedAcknowledgement{
Result: ibcmock.MockAcknowledgement.Acknowledgement(),
ForwardRelayerAddress: forwardAddr,
UnderlyingAppSuccess: true,
}
suite.Require().Equal(expectedAck, result)

case !tc.feeEnabled:
suite.Require().Equal(ibcmock.MockAcknowledgement, result)

case tc.forwardRelayer && result == nil:
suite.Require().Equal(nil, result)
packetId := channeltypes.NewPacketId(packet.GetDestChannel(), packet.GetDestPort(), packet.GetSequence())

// retrieve the forward relayer that was stored in `onRecvPacket`
relayer, _ := suite.chainB.GetSimApp().IBCFeeKeeper.GetRelayerAddressForAsyncAck(suite.chainB.GetContext(), packetId)
suite.Require().Equal(relayer, suite.chainA.SenderAccount.GetAddress().String())

case !tc.forwardRelayer:
expectedAck := types.IncentivizedAcknowledgement{
Expand Down
15 changes: 13 additions & 2 deletions modules/apps/29-fee/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,19 @@ type KeeperTestSuite struct {
// testing chains used for convenience and readability
chainA *ibctesting.TestChain
chainB *ibctesting.TestChain
chainC *ibctesting.TestChain

path *ibctesting.Path
pathAToC *ibctesting.Path

path *ibctesting.Path
queryClient types.QueryClient
}

func (suite *KeeperTestSuite) SetupTest() {
suite.coordinator = ibctesting.NewCoordinator(suite.T(), 2)
suite.coordinator = ibctesting.NewCoordinator(suite.T(), 3)
suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(1))
suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(2))
suite.chainC = suite.coordinator.GetChain(ibctesting.GetChainID(3))

path := ibctesting.NewPath(suite.chainA, suite.chainB)
mockFeeVersion := string(types.ModuleCdc.MustMarshalJSON(&types.Metadata{FeeVersion: types.Version, AppVersion: ibcmock.Version}))
Expand All @@ -46,6 +50,13 @@ func (suite *KeeperTestSuite) SetupTest() {
path.EndpointB.ChannelConfig.PortID = ibctesting.MockFeePort
suite.path = path

path = ibctesting.NewPath(suite.chainA, suite.chainC)
path.EndpointA.ChannelConfig.Version = mockFeeVersion
path.EndpointB.ChannelConfig.Version = mockFeeVersion
path.EndpointA.ChannelConfig.PortID = ibctesting.MockFeePort
path.EndpointB.ChannelConfig.PortID = ibctesting.MockFeePort
suite.pathAToC = path

queryHelper := baseapp.NewQueryServerTestHelper(suite.chainA.GetContext(), suite.chainA.GetSimApp().InterfaceRegistry())
types.RegisterQueryServer(queryHelper, suite.chainA.GetSimApp().IBCFeeKeeper)
suite.queryClient = types.NewQueryClient(queryHelper)
Expand Down
2 changes: 1 addition & 1 deletion modules/apps/29-fee/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ func (suite *KeeperTestSuite) TestRegisterCounterpartyAddress() {
func() {},
},
{
"success",
"counterparty is an arbitrary string",
true,
func() { counterparty = "arbitrary-string" },
},
Expand Down
6 changes: 3 additions & 3 deletions modules/apps/29-fee/keeper/relay.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,17 @@ func (k Keeper) WriteAcknowledgement(ctx sdk.Context, chanCap *capabilitytypes.C
return k.ics4Wrapper.WriteAcknowledgement(ctx, chanCap, packet, acknowledgement)
}

// retrieve the forward relayer that was stored in `onRecvPacket`
packetId := channeltypes.NewPacketId(packet.GetSourceChannel(), packet.GetSourcePort(), packet.GetSequence())
packetId := channeltypes.NewPacketId(packet.GetDestChannel(), packet.GetDestPort(), packet.GetSequence())

// retrieve the forward relayer that was stored in `onRecvPacket`
relayer, found := k.GetRelayerAddressForAsyncAck(ctx, packetId)
if !found {
return sdkerrors.Wrapf(types.ErrRelayerNotFoundForAsyncAck, "no relayer address stored for async acknowledgement for packet with portID: %s, channelID: %s, sequence: %d", packetId.PortId, packetId.ChannelId, packetId.Sequence)
}

// it is possible that a relayer has not registered a counterparty address.
// if there is no registered counterparty address then write acknowledgement with empty relayer address and refund recv_fee.
forwardRelayer, _ := k.GetCounterpartyAddress(ctx, relayer, packet.GetSourceChannel())
forwardRelayer, _ := k.GetCounterpartyAddress(ctx, relayer, packet.GetDestChannel())

ack := types.NewIncentivizedAcknowledgement(forwardRelayer, acknowledgement.Acknowledgement(), acknowledgement.Success())

Expand Down
14 changes: 11 additions & 3 deletions modules/apps/29-fee/keeper/relay_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper_test

import (
"github.com/cosmos/ibc-go/v3/modules/apps/29-fee/types"
clienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/types"
channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
)
Expand All @@ -14,8 +15,8 @@ func (suite *KeeperTestSuite) TestWriteAcknowledgementAsync() {
{
"success",
func() {
suite.chainB.GetSimApp().IBCFeeKeeper.SetRelayerAddressForAsyncAck(suite.chainB.GetContext(), channeltypes.NewPacketId(suite.path.EndpointA.ChannelID, suite.path.EndpointA.ChannelConfig.PortID, 1), suite.chainA.SenderAccount.GetAddress().String())
suite.chainB.GetSimApp().IBCFeeKeeper.SetCounterpartyAddress(suite.chainB.GetContext(), suite.chainA.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.GetAddress().String(), suite.path.EndpointA.ChannelID)
suite.chainB.GetSimApp().IBCFeeKeeper.SetRelayerAddressForAsyncAck(suite.chainB.GetContext(), channeltypes.NewPacketId(suite.path.EndpointB.ChannelID, suite.path.EndpointB.ChannelConfig.PortID, 1), suite.chainA.SenderAccount.GetAddress().String())
suite.chainB.GetSimApp().IBCFeeKeeper.SetCounterpartyAddress(suite.chainB.GetContext(), suite.chainA.SenderAccount.GetAddress().String(), suite.chainB.SenderAccount.GetAddress().String(), suite.path.EndpointB.ChannelID)
},
true,
},
Expand All @@ -31,7 +32,10 @@ func (suite *KeeperTestSuite) TestWriteAcknowledgementAsync() {
suite.Run(tc.name, func() {
suite.SetupTest()

// open incentivized channel
// open incentivized channels
// setup pathAToC (chainA -> chainC) first in order to have different channel IDs for chainA & chainB
suite.coordinator.Setup(suite.pathAToC)
// setup path for chainA -> chainB
suite.coordinator.Setup(suite.path)

// build packet
Expand Down Expand Up @@ -59,6 +63,10 @@ func (suite *KeeperTestSuite) TestWriteAcknowledgementAsync() {
suite.Require().NoError(err)
_, found := suite.chainB.GetSimApp().IBCFeeKeeper.GetRelayerAddressForAsyncAck(suite.chainB.GetContext(), channeltypes.NewPacketId(suite.path.EndpointA.ChannelID, suite.path.EndpointA.ChannelConfig.PortID, 1))
suite.Require().False(found)

expectedAck := types.NewIncentivizedAcknowledgement(suite.chainB.SenderAccount.GetAddress().String(), ack.Acknowledgement(), ack.Success())
commitedAck, _ := suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.GetPacketAcknowledgement(suite.chainB.GetContext(), packet.DestinationPort, packet.DestinationChannel, 1)
suite.Require().Equal(commitedAck, channeltypes.CommitAcknowledgement(expectedAck.Acknowledgement()))
} else {
suite.Require().Error(err)
}
Expand Down

0 comments on commit 7991f79

Please sign in to comment.