fix: don't unescrow tokens that are moved between escrow accounts (#230)

* fix: don't unescrow tokens that are moved between escrow accounts

* add more tests

* consolidate new tests

* feat(pfm): Migration to align expected escrow state (#231)

* feat(packetforward): Bump consensus version from 2 to 3

* feat(packetforward): Add empty migration setup

* test(packetforward): Initialize mocks and tests for migration

* feat(packetforward): Implement migration

* fix(packetforward): Use correct expected keeper method names and parameters

* docs: Migration test steps

* test(packetforward): Upgrade e2e test

* fix: Upgrade in app

* fix: Use preblock for upgrade

* fix: Set missing app PreBlocker

* fix: Use corrected 8.1.0 pre-upgrade image

SetPreBlocker was not set, so upgrade module did not trigger. Also,
upgrade handlers should not be set in the previous binary.

* ci: Update deprecated artifacts actions

* ci: Update golangci/golangci-lint-action

* docs: Remove inaccurate previous image upgrade info

* ci: Update e2e test to use 8.1.0 pre-upgrade docker image

* docs: Test case docs

* build: Replace 8.1.0 pre-upgrade image with linux/amd64

* test: Enable upgrade handlers in local simapp for upgrade e2e test

---------

Co-authored-by: drklee3 <[email protected]>

Author: wllmshao

.github/workflows/packet-forward-middleware.yml

@@ -22,7 +22,7 @@ jobs:
       - uses: actions/checkout@v3
 
       - name: golangci-lint
-        uses: golangci/golangci-lint-action@v2
+        uses: golangci/golangci-lint-action@v6
         with:
           version: ${{ env.LINT_VERSION }}
           working-directory: ${{ env.WORKING_DIRECTORY }}
@@ -64,7 +64,7 @@ jobs:
           outputs: type=docker,dest=${{ env.TAR_PATH }}
 
       - name: Upload artifact
-        uses: actions/upload-artifact@v3
+        uses: actions/upload-artifact@v4
         with:
           name: ${{ env.IMAGE_NAME }}
           path: ${{ env.TAR_PATH }}
@@ -94,15 +94,15 @@ jobs:
       - uses: actions/checkout@v4
 
       - name: Download Tarball Artifact
-        uses: actions/download-artifact@v3
+        uses: actions/download-artifact@v4
         with:
           name: ${{ env.IMAGE_NAME }}
           path: /tmp
 
       - name: Load Docker Image
         run: |
           docker image load -i ${{ env.TAR_PATH }}
-          docker image load -i testing/previous_images/pfm_7_0_1.tar
+          docker image load -i testing/previous_images/pfm_8_1_0.tar
           docker image ls -a
 
       - name: Run Test

middleware/packet-forward-middleware/Dockerfile

@@ -1,7 +1,7 @@
 # docker build . -t pfm:local
 # docker run --rm -it pfm:local q ibc-router
 
-FROM golang:1.21-alpine3.18 as builder
+FROM golang:1.21-alpine3.18 AS builder
 
 RUN set -eux; apk add --no-cache git libusb-dev linux-headers gcc musl-dev make;
 

middleware/packet-forward-middleware/e2e/packet_forward_test.go

@@ -3,6 +3,7 @@ package e2e
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"testing"
 	"time"
 
@@ -17,6 +18,8 @@ import (
 	"github.com/strangelove-ventures/interchaintest/v8/testutil"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/zap/zaptest"
+	"google.golang.org/grpc"
+	"google.golang.org/grpc/credentials/insecure"
 )
 
 type PacketMetadata struct {
@@ -633,6 +636,47 @@ func TestPacketForwardMiddleware(t *testing.T) {
 		require.True(t, baEscrowBalance.Equal(transferAmount))
 		require.True(t, bcEscrowBalance.Equal(zeroBal))
 		require.True(t, cdEscrowBalance.Equal(zeroBal))
+
+		conn, err := grpc.Dial(chainB.GetHostGRPCAddress(), grpc.WithTransportCredentials(insecure.NewCredentials()))
+		require.NoError(t, err)
+		defer conn.Close()
+
+		queryClient := transfertypes.NewQueryClient(conn)
+
+		req := &transfertypes.QueryTotalEscrowForDenomRequest{Denom: chainB.Config().Denom}
+		res, err := queryClient.TotalEscrowForDenom(ctx, req)
+		require.NoError(t, err)
+
+		// assert reported total escrow balance does not change
+		require.True(t, baEscrowBalance.Equal(res.Amount.Amount), fmt.Sprintf("expected B->A escrow amount %s to equal reported B total escrow %s", baEscrowBalance.String(), res.Amount.Amount.String()))
+
+		// verify that failing another A->B->C->D works
+		chainAHeight, err = chainA.Height(ctx)
+		require.NoError(t, err)
+
+		transferTx, err = chainA.SendIBCTransfer(ctx, abChan.ChannelID, userA.KeyName(), transfer, ibc.TransferOptions{Memo: string(memo)})
+		require.NoError(t, err)
+		_, err = testutil.PollForAck(ctx, chainA, chainAHeight, chainAHeight+30, transferTx.Packet)
+		require.NoError(t, err)
+		err = testutil.WaitForBlocks(ctx, waitBlocks, chainA)
+		require.NoError(t, err)
+
+		// verify that unwinding from A to B works
+		aToBTransfer := ibc.WalletAmount{
+			Address: userB.FormattedAddress(),
+			Denom:   baIBCDenom,
+			Amount:  transferAmount,
+		}
+
+		chainAHeight, err = chainA.Height(ctx)
+		require.NoError(t, err)
+
+		transferTx, err = chainA.SendIBCTransfer(ctx, abChan.ChannelID, userA.KeyName(), aToBTransfer, ibc.TransferOptions{})
+		require.NoError(t, err)
+		_, err = testutil.PollForAck(ctx, chainA, chainAHeight, chainAHeight+10, transferTx.Packet)
+		require.NoError(t, err)
+		err = testutil.WaitForBlocks(ctx, waitBlocks, chainA)
+		require.NoError(t, err)
 	})
 
 	t.Run("forward a->b->a", func(t *testing.T) {