diff --git a/.github/workflows/packet-forward-middleware.yml b/.github/workflows/packet-forward-middleware.yml index c66ef380..9c5cec65 100644 --- a/.github/workflows/packet-forward-middleware.yml +++ b/.github/workflows/packet-forward-middleware.yml @@ -1,23 +1,23 @@ name: packet-forward-middleware -on: +on: pull_request: paths: - 'middleware/packet-forward-middleware/**' - '.github/workflows/packet-forward-middleware.yml' env: - LINT_VERSION: v1.52 + LINT_VERSION: v1.55.2 GO_VERSION: 1.21.0 WORKING_DIRECTORY: middleware/packet-forward-middleware - + DOCKER_TAG: pfm:local TAR_PATH: /tmp/pfm-docker-image.tar IMAGE_NAME: pfm-docker-image jobs: golangci: - name: Linter - runs-on: ubuntu-latest + name: Linter + runs-on: ubuntu-latest steps: - uses: actions/checkout@v3 @@ -51,14 +51,14 @@ jobs: - name: Setup Go ${{ env.GO_VERSION }} uses: actions/setup-go@v4 with: - go-version: ${{ env.GO_VERSION }} + go-version: ${{ env.GO_VERSION }} - name: Set up Docker Buildx - uses: docker/setup-buildx-action@v3 + uses: docker/setup-buildx-action@v3 - name: Build and export uses: docker/build-push-action@v5 - with: + with: context: ${{ env.WORKING_DIRECTORY }} tags: ${{ env.DOCKER_TAG }} outputs: type=docker,dest=${{ env.TAR_PATH }} @@ -68,7 +68,7 @@ jobs: with: name: ${{ env.IMAGE_NAME }} path: ${{ env.TAR_PATH }} - + e2e-tests: needs: build-docker runs-on: ubuntu-latest @@ -76,7 +76,7 @@ jobs: run: working-directory: ${{ env.WORKING_DIRECTORY }} strategy: - matrix: + matrix: test: - "ictest-forward" - "ictest-timeout" @@ -88,8 +88,8 @@ jobs: - name: Set up Go ${{ env.GO_VERSION }} uses: actions/setup-go@v4 with: - go-version: ${{ env.GO_VERSION }} - + go-version: ${{ env.GO_VERSION }} + - uses: actions/checkout@v4 - name: Download Tarball Artifact diff --git a/middleware/packet-forward-middleware/e2e/forward_timeout_test.go b/middleware/packet-forward-middleware/e2e/forward_timeout_test.go index c01981f2..f274c6b6 100644 --- a/middleware/packet-forward-middleware/e2e/forward_timeout_test.go +++ b/middleware/packet-forward-middleware/e2e/forward_timeout_test.go @@ -214,7 +214,7 @@ func TestTimeoutOnForward(t *testing.T) { time.Sleep(time.Second * 11) // Restart the relayer - err = r.StartRelayer(ctx, eRep, pathAB, pathBC) + err = r.StartRelayer(ctx, eRep, pathAB, pathBC, pathCD) require.NoError(t, err) chainAHeight, err := chainA.Height(ctx) @@ -263,4 +263,149 @@ func TestTimeoutOnForward(t *testing.T) { require.True(t, firstHopEscrowBalance.Equal(zeroBal)) require.True(t, secondHopEscrowBalance.Equal(zeroBal)) require.True(t, thirdHopEscrowBalance.Equal(zeroBal)) + + // Send IBC transfer from ChainA -> ChainB -> ChainC -> ChainD that will succeed + secondHopMetadata = &PacketMetadata{ + Forward: &ForwardMetadata{ + Receiver: userD.FormattedAddress(), + Channel: cdChan.ChannelID, + Port: cdChan.PortID, + }, + } + nextBz, err = json.Marshal(secondHopMetadata) + require.NoError(t, err) + next = string(nextBz) + + firstHopMetadata = &PacketMetadata{ + Forward: &ForwardMetadata{ + Receiver: userC.FormattedAddress(), + Channel: bcChan.ChannelID, + Port: bcChan.PortID, + Next: &next, + }, + } + + memo, err = json.Marshal(firstHopMetadata) + require.NoError(t, err) + + opts = ibc.TransferOptions{ + Memo: string(memo), + } + + chainAHeight, err = chainA.Height(ctx) + require.NoError(t, err) + + transferTx, err = chainA.SendIBCTransfer(ctx, abChan.ChannelID, userA.KeyName(), transfer, opts) + require.NoError(t, err) + + _, err = testutil.PollForAck(ctx, chainA, chainAHeight, chainAHeight+30, transferTx.Packet) + require.NoError(t, err) + + err = testutil.WaitForBlocks(ctx, 5, chainA) + require.NoError(t, err) + + // Assert balances are updated to reflect tokens now being on ChainD + chainABalance, err = chainA.GetBalance(ctx, userA.FormattedAddress(), chainA.Config().Denom) + require.NoError(t, err) + + chainBBalance, err = chainB.GetBalance(ctx, userB.FormattedAddress(), firstHopIBCDenom) + require.NoError(t, err) + + chainCBalance, err = chainC.GetBalance(ctx, userC.FormattedAddress(), secondHopIBCDenom) + require.NoError(t, err) + + chainDBalance, err = chainD.GetBalance(ctx, userD.FormattedAddress(), thirdHopIBCDenom) + require.NoError(t, err) + + require.True(t, chainABalance.Equal(initBal.Sub(transferAmount))) + require.True(t, chainBBalance.Equal(zeroBal)) + require.True(t, chainCBalance.Equal(zeroBal)) + require.True(t, chainDBalance.Equal(transferAmount)) + + firstHopEscrowBalance, err = chainA.GetBalance(ctx, firstHopEscrowAccount, chainA.Config().Denom) + require.NoError(t, err) + + secondHopEscrowBalance, err = chainB.GetBalance(ctx, secondHopEscrowAccount, firstHopIBCDenom) + require.NoError(t, err) + + thirdHopEscrowBalance, err = chainC.GetBalance(ctx, thirdHopEscrowAccount, secondHopIBCDenom) + require.NoError(t, err) + + require.True(t, firstHopEscrowBalance.Equal(transferAmount)) + require.True(t, secondHopEscrowBalance.Equal(transferAmount)) + require.True(t, thirdHopEscrowBalance.Equal(transferAmount)) + + // Compose IBC tx that will attempt to go from ChainD -> ChainC -> ChainB -> ChainA but timeout between ChainB->ChainA + transfer = ibc.WalletAmount{ + Address: userC.FormattedAddress(), + Denom: thirdHopDenom, + Amount: transferAmount, + } + + secondHopMetadata = &PacketMetadata{ + Forward: &ForwardMetadata{ + Receiver: userA.FormattedAddress(), + Channel: baChan.ChannelID, + Port: baChan.PortID, + Timeout: 1 * time.Second, + }, + } + nextBz, err = json.Marshal(secondHopMetadata) + require.NoError(t, err) + next = string(nextBz) + + firstHopMetadata = &PacketMetadata{ + Forward: &ForwardMetadata{ + Receiver: userB.FormattedAddress(), + Channel: cbChan.ChannelID, + Port: cbChan.PortID, + Next: &next, + }, + } + + memo, err = json.Marshal(firstHopMetadata) + require.NoError(t, err) + + chainDHeight, err := chainD.Height(ctx) + require.NoError(t, err) + + transferTx, err = chainD.SendIBCTransfer(ctx, dcChan.ChannelID, userD.KeyName(), transfer, ibc.TransferOptions{Memo: string(memo)}) + require.NoError(t, err) + + _, err = testutil.PollForAck(ctx, chainD, chainDHeight, chainDHeight+25, transferTx.Packet) + require.NoError(t, err) + + err = testutil.WaitForBlocks(ctx, 5, chainD) + require.NoError(t, err) + + // Assert balances to ensure timeout happened and user funds are still present on ChainD + chainABalance, err = chainA.GetBalance(ctx, userA.FormattedAddress(), chainA.Config().Denom) + require.NoError(t, err) + + chainBBalance, err = chainB.GetBalance(ctx, userB.FormattedAddress(), firstHopIBCDenom) + require.NoError(t, err) + + chainCBalance, err = chainC.GetBalance(ctx, userC.FormattedAddress(), secondHopIBCDenom) + require.NoError(t, err) + + chainDBalance, err = chainD.GetBalance(ctx, userD.FormattedAddress(), thirdHopIBCDenom) + require.NoError(t, err) + + require.True(t, chainABalance.Equal(initBal.Sub(transferAmount))) + require.True(t, chainBBalance.Equal(zeroBal)) + require.True(t, chainCBalance.Equal(zeroBal)) + require.True(t, chainDBalance.Equal(transferAmount)) + + firstHopEscrowBalance, err = chainA.GetBalance(ctx, firstHopEscrowAccount, chainA.Config().Denom) + require.NoError(t, err) + + secondHopEscrowBalance, err = chainB.GetBalance(ctx, secondHopEscrowAccount, firstHopIBCDenom) + require.NoError(t, err) + + thirdHopEscrowBalance, err = chainC.GetBalance(ctx, thirdHopEscrowAccount, secondHopIBCDenom) + require.NoError(t, err) + + require.True(t, firstHopEscrowBalance.Equal(transferAmount)) + require.True(t, secondHopEscrowBalance.Equal(transferAmount)) + require.True(t, thirdHopEscrowBalance.Equal(transferAmount)) } diff --git a/middleware/packet-forward-middleware/e2e/go.mod b/middleware/packet-forward-middleware/e2e/go.mod index e4e2b1a7..54305ac8 100644 --- a/middleware/packet-forward-middleware/e2e/go.mod +++ b/middleware/packet-forward-middleware/e2e/go.mod @@ -12,6 +12,7 @@ require ( github.com/strangelove-ventures/interchaintest/v7 v7.0.1-0.20231121220910-a334ab4006ae github.com/stretchr/testify v1.8.4 go.uber.org/zap v1.26.0 + golang.org/x/sync v0.4.0 ) require ( @@ -202,7 +203,6 @@ require ( golang.org/x/mod v0.12.0 // indirect golang.org/x/net v0.17.0 // indirect golang.org/x/oauth2 v0.11.0 // indirect - golang.org/x/sync v0.4.0 // indirect golang.org/x/sys v0.13.0 // indirect golang.org/x/term v0.13.0 // indirect golang.org/x/text v0.13.0 // indirect diff --git a/middleware/packet-forward-middleware/packetforward/keeper/keeper.go b/middleware/packet-forward-middleware/packetforward/keeper/keeper.go index 998831ae..f195c58e 100644 --- a/middleware/packet-forward-middleware/packetforward/keeper/keeper.go +++ b/middleware/packet-forward-middleware/packetforward/keeper/keeper.go @@ -224,41 +224,40 @@ func (k *Keeper) WriteAcknowledgementForForwardedPacket( } } + amount, ok := sdk.NewIntFromString(data.Amount) + if !ok { + return fmt.Errorf("failed to parse amount from packet data for forward refund: %s", data.Amount) + } + + denomTrace := transfertypes.ParseDenomTrace(fullDenomPath) + token := sdk.NewCoin(denomTrace.IBCDenom(), amount) + + escrowAddress := transfertypes.GetEscrowAddress(packet.SourcePort, packet.SourceChannel) + refundEscrowAddress := transfertypes.GetEscrowAddress(inFlightPacket.RefundPortId, inFlightPacket.RefundChannelId) + + newToken := sdk.NewCoins(token) + if transfertypes.SenderChainIsSource(packet.SourcePort, packet.SourceChannel, fullDenomPath) { // funds were moved to escrow account for transfer, so they need to either: // - move to the other escrow account, in the case of native denom // - burn - - amount, ok := sdk.NewIntFromString(data.Amount) - if !ok { - return fmt.Errorf("failed to parse amount from packet data for forward refund: %s", data.Amount) - } - denomTrace := transfertypes.ParseDenomTrace(fullDenomPath) - token := sdk.NewCoin(denomTrace.IBCDenom(), amount) - - escrowAddress := transfertypes.GetEscrowAddress(packet.SourcePort, packet.SourceChannel) - if transfertypes.SenderChainIsSource(inFlightPacket.RefundPortId, inFlightPacket.RefundChannelId, fullDenomPath) { // transfer funds from escrow account for forwarded packet to escrow account going back for refund. - - refundEscrowAddress := transfertypes.GetEscrowAddress(inFlightPacket.RefundPortId, inFlightPacket.RefundChannelId) - if err := k.bankKeeper.SendCoins( - ctx, escrowAddress, refundEscrowAddress, sdk.NewCoins(token), + ctx, escrowAddress, refundEscrowAddress, newToken, ); err != nil { return fmt.Errorf("failed to send coins from escrow account to refund escrow account: %w", err) } } else { // transfer the coins from the escrow account to the module account and burn them. - if err := k.bankKeeper.SendCoinsFromAccountToModule( - ctx, escrowAddress, transfertypes.ModuleName, sdk.NewCoins(token), + ctx, escrowAddress, transfertypes.ModuleName, newToken, ); err != nil { return fmt.Errorf("failed to send coins from escrow to module account for burn: %w", err) } if err := k.bankKeeper.BurnCoins( - ctx, transfertypes.ModuleName, sdk.NewCoins(token), + ctx, transfertypes.ModuleName, newToken, ); err != nil { // NOTE: should not happen as the module account was // retrieved on the step above and it has enough balace @@ -270,6 +269,20 @@ func (k *Keeper) WriteAcknowledgementForForwardedPacket( // We move funds from the escrowAddress in both cases, // update the total escrow amount for the denom. k.unescrowToken(ctx, token) + } else { + // Funds in the escrow account were burned, + // so on a timeout or acknowledgement error we need to mint the funds back to the escrow account. + if err := k.bankKeeper.MintCoins(ctx, transfertypes.ModuleName, newToken); err != nil { + return fmt.Errorf("cannot mint coins to the %s module account: %v", transfertypes.ModuleName, err) + } + + if err := k.bankKeeper.SendCoinsFromModuleToAccount(ctx, transfertypes.ModuleName, refundEscrowAddress, newToken); err != nil { + return fmt.Errorf("cannot send coins from the %s module to the escrow account %s: %v", transfertypes.ModuleName, refundEscrowAddress, err) + } + + currentTotalEscrow := k.transferKeeper.GetTotalEscrowForDenom(ctx, token.GetDenom()) + newTotalEscrow := currentTotalEscrow.Add(token) + k.transferKeeper.SetTotalEscrowForDenom(ctx, newTotalEscrow) } }