From 8987888c6a0a4ed9b0f88ab233f361078111584f Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Wed, 20 Jul 2022 15:31:40 +0100 Subject: [PATCH 01/10] Test for AsyncSingleSender (E2E #6) (#1682) --- e2e/fee_middleware_test.go | 125 +++++++++++++++++++++++++++++++++++-- e2e/go.mod | 6 +- e2e/testsuite/testsuite.go | 38 ++++++++++- e2e/testvalues/values.go | 20 ++++++ 4 files changed, 179 insertions(+), 10 deletions(-) create mode 100644 e2e/testvalues/values.go diff --git a/e2e/fee_middleware_test.go b/e2e/fee_middleware_test.go index a6532a8fd1f..f8c05822999 100644 --- a/e2e/fee_middleware_test.go +++ b/e2e/fee_middleware_test.go @@ -8,9 +8,11 @@ import ( "github.com/strangelove-ventures/ibctest/broadcast" "github.com/strangelove-ventures/ibctest/chain/cosmos" "github.com/strangelove-ventures/ibctest/ibc" + "github.com/strangelove-ventures/ibctest/test" "github.com/stretchr/testify/suite" "e2e/testsuite" + "e2e/testvalues" feetypes "github.com/cosmos/ibc-go/v4/modules/apps/29-fee/types" channeltypes "github.com/cosmos/ibc-go/v4/modules/core/04-channel/types" @@ -75,11 +77,124 @@ func (s *FeeMiddlewareTestSuite) QueryIncentivizedPacketsForChannel( return res.IncentivizedPackets, err } -func (s *FeeMiddlewareTestSuite) TestPlaceholder() { - ctx := context.Background() - r := s.SetupChainsRelayerAndChannel(ctx, feeMiddlewareChannelOptions()) - s.T().Run("start relayer", func(t *testing.T) { - s.StartRelayer(r) +func (s *FeeMiddlewareTestSuite) TestMsgPayPacketFeeAsyncSingleSender() { + t := s.T() + ctx := context.TODO() + + relayer, channelA := s.SetupChainsRelayerAndChannel(ctx, feeMiddlewareChannelOptions()) + chainA, chainB := s.GetChains() + + var ( + chainADenom = chainA.Config().Denom + testFee = testvalues.DefaultFee(chainADenom) + chainATx ibc.Tx + payPacketFeeTxResp sdk.TxResponse + ) + + chainAWallet := s.CreateUserOnChainA(ctx, testvalues.StartingTokenAmount) + + t.Run("relayer wallets recovered", func(t *testing.T) { + err := s.RecoverRelayerWallets(ctx, relayer) + s.Require().NoError(err) + }) + + chainARelayerWallet, chainBRelayerWallet, err := s.GetRelayerWallets(relayer) + t.Run("relayer wallets fetched", func(t *testing.T) { + s.Require().NoError(err) + }) + + s.Require().NoError(test.WaitForBlocks(ctx, 1, chainA, chainB), "failed to wait for blocks") + + _, chainBRelayerUser := s.GetRelayerUsers(ctx) + + t.Run("register counter party payee", func(t *testing.T) { + resp, err := s.RegisterCounterPartyPayee(ctx, chainB, chainBRelayerUser, channelA.Counterparty.PortID, channelA.Counterparty.ChannelID, chainBRelayerWallet.Address, chainARelayerWallet.Address) + s.Require().NoError(err) + s.AssertValidTxResponse(resp) + }) + + t.Run("verify counter party payee", func(t *testing.T) { + address, err := s.QueryCounterPartyPayee(ctx, chainB, chainBRelayerWallet.Address, channelA.Counterparty.ChannelID) + s.Require().NoError(err) + s.Require().Equal(chainARelayerWallet.Address, address) + }) + + walletAmount := ibc.WalletAmount{ + Address: chainAWallet.Bech32Address(chainB.Config().Bech32Prefix), // destination address + Denom: chainADenom, + Amount: testvalues.IBCTransferAmount, + } + + t.Run("send IBC transfer", func(t *testing.T) { + chainATx, err = chainA.SendIBCTransfer(ctx, channelA.ChannelID, chainAWallet.KeyName, walletAmount, nil) + s.Require().NoError(err) + s.Require().NoError(chainATx.Validate(), "chain-a ibc transfer tx is invalid") + }) + + t.Run("tokens are escrowed", func(t *testing.T) { + actualBalance, err := s.GetChainANativeBalance(ctx, chainAWallet) + s.Require().NoError(err) + + expected := testvalues.StartingTokenAmount - walletAmount.Amount - chainA.GetGasFeesInNativeDenom(chainATx.GasSpent) + s.Require().Equal(expected, actualBalance) + }) + + t.Run("pay packet fee", func(t *testing.T) { + t.Run("no incentivized packets", func(t *testing.T) { + packets, err := s.QueryIncentivizedPacketsForChannel(ctx, chainA, channelA.PortID, channelA.ChannelID) + s.Require().NoError(err) + s.Require().Empty(packets) + }) + + packetId := channeltypes.NewPacketId(channelA.PortID, channelA.ChannelID, 1) + packetFee := feetypes.NewPacketFee(testFee, chainAWallet.Bech32Address(chainA.Config().Bech32Prefix), nil) + + t.Run("should succeed", func(t *testing.T) { + payPacketFeeTxResp, err = s.PayPacketFeeAsync(ctx, chainA, chainAWallet, packetId, packetFee) + s.Require().NoError(err) + s.AssertValidTxResponse(payPacketFeeTxResp) + }) + + t.Run("there should be incentivized packets", func(t *testing.T) { + packets, err := s.QueryIncentivizedPacketsForChannel(ctx, chainA, channelA.PortID, channelA.ChannelID) + s.Require().NoError(err) + s.Require().Len(packets, 1) + actualFee := packets[0].PacketFees[0].Fee + + s.Require().True(actualFee.RecvFee.IsEqual(testFee.RecvFee)) + s.Require().True(actualFee.AckFee.IsEqual(testFee.AckFee)) + s.Require().True(actualFee.TimeoutFee.IsEqual(testFee.TimeoutFee)) + }) + + t.Run("balance should be lowered by sum of recv ack and timeout", func(t *testing.T) { + // The balance should be lowered by the sum of the recv, ack and timeout fees. + actualBalance, err := s.GetChainANativeBalance(ctx, chainAWallet) + s.Require().NoError(err) + + gasFees := chainA.GetGasFeesInNativeDenom(chainATx.GasSpent) + chainA.GetGasFeesInNativeDenom(payPacketFeeTxResp.GasWanted) + expected := testvalues.StartingTokenAmount - walletAmount.Amount - gasFees - testFee.Total().AmountOf(chainADenom).Int64() + s.Require().Equal(expected, actualBalance) + }) + }) + + t.Run("start relayer", func(t *testing.T) { + s.StartRelayer(relayer) + }) + + t.Run("packets are relayed", func(t *testing.T) { + packets, err := s.QueryIncentivizedPacketsForChannel(ctx, chainA, channelA.PortID, channelA.ChannelID) + s.Require().NoError(err) + s.Require().Empty(packets) + }) + + t.Run("timeout fee is refunded", func(t *testing.T) { + actualBalance, err := s.GetChainANativeBalance(ctx, chainAWallet) + s.Require().NoError(err) + + gasFees := chainA.GetGasFeesInNativeDenom(chainATx.GasSpent) + chainA.GetGasFeesInNativeDenom(payPacketFeeTxResp.GasWanted) + // once the relayer has relayed the packets, the timeout fee should be refunded. + expected := testvalues.StartingTokenAmount - walletAmount.Amount - gasFees - testFee.AckFee.AmountOf(chainADenom).Int64() - testFee.RecvFee.AmountOf(chainADenom).Int64() + s.Require().Equal(expected, actualBalance) }) } diff --git a/e2e/go.mod b/e2e/go.mod index 5b80823e0f3..feb8f7f43c1 100644 --- a/e2e/go.mod +++ b/e2e/go.mod @@ -5,10 +5,13 @@ go 1.18 replace github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1 require ( + github.com/cosmos/cosmos-sdk v0.45.6 + github.com/cosmos/ibc-go/v4 v4.0.0-rc0 github.com/docker/docker v20.10.17+incompatible github.com/strangelove-ventures/ibctest v0.0.0-20220713213153-930886d8db30 github.com/stretchr/testify v1.8.0 go.uber.org/zap v1.21.0 + google.golang.org/grpc v1.47.0 ) require ( @@ -27,10 +30,8 @@ require ( github.com/cespare/xxhash/v2 v2.1.2 // indirect github.com/confio/ics23/go v0.7.0 // indirect github.com/cosmos/btcutil v1.0.4 // indirect - github.com/cosmos/cosmos-sdk v0.45.6 // indirect github.com/cosmos/go-bip39 v1.0.0 // indirect github.com/cosmos/iavl v0.17.3 // indirect - github.com/cosmos/ibc-go/v4 v4.0.0-rc0 // indirect github.com/cosmos/ledger-cosmos-go v0.11.1 // indirect github.com/cosmos/ledger-go v0.9.2 // indirect github.com/danieljoos/wincred v1.0.2 // indirect @@ -124,7 +125,6 @@ require ( golang.org/x/tools v0.1.10 // indirect golang.org/x/xerrors v0.0.0-20220517211312-f3a8303e98df // indirect google.golang.org/genproto v0.0.0-20220519153652-3a47de7e79bd // indirect - google.golang.org/grpc v1.47.0 // indirect google.golang.org/protobuf v1.28.0 // indirect gopkg.in/ini.v1 v1.66.4 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect diff --git a/e2e/testsuite/testsuite.go b/e2e/testsuite/testsuite.go index d8b5387b305..7874f956639 100644 --- a/e2e/testsuite/testsuite.go +++ b/e2e/testsuite/testsuite.go @@ -65,11 +65,33 @@ func newPath(chainA, chainB *cosmos.CosmosChain) path { } } +// GetRelayerUsers returns two ibctest.User instances which can be used for the relayer users +// on the two chains. +func (s *E2ETestSuite) GetRelayerUsers(ctx context.Context, chainOpts ...testconfig.ChainOptionConfiguration) (*ibctest.User, *ibctest.User) { + chainA, chainB := s.GetChains(chainOpts...) + chainAAccountBytes, err := chainA.GetAddress(ctx, ChainARelayerName) + s.Require().NoError(err) + + chainBAccountBytes, err := chainB.GetAddress(ctx, ChainBRelayerName) + s.Require().NoError(err) + + chainARelayerUser := ibctest.User{ + Address: chainAAccountBytes, + KeyName: ChainARelayerName, + } + + chainBRelayerUser := ibctest.User{ + Address: chainBAccountBytes, + KeyName: ChainBRelayerName, + } + return &chainARelayerUser, &chainBRelayerUser +} + // SetupChainsRelayerAndChannel create two chains, a relayer, establishes a connection and creates a channel // using the given channel options. The relayer returned by this function has not yet started. It should be started // with E2ETestSuite.StartRelayer if needed. // This should be called at the start of every test, unless fine grained control is required. -func (s *E2ETestSuite) SetupChainsRelayerAndChannel(ctx context.Context, channelOpts ...func(*ibc.CreateChannelOptions)) ibc.Relayer { +func (s *E2ETestSuite) SetupChainsRelayerAndChannel(ctx context.Context, channelOpts ...func(*ibc.CreateChannelOptions)) (ibc.Relayer, ibc.ChannelOutput) { chainA, chainB := s.GetChains() home, err := ioutil.TempDir("", "") s.Require().NoError(err) @@ -121,7 +143,9 @@ func (s *E2ETestSuite) SetupChainsRelayerAndChannel(ctx context.Context, channel s.initGRPCClients(chainA) s.initGRPCClients(chainB) - return r + chainAChannels, err := r.GetChannels(ctx, eRep, chainA.Config().ChainID) + s.Require().NoError(err) + return r, chainAChannels[len(chainAChannels)-1] } // GetChains returns two chains that can be used in a test. The pair returned @@ -260,6 +284,16 @@ func (s *E2ETestSuite) initGRPCClients(chain *cosmos.CosmosChain) { } } +// AssertValidTxResponse verifies that an sdk.TxResponse +// has non-empty values. +func (s *E2ETestSuite) AssertValidTxResponse(resp sdk.TxResponse) { + respLogsMsg := resp.Logs.String() + s.Require().NotEqual(int64(0), resp.GasUsed, respLogsMsg) + s.Require().NotEqual(int64(0), resp.GasWanted, respLogsMsg) + s.Require().NotEmpty(resp.Events, respLogsMsg) + s.Require().NotEmpty(resp.Data, respLogsMsg) +} + // createCosmosChains creates two separate chains in docker containers. // test and can be retrieved with GetChains. func (s *E2ETestSuite) createCosmosChains(chainOptions testconfig.ChainOptions) (*cosmos.CosmosChain, *cosmos.CosmosChain) { diff --git a/e2e/testvalues/values.go b/e2e/testvalues/values.go new file mode 100644 index 00000000000..b91cfae1b84 --- /dev/null +++ b/e2e/testvalues/values.go @@ -0,0 +1,20 @@ +package testvalues + +import ( + sdk "github.com/cosmos/cosmos-sdk/types" + + feetypes "github.com/cosmos/ibc-go/v4/modules/apps/29-fee/types" +) + +const ( + StartingTokenAmount int64 = 10_000_000 + IBCTransferAmount int64 = 10_000 +) + +func DefaultFee(denom string) feetypes.Fee { + return feetypes.Fee{ + RecvFee: sdk.NewCoins(sdk.NewCoin(denom, sdk.NewInt(50))), + AckFee: sdk.NewCoins(sdk.NewCoin(denom, sdk.NewInt(25))), + TimeoutFee: sdk.NewCoins(sdk.NewCoin(denom, sdk.NewInt(10))), + } +} From cc74109c1d36030d2c0e3870189efca929103f19 Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Wed, 20 Jul 2022 19:52:50 +0100 Subject: [PATCH 02/10] fix: running e2e-fork for dependabot PRs (#1745) --- .github/workflows/e2e-fork.yml | 4 ++-- .github/workflows/e2e.yaml | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/e2e-fork.yml b/.github/workflows/e2e-fork.yml index b3d8aa9b32b..bda1246521d 100644 --- a/.github/workflows/e2e-fork.yml +++ b/.github/workflows/e2e-fork.yml @@ -8,7 +8,7 @@ on: jobs: # dynamically build a matrix of test/test suite pairs to run build-test-matrix: - if: ${{ github.event.pull_request.head.repo.fork }} + if: ${{ github.event.pull_request.head.repo.fork || github.actor == 'dependabot[bot]' }} runs-on: ubuntu-latest outputs: matrix: ${{ steps.set-matrix.outputs.matrix }} @@ -24,7 +24,7 @@ jobs: env: SIMD_TAG: latest SIMD_IMAGE: ibc-go-simd-e2e - if: ${{ github.event.pull_request.head.repo.fork }} + if: ${{ github.event.pull_request.head.repo.fork || github.actor == 'dependabot[bot]' }} needs: - build-test-matrix runs-on: ubuntu-latest diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index 12f52ef098e..989a73285f2 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -13,7 +13,7 @@ env: jobs: docker-build: - if: ${{ !github.event.pull_request.head.repo.fork }} + if: ${{ !github.event.pull_request.head.repo.fork && github.actor != 'dependabot[bot]' }} runs-on: ubuntu-latest steps: - uses: actions/checkout@v3 @@ -40,7 +40,7 @@ jobs: # dynamically build a matrix of test/test suite pairs to run build-test-matrix: - if: ${{ !github.event.pull_request.head.repo.fork }} + if: ${{ !github.event.pull_request.head.repo.fork && github.actor != 'dependabot[bot]' }} runs-on: ubuntu-latest outputs: matrix: ${{ steps.set-matrix.outputs.matrix }} @@ -56,7 +56,7 @@ jobs: # the tag of the image will differ if this is a PR or the branch is being merged into main. # we store the tag as an environment variable and use it in the E2E tests to determine the tag. determine-image-tag: - if: ${{ !github.event.pull_request.head.repo.fork }} + if: ${{ !github.event.pull_request.head.repo.fork && github.actor != 'dependabot[bot]' }} runs-on: ubuntu-latest outputs: simd-tag: ${{ steps.get-tag.outputs.simd-tag }} @@ -73,7 +73,7 @@ jobs: e2e: - if: ${{ !github.event.pull_request.head.repo.fork }} + if: ${{ !github.event.pull_request.head.repo.fork && github.actor != 'dependabot[bot]' }} runs-on: ubuntu-latest needs: - build-test-matrix From 5f5a2871803910e123ff1ea8c9944f29c62bcef8 Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Thu, 21 Jul 2022 09:59:02 +0100 Subject: [PATCH 03/10] Adding manual triggering of e2e via workflow dispatch (#1749) --- .github/workflows/e2e-fork.yml | 6 ++++-- .github/workflows/e2e.yaml | 1 + 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/.github/workflows/e2e-fork.yml b/.github/workflows/e2e-fork.yml index bda1246521d..8d7e510fe22 100644 --- a/.github/workflows/e2e-fork.yml +++ b/.github/workflows/e2e-fork.yml @@ -1,14 +1,16 @@ name: Tests / E2E Fork on: + workflow_dispatch: pull_request: branches: - main paths-ignore: - docs/** + jobs: # dynamically build a matrix of test/test suite pairs to run build-test-matrix: - if: ${{ github.event.pull_request.head.repo.fork || github.actor == 'dependabot[bot]' }} + if: ${{ github.event.pull_request.head.repo.fork || github.actor == 'dependabot[bot]' || github.event_name == 'workflow_dispatch' }} runs-on: ubuntu-latest outputs: matrix: ${{ steps.set-matrix.outputs.matrix }} @@ -24,7 +26,7 @@ jobs: env: SIMD_TAG: latest SIMD_IMAGE: ibc-go-simd-e2e - if: ${{ github.event.pull_request.head.repo.fork || github.actor == 'dependabot[bot]' }} + if: ${{ github.event.pull_request.head.repo.fork || github.actor == 'dependabot[bot]' || github.event_name == 'workflow_dispatch' }} needs: - build-test-matrix runs-on: ubuntu-latest diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml index 989a73285f2..80aba365318 100644 --- a/.github/workflows/e2e.yaml +++ b/.github/workflows/e2e.yaml @@ -1,5 +1,6 @@ name: Tests / E2E on: + workflow_dispatch: pull_request: push: branches: From be5ccf3a8007c69502fd34ba281112720a2f8578 Mon Sep 17 00:00:00 2001 From: Charly Date: Thu, 21 Jul 2022 12:13:36 +0200 Subject: [PATCH 04/10] chore: denom traces migration handler (#1680) * update code & test * register migrator service --- CHANGELOG.md | 1 + .../migrations/support-denoms-with-slashes.md | 34 +---- docs/migrations/v3-to-v4.md | 22 +++- modules/apps/transfer/keeper/migrations.go | 59 +++++++++ .../apps/transfer/keeper/migrations_test.go | 123 ++++++++++++++++++ modules/apps/transfer/module.go | 7 +- modules/apps/transfer/types/trace.go | 27 +++- modules/apps/transfer/types/trace_test.go | 58 +++++---- 8 files changed, 265 insertions(+), 66 deletions(-) create mode 100644 modules/apps/transfer/keeper/migrations.go create mode 100644 modules/apps/transfer/keeper/migrations_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index db771acd750..e59ec65cc8b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,6 +59,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements +* (app/20-transfer) [\#1730](https://github.com/cosmos/ibc-go/pull/1730) parse the ics20 denomination provided via a packet using the channel identifier format specified by ibc-go. * (cleanup) [\#1335](https://github.com/cosmos/ibc-go/pull/1335/) `gofumpt -w -l .` to standardize the code layout more strictly than `go fmt ./...` * (middleware) [\#1022](https://github.com/cosmos/ibc-go/pull/1022) Add `GetAppVersion` to the ICS4Wrapper interface. This function should be used by IBC applications to obtain their own version since the version set in the channel structure may be wrapped many times by middleware. * (modules/core/04-channel) [\#1232](https://github.com/cosmos/ibc-go/pull/1232) Updating params on `NewPacketId` and moving to bottom of file. diff --git a/docs/migrations/support-denoms-with-slashes.md b/docs/migrations/support-denoms-with-slashes.md index 40fad8272f8..3bc3d1b6b83 100644 --- a/docs/migrations/support-denoms-with-slashes.md +++ b/docs/migrations/support-denoms-with-slashes.md @@ -9,7 +9,7 @@ There are four sections based on the four potential user groups of this document - Relayers - IBC Light Clients -This document is necessary when chains are upgrading from a version that does not support base denoms with slashes (e.g. v3.0.0) to a version that does (e.g. v3.1.0). All versions of ibc-go smaller than v1.5.0 for the v1.x release line, v2.3.0 for the v2.x release line, and v3.1.0 for the v3.x release line do *NOT** support IBC token transfers of coins whose base denoms contain slashes. Therefore the in-place of genesis migration described in this document are required when upgrading. +This document is necessary when chains are upgrading from a version that does not support base denoms with slashes (e.g. v3.0.0) to a version that does (e.g. v3.2.0). All versions of ibc-go smaller than v1.5.0 for the v1.x release line, v2.3.0 for the v2.x release line, and v3.1.0 for the v3.x release line do **NOT** support IBC token transfers of coins whose base denoms contain slashes. Therefore the in-place of genesis migration described in this document are required when upgrading. If a chain receives coins of a base denom with slashes before it upgrades to supporting it, the receive may pass however the trace information will be incorrect. @@ -28,41 +28,17 @@ The transfer module will now support slashes in base denoms, so we must iterate ### Upgrade Proposal ```go -// Here the upgrade name is the upgrade name set by the chain -app.UpgradeKeeper.SetUpgradeHandler("supportSlashedDenomsUpgrade", +app.UpgradeKeeper.SetUpgradeHandler("MigrateTraces", func(ctx sdk.Context, _ upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) { - // list of traces that must replace the old traces in store - var newTraces []ibctransfertypes.DenomTrace - app.TransferKeeper.IterateDenomTraces(ctx, - func(dt ibctransfertypes.DenomTrace) bool { - // check if the new way of splitting FullDenom - // into Trace and BaseDenom passes validation and - // is the same as the current DenomTrace. - // If it isn't then store the new DenomTrace in the list of new traces. - newTrace := ibctransfertypes.ParseDenomTrace(dt.GetFullDenomPath()) - if err := newTrace.Validate(); err == nil && !equalTraces(newTrace, dt) { - newTraces = append(newTraces, newTrace) - } - - return false - }) - - // replace the outdated traces with the new trace information - for _, nt := range newTraces { - app.TransferKeeper.SetDenomTrace(ctx, nt) - } - + // transfer module consensus version has been bumped to 2 return app.mm.RunMigrations(ctx, app.configurator, fromVM) }) - -func equalTraces(dtA, dtB ibctransfertypes.DenomTrace) bool { - return dtA.BaseDenom == dtB.BaseDenom && dtA.Path == dtB.Path -} + ``` This is only necessary if there are denom traces in the store with incorrect trace information from previously received coins that had a slash in the base denom. However, it is recommended that any chain upgrading to support base denominations with slashes runs this code for safety. -For a more detailed sample, please check out the code changes in [this pull request](https://github.com/cosmos/ibc-go/pull/1527). +For a more detailed sample, please check out the code changes in [this pull request](https://github.com/cosmos/ibc-go/pull/1680). ### Genesis Migration diff --git a/docs/migrations/v3-to-v4.md b/docs/migrations/v3-to-v4.md index e88444163c1..bdf8654dcb7 100644 --- a/docs/migrations/v3-to-v4.md +++ b/docs/migrations/v3-to-v4.md @@ -18,7 +18,27 @@ No genesis or in-place migrations required when upgrading from v1 or v2 of ibc-g ## Chains -- No relevant changes were made in this release. +### Migration to fix support for base denoms with slashes + +As part of [v1.5.0](https://github.com/cosmos/ibc-go/releases/tag/v1.5.0), [v2.3.0](https://github.com/cosmos/ibc-go/releases/tag/v2.3.0) and [v3.1.0](https://github.com/cosmos/ibc-go/releases/tag/v3.1.0) some [migration handler code sample was documented](https://github.com/cosmos/ibc-go/blob/main/docs/migrations/support-denoms-with-slashes.md#upgrade-proposal) that needs to run in order to correct the trace information of coins transferred using ICS20 whose base denom contains slashes. + +Based on feedback from the community we add now an improved solution to run the same migration that does not require copying a large piece of code over from the migration document, but instead requires only adding a one-line upgrade handler. + +If the chain will migrate to supporting base denoms with slashes, it must set the appropriate params during the execution of the upgrade handler in `app.go`: +```go +app.UpgradeKeeper.SetUpgradeHandler("MigrateTraces", + func(ctx sdk.Context, _ upgradetypes.Plan, fromVM module.VersionMap) (module.VersionMap, error) { + // transfer module consensus version has been bumped to 2 + return app.mm.RunMigrations(ctx, app.configurator, fromVM) + }) + +``` + +If a chain receives coins of a base denom with slashes before it upgrades to supporting it, the receive may pass however the trace information will be incorrect. + +E.g. If a base denom of `testcoin/testcoin/testcoin` is sent to a chain that does not support slashes in the base denom, the receive will be successful. However, the trace information stored on the receiving chain will be: `Trace: "transfer/{channel-id}/testcoin/testcoin", BaseDenom: "testcoin"`. + +This incorrect trace information must be corrected when the chain does upgrade to fully supporting denominations with slashes. ## IBC Apps diff --git a/modules/apps/transfer/keeper/migrations.go b/modules/apps/transfer/keeper/migrations.go new file mode 100644 index 00000000000..a1f3654f170 --- /dev/null +++ b/modules/apps/transfer/keeper/migrations.go @@ -0,0 +1,59 @@ +package keeper + +import ( + "fmt" + + sdk "github.com/cosmos/cosmos-sdk/types" + + "github.com/cosmos/ibc-go/v4/modules/apps/transfer/types" +) + +// Migrator is a struct for handling in-place store migrations. +type Migrator struct { + keeper Keeper +} + +// NewMigrator returns a new Migrator. +func NewMigrator(keeper Keeper) Migrator { + return Migrator{keeper: keeper} +} + +// MigrateTraces migrates the DenomTraces to the correct format, accounting for slashes in the BaseDenom. +func (m Migrator) MigrateTraces(ctx sdk.Context) error { + + // list of traces that must replace the old traces in store + var newTraces []types.DenomTrace + m.keeper.IterateDenomTraces(ctx, + func(dt types.DenomTrace) (stop bool) { + // check if the new way of splitting FullDenom + // is the same as the current DenomTrace. + // If it isn't then store the new DenomTrace in the list of new traces. + newTrace := types.ParseDenomTrace(dt.GetFullDenomPath()) + err := newTrace.Validate() + if err != nil { + panic(err) + } + + if dt.IBCDenom() != newTrace.IBCDenom() { + // The new form of parsing will result in a token denomination change. + // A bank migration is required. A panic should occur to prevent the + // chain from using corrupted state. + panic(fmt.Sprintf("migration will result in corrupted state. Previous IBC token (%s) requires a bank migration. Expected denom trace (%s)", dt, newTrace)) + } + + if !equalTraces(newTrace, dt) { + newTraces = append(newTraces, newTrace) + } + return false + }) + + // replace the outdated traces with the new trace information + for _, nt := range newTraces { + m.keeper.SetDenomTrace(ctx, nt) + } + return nil +} + +func equalTraces(dtA, dtB types.DenomTrace) bool { + return dtA.BaseDenom == dtB.BaseDenom && dtA.Path == dtB.Path +} diff --git a/modules/apps/transfer/keeper/migrations_test.go b/modules/apps/transfer/keeper/migrations_test.go new file mode 100644 index 00000000000..ff7da8ca089 --- /dev/null +++ b/modules/apps/transfer/keeper/migrations_test.go @@ -0,0 +1,123 @@ +package keeper_test + +import ( + "fmt" + + transferkeeper "github.com/cosmos/ibc-go/v4/modules/apps/transfer/keeper" + transfertypes "github.com/cosmos/ibc-go/v4/modules/apps/transfer/types" +) + +func (suite *KeeperTestSuite) TestMigratorMigrateTraces() { + + testCases := []struct { + msg string + malleate func() + expectedTraces transfertypes.Traces + }{ + + { + "success: two slashes in base denom", + func() { + suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace( + suite.chainA.GetContext(), + transfertypes.DenomTrace{ + BaseDenom: "pool/1", Path: "transfer/channel-0/gamm", + }) + }, + transfertypes.Traces{ + { + BaseDenom: "gamm/pool/1", Path: "transfer/channel-0", + }, + }, + }, + { + "success: one slash in base denom", + func() { + suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace( + suite.chainA.GetContext(), + transfertypes.DenomTrace{ + BaseDenom: "0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA", Path: "transfer/channel-149/erc", + }) + }, + transfertypes.Traces{ + { + BaseDenom: "erc/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA", Path: "transfer/channel-149", + }, + }, + }, + { + "success: multiple slashes in a row in base denom", + func() { + suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace( + suite.chainA.GetContext(), + transfertypes.DenomTrace{ + BaseDenom: "1", Path: "transfer/channel-5/gamm//pool", + }) + }, + transfertypes.Traces{ + { + BaseDenom: "gamm//pool/1", Path: "transfer/channel-5", + }, + }, + }, + { + "success: multihop base denom", + func() { + suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace( + suite.chainA.GetContext(), + transfertypes.DenomTrace{ + BaseDenom: "transfer/channel-1/uatom", Path: "transfer/channel-0", + }) + }, + transfertypes.Traces{ + { + BaseDenom: "uatom", Path: "transfer/channel-0/transfer/channel-1", + }, + }, + }, + { + "success: non-standard port", + func() { + suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace( + suite.chainA.GetContext(), + transfertypes.DenomTrace{ + BaseDenom: "customport/channel-7/uatom", Path: "transfer/channel-0/transfer/channel-1", + }) + }, + transfertypes.Traces{ + { + BaseDenom: "uatom", Path: "transfer/channel-0/transfer/channel-1/customport/channel-7", + }, + }, + }, + } + + for _, tc := range testCases { + suite.Run(fmt.Sprintf("case %s", tc.msg), func() { + suite.SetupTest() // reset + + tc.malleate() // explicitly set up denom traces + + migrator := transferkeeper.NewMigrator(suite.chainA.GetSimApp().TransferKeeper) + err := migrator.MigrateTraces(suite.chainA.GetContext()) + suite.Require().NoError(err) + + traces := suite.chainA.GetSimApp().TransferKeeper.GetAllDenomTraces(suite.chainA.GetContext()) + suite.Require().Equal(tc.expectedTraces, traces) + }) + } +} + +func (suite *KeeperTestSuite) TestMigratorMigrateTracesCorruptionDetection() { + // IBCDenom() previously would return "customport/channel-0/uatom", but now should return ibc/{hash} + corruptedDenomTrace := transfertypes.DenomTrace{ + BaseDenom: "customport/channel-0/uatom", + Path: "", + } + suite.chainA.GetSimApp().TransferKeeper.SetDenomTrace(suite.chainA.GetContext(), corruptedDenomTrace) + + migrator := transferkeeper.NewMigrator(suite.chainA.GetSimApp().TransferKeeper) + suite.Panics(func() { + migrator.MigrateTraces(suite.chainA.GetContext()) + }) +} diff --git a/modules/apps/transfer/module.go b/modules/apps/transfer/module.go index 11279e4d93f..9503a06d926 100644 --- a/modules/apps/transfer/module.go +++ b/modules/apps/transfer/module.go @@ -120,6 +120,11 @@ func (am AppModule) LegacyQuerierHandler(*codec.LegacyAmino) sdk.Querier { func (am AppModule) RegisterServices(cfg module.Configurator) { types.RegisterMsgServer(cfg.MsgServer(), am.keeper) types.RegisterQueryServer(cfg.QueryServer(), am.keeper) + + m := keeper.NewMigrator(am.keeper) + if err := cfg.RegisterMigration(types.ModuleName, 1, m.MigrateTraces); err != nil { + panic(fmt.Sprintf("failed to migrate transfer app from version 1 to 2: %v", err)) + } } // InitGenesis performs genesis initialization for the ibc-transfer module. It returns @@ -139,7 +144,7 @@ func (am AppModule) ExportGenesis(ctx sdk.Context, cdc codec.JSONCodec) json.Raw } // ConsensusVersion implements AppModule/ConsensusVersion. -func (AppModule) ConsensusVersion() uint64 { return 1 } +func (AppModule) ConsensusVersion() uint64 { return 2 } // BeginBlock implements the AppModule interface func (am AppModule) BeginBlock(ctx sdk.Context, req abci.RequestBeginBlock) { diff --git a/modules/apps/transfer/types/trace.go b/modules/apps/transfer/types/trace.go index 8d15f3bacf0..24f10c232a9 100644 --- a/modules/apps/transfer/types/trace.go +++ b/modules/apps/transfer/types/trace.go @@ -12,6 +12,7 @@ import ( tmbytes "github.com/tendermint/tendermint/libs/bytes" tmtypes "github.com/tendermint/tendermint/types" + channeltypes "github.com/cosmos/ibc-go/v4/modules/core/04-channel/types" host "github.com/cosmos/ibc-go/v4/modules/core/24-host" ) @@ -20,9 +21,9 @@ import ( // // Examples: // -// - "transfer/channelidone/uatom" => DenomTrace{Path: "transfer/channelidone", BaseDenom: "uatom"} -// - "transfer/channelidone/transfer/channelidtwo/uatom" => DenomTrace{Path: "transfer/channelidone/transfer/channelidtwo", BaseDenom: "uatom"} -// - "transfer/channelidone/gamm/pool/1" => DenomTrace{Path: "transfer/channelidone", BaseDenom: "gamm/pool/1"} +// - "portidone/channel-0/uatom" => DenomTrace{Path: "portidone/channel-0", BaseDenom: "uatom"} +// - "portidone/channel-0/portidtwo/channel-1/uatom" => DenomTrace{Path: "portidone/channel-0/portidtwo/channel-1", BaseDenom: "uatom"} +// - "portidone/channel-0/gamm/pool/1" => DenomTrace{Path: "portidone/channel-0", BaseDenom: "gamm/pool/1"} // - "gamm/pool/1" => DenomTrace{Path: "", BaseDenom: "gamm/pool/1"} // - "uatom" => DenomTrace{Path: "", BaseDenom: "uatom"} func ParseDenomTrace(rawDenom string) DenomTrace { @@ -77,11 +78,23 @@ func (dt DenomTrace) GetFullDenomPath() string { // extractPathAndBaseFromFullDenom returns the trace path and the base denom from // the elements that constitute the complete denom. func extractPathAndBaseFromFullDenom(fullDenomItems []string) (string, string) { - var path []string - var baseDenom []string + var ( + path []string + baseDenom []string + ) + length := len(fullDenomItems) for i := 0; i < length; i = i + 2 { - if i < length-1 && length > 2 && fullDenomItems[i] == PortID { + // The IBC specification does not guarentee the expected format of the + // destination port or destination channel identifier. A short term solution + // to determine base denomination is to expect the channel identifier to be the + // one ibc-go specifies. A longer term solution is to separate the path and base + // denomination in the ICS20 packet. If an intermediate hop prefixes the full denom + // with a channel identifier format different from our own, the base denomination + // will be incorrectly parsed, but the token will continue to be treated correctly + // as an IBC denomination. The hash used to store the token internally on our chain + // will be the same value as the base denomination being correctly parsed. + if i < length-1 && length > 2 && channeltypes.IsValidChannelID(fullDenomItems[i+1]) { path = append(path, fullDenomItems[i], fullDenomItems[i+1]) } else { baseDenom = fullDenomItems[i:] @@ -165,7 +178,7 @@ func (t Traces) Sort() Traces { // ValidatePrefixedDenom checks that the denomination for an IBC fungible token packet denom is correctly prefixed. // The function will return no error if the given string follows one of the two formats: // -// - Prefixed denomination: 'transfer/{channelIDN}/.../transfer/{channelID0}/baseDenom' +// - Prefixed denomination: '{portIDN}/{channelIDN}/.../{portID0}/{channelID0}/baseDenom' // - Unprefixed denomination: 'baseDenom' // // 'baseDenom' may or may not contain '/'s diff --git a/modules/apps/transfer/types/trace_test.go b/modules/apps/transfer/types/trace_test.go index 6526cbd952d..ba0690423bd 100644 --- a/modules/apps/transfer/types/trace_test.go +++ b/modules/apps/transfer/types/trace_test.go @@ -17,20 +17,23 @@ func TestParseDenomTrace(t *testing.T) { {"base denom ending with '/'", "uatom/", DenomTrace{BaseDenom: "uatom/"}}, {"base denom with single '/'s", "gamm/pool/1", DenomTrace{BaseDenom: "gamm/pool/1"}}, {"base denom with double '/'s", "gamm//pool//1", DenomTrace{BaseDenom: "gamm//pool//1"}}, - {"trace info", "transfer/channelToA/uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer/channelToA"}}, - {"trace info with base denom ending in '/'", "transfer/channelToA/uatom/", DenomTrace{BaseDenom: "uatom/", Path: "transfer/channelToA"}}, - {"trace info with single '/' in base denom", "transfer/channelToA/erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA", DenomTrace{BaseDenom: "erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA", Path: "transfer/channelToA"}}, - {"trace info with multiple '/'s in base denom", "transfer/channelToA/gamm/pool/1", DenomTrace{BaseDenom: "gamm/pool/1", Path: "transfer/channelToA"}}, - {"trace info with multiple double '/'s in base denom", "transfer/channelToA/gamm//pool//1", DenomTrace{BaseDenom: "gamm//pool//1", Path: "transfer/channelToA"}}, - {"trace info with multiple port/channel pairs", "transfer/channelToA/transfer/channelToB/uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer/channelToA/transfer/channelToB"}}, + {"trace info", "transfer/channel-1/uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer/channel-1"}}, + {"trace info with custom port", "customtransfer/channel-1/uatom", DenomTrace{BaseDenom: "uatom", Path: "customtransfer/channel-1"}}, + {"trace info with base denom ending in '/'", "transfer/channel-1/uatom/", DenomTrace{BaseDenom: "uatom/", Path: "transfer/channel-1"}}, + {"trace info with single '/' in base denom", "transfer/channel-1/erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA", DenomTrace{BaseDenom: "erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA", Path: "transfer/channel-1"}}, + {"trace info with multiple '/'s in base denom", "transfer/channel-1/gamm/pool/1", DenomTrace{BaseDenom: "gamm/pool/1", Path: "transfer/channel-1"}}, + {"trace info with multiple double '/'s in base denom", "transfer/channel-1/gamm//pool//1", DenomTrace{BaseDenom: "gamm//pool//1", Path: "transfer/channel-1"}}, + {"trace info with multiple port/channel pairs", "transfer/channel-1/transfer/channel-2/uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer/channel-1/transfer/channel-2"}}, + {"trace info with multiple custom ports", "customtransfer/channel-1/alternativetransfer/channel-2/uatom", DenomTrace{BaseDenom: "uatom", Path: "customtransfer/channel-1/alternativetransfer/channel-2"}}, {"incomplete path", "transfer/uatom", DenomTrace{BaseDenom: "transfer/uatom"}}, - {"invalid path (1)", "transfer//uatom", DenomTrace{BaseDenom: "uatom", Path: "transfer/"}}, - {"invalid path (2)", "channelToA/transfer/uatom", DenomTrace{BaseDenom: "channelToA/transfer/uatom"}}, + {"invalid path (1)", "transfer//uatom", DenomTrace{BaseDenom: "transfer//uatom", Path: ""}}, + {"invalid path (2)", "channel-1/transfer/uatom", DenomTrace{BaseDenom: "channel-1/transfer/uatom"}}, {"invalid path (3)", "uatom/transfer", DenomTrace{BaseDenom: "uatom/transfer"}}, - {"invalid path (4)", "transfer/channelToA", DenomTrace{BaseDenom: "transfer/channelToA"}}, - {"invalid path (5)", "transfer/channelToA/", DenomTrace{Path: "transfer/channelToA"}}, - {"invalid path (6)", "transfer/channelToA/transfer", DenomTrace{BaseDenom: "transfer", Path: "transfer/channelToA"}}, - {"invalid path (7)", "transfer/channelToA/transfer/channelToB", DenomTrace{Path: "transfer/channelToA/transfer/channelToB"}}, + {"invalid path (4)", "transfer/channel-1", DenomTrace{BaseDenom: "transfer/channel-1"}}, + {"invalid path (5)", "transfer/channel-1/", DenomTrace{Path: "transfer/channel-1"}}, + {"invalid path (6)", "transfer/channel-1/transfer", DenomTrace{BaseDenom: "transfer", Path: "transfer/channel-1"}}, + {"invalid path (7)", "transfer/channel-1/transfer/channel-2", DenomTrace{Path: "transfer/channel-1/transfer/channel-2"}}, + {"invalid path (8)", "transfer/channelToA/uatom", DenomTrace{BaseDenom: "transfer/channelToA/uatom", Path: ""}}, } for _, tc := range testCases { @@ -46,7 +49,7 @@ func TestDenomTrace_IBCDenom(t *testing.T) { expDenom string }{ {"base denom", DenomTrace{BaseDenom: "uatom"}, "uatom"}, - {"trace info", DenomTrace{BaseDenom: "uatom", Path: "transfer/channelToA"}, "ibc/7F1D3FCF4AE79E1554D670D1AD949A9BA4E4A3C76C63093E17E446A46061A7A2"}, + {"trace info", DenomTrace{BaseDenom: "uatom", Path: "transfer/channel-1"}, "ibc/C4CFF46FD6DE35CA4CF4CE031E643C8FDC9BA4B99AE598E9B0ED98FE3A2319F9"}, } for _, tc := range testCases { @@ -65,12 +68,12 @@ func TestDenomTrace_Validate(t *testing.T) { {"base denom only with single '/'", DenomTrace{BaseDenom: "erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA"}, false}, {"base denom only with multiple '/'s", DenomTrace{BaseDenom: "gamm/pool/1"}, false}, {"empty DenomTrace", DenomTrace{}, true}, - {"valid single trace info", DenomTrace{BaseDenom: "uatom", Path: "transfer/channelToA"}, false}, - {"valid multiple trace info", DenomTrace{BaseDenom: "uatom", Path: "transfer/channelToA/transfer/channelToB"}, false}, + {"valid single trace info", DenomTrace{BaseDenom: "uatom", Path: "transfer/channel-1"}, false}, + {"valid multiple trace info", DenomTrace{BaseDenom: "uatom", Path: "transfer/channel-1/transfer/channel-2"}, false}, {"single trace identifier", DenomTrace{BaseDenom: "uatom", Path: "transfer"}, true}, - {"invalid port ID", DenomTrace{BaseDenom: "uatom", Path: "(transfer)/channelToA"}, true}, - {"invalid channel ID", DenomTrace{BaseDenom: "uatom", Path: "transfer/(channelToA)"}, true}, - {"empty base denom with trace", DenomTrace{BaseDenom: "", Path: "transfer/channelToA"}, true}, + {"invalid port ID", DenomTrace{BaseDenom: "uatom", Path: "(transfer)/channel-1"}, true}, + {"invalid channel ID", DenomTrace{BaseDenom: "uatom", Path: "transfer/(channel-1)"}, true}, + {"empty base denom with trace", DenomTrace{BaseDenom: "", Path: "transfer/channel-1"}, true}, } for _, tc := range testCases { @@ -90,16 +93,16 @@ func TestTraces_Validate(t *testing.T) { expError bool }{ {"empty Traces", Traces{}, false}, - {"valid multiple trace info", Traces{{BaseDenom: "uatom", Path: "transfer/channelToA/transfer/channelToB"}}, false}, + {"valid multiple trace info", Traces{{BaseDenom: "uatom", Path: "transfer/channel-1/transfer/channel-2"}}, false}, { "valid multiple trace info", Traces{ - {BaseDenom: "uatom", Path: "transfer/channelToA/transfer/channelToB"}, - {BaseDenom: "uatom", Path: "transfer/channelToA/transfer/channelToB"}, + {BaseDenom: "uatom", Path: "transfer/channel-1/transfer/channel-2"}, + {BaseDenom: "uatom", Path: "transfer/channel-1/transfer/channel-2"}, }, true, }, - {"empty base denom with trace", Traces{{BaseDenom: "", Path: "transfer/channelToA"}}, true}, + {"empty base denom with trace", Traces{{BaseDenom: "", Path: "transfer/channel-1"}}, true}, } for _, tc := range testCases { @@ -118,26 +121,25 @@ func TestValidatePrefixedDenom(t *testing.T) { denom string expError bool }{ - {"prefixed denom", "transfer/channelToA/uatom", false}, - {"prefixed denom with '/'", "transfer/channelToA/gamm/pool/1", false}, + {"prefixed denom", "transfer/channel-1/uatom", false}, + {"prefixed denom with '/'", "transfer/channel-1/gamm/pool/1", false}, {"empty prefix", "/uatom", false}, {"empty identifiers", "//uatom", false}, {"base denom", "uatom", false}, {"base denom with single '/'", "erc20/0x85bcBCd7e79Ec36f4fBBDc54F90C643d921151AA", false}, {"base denom with multiple '/'s", "gamm/pool/1", false}, - {"invalid port ID", "(transfer)/channelToA/uatom", false}, + {"invalid port ID", "(transfer)/channel-1/uatom", true}, {"empty denom", "", true}, {"single trace identifier", "transfer/", true}, - {"invalid channel ID", "transfer/(channelToA)/uatom", true}, } for _, tc := range testCases { err := ValidatePrefixedDenom(tc.denom) if tc.expError { require.Error(t, err, tc.name) - continue + } else { + require.NoError(t, err, tc.name) } - require.NoError(t, err, tc.name) } } From 049d3364ccbb4879d2aaf5cfca42f39ea7d973ea Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Thu, 21 Jul 2022 14:33:54 +0200 Subject: [PATCH 05/10] add changelog entry for #1680 --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e59ec65cc8b..83703b5140d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,6 +59,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements +* (app/20-transfer) [\#1680](https://github.com/cosmos/ibc-go/pull/1680) Adds migration to correct any malformed trace path information of tokens with denoms that contains slashes. The transfer module consensus version has been bumped to 2. * (app/20-transfer) [\#1730](https://github.com/cosmos/ibc-go/pull/1730) parse the ics20 denomination provided via a packet using the channel identifier format specified by ibc-go. * (cleanup) [\#1335](https://github.com/cosmos/ibc-go/pull/1335/) `gofumpt -w -l .` to standardize the code layout more strictly than `go fmt ./...` * (middleware) [\#1022](https://github.com/cosmos/ibc-go/pull/1022) Add `GetAppVersion` to the ICS4Wrapper interface. This function should be used by IBC applications to obtain their own version since the version set in the channel structure may be wrapped many times by middleware. From 0caf6255d139caaafbb701632f7175e0e042839f Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Thu, 21 Jul 2022 14:45:35 +0200 Subject: [PATCH 06/10] add issue templates for epics and releases (#1702) * add issue templates for epics and releases * remove text Co-authored-by: Carlos Rodriguez --- .github/ISSUE_TEMPLATE/epic-tracker.md | 64 ++++++++++++++++++++++ .github/ISSUE_TEMPLATE/release-tracker.md | 65 +++++++++++++++++++++++ 2 files changed, 129 insertions(+) create mode 100644 .github/ISSUE_TEMPLATE/epic-tracker.md create mode 100644 .github/ISSUE_TEMPLATE/release-tracker.md diff --git a/.github/ISSUE_TEMPLATE/epic-tracker.md b/.github/ISSUE_TEMPLATE/epic-tracker.md new file mode 100644 index 00000000000..df3f553ff49 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/epic-tracker.md @@ -0,0 +1,64 @@ +--- +name: Epic Tracker +about: Create an issue to track feature epic progress + +--- + + + +## Requirements document + + + +## IBC spec + + + +## ADRs + + + +## Milestones + + + +## Implementation issues + + + +## QA scenarios + + + +## Automated e2e tests + + + +## Pre-releases + + + +## Checklist + + + +- [ ] Internal audit(s) +- [ ] External audit(s) +- [ ] Documentation +- [ ] Swagger +- [ ] Integration with relayers: + - [ ] Hermes + - [ ] Rly + +____ + +#### For Admin Use + +- [ ] Not duplicate issue +- [ ] Appropriate labels applied +- [ ] Appropriate contributors tagged/assigned diff --git a/.github/ISSUE_TEMPLATE/release-tracker.md b/.github/ISSUE_TEMPLATE/release-tracker.md new file mode 100644 index 00000000000..1c91082024a --- /dev/null +++ b/.github/ISSUE_TEMPLATE/release-tracker.md @@ -0,0 +1,65 @@ +--- +name: Release tracker +about: Create an issue to track release progress + +--- + + + +## Milestones + + + +### IBC spec compatibility + + + +### QA + +#### Backwards compatibility + + + +- [ ] Fungible token transfers over a non-incentivised channel works with a counterparty chain running each previous major version. +- [ ] Fungible token transfers over an incentivised channel works with a counterparty chain running each previous major version that supports ICS-29 Fee Middleware. +- [ ] Interchain Accounts over a non-incentivised channel works with a counterparty chain running each previous major version that supports ICS-27 Interchain Accounts over non-incentivised channels. +- [ ] Interchain Accounts over an incentivised channel works with a counterparty chain running each previous major version that supports ICS-27 Interchain Accounts over incentivised channels. + +### Migration + + + +## Checklist + + + +- [ ] Bump [go package version](https://github.com/cosmos/ibc-go/blob/main/go.mod#L3). +- [ ] Change all imports starting with `github.com/cosmos/ibc-go/v{x}` to ``github.com/cosmos/ibc-go/v{x+1}`. +- [ ] Branch off main to create release branch in the form of `release/vx.y.z.`. +- [ ] Add branch protection rules to new release branch. +- [ ] Add backport task to [`mergify.yml`](https://github.com/cosmos/ibc-go/blob/main/.github/mergify.yml) +- [ ] Upgrade ibc-go version in [ibctest](https://github.com/strangelove-ventures/ibctest). +- [ ] Check Swagger is up-to-date. + +## Post-release checklist + +- [ ] Update [`CHANGELOG.md`](https://github.com/cosmos/ibc-go/blob/main/CHANGELOG.md) +- [ ] Update the table of supported release lines (and End of Life dates) in [`RELEASES.md`](https://github.com/cosmos/ibc-go/blob/main/RELEASES.md). +- [ ] Update docs site: + - [ ] Add new release branch to [`docs/versions`](https://github.com/cosmos/ibc-go/blob/main/docs/versions) file. + - [ ] Add `label` and `key` to `versions` array in [`config.js`](https://github.com/cosmos/ibc-go/blob/main/docs/.vuepress/config.js#L33). +- [ ] After changes to docs site are deployed, check [ibc.cosmos.network](https://ibc.cosmos.network) is updated. + +____ + +#### For Admin Use + +- [ ] Not duplicate issue +- [ ] Appropriate labels applied +- [ ] Appropriate contributors tagged/assigned From c1a727eb434035fe30c19f6f96c0fc60f69122d4 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Thu, 21 Jul 2022 14:49:59 +0200 Subject: [PATCH 07/10] fix typo --- .github/ISSUE_TEMPLATE/release-tracker.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/ISSUE_TEMPLATE/release-tracker.md b/.github/ISSUE_TEMPLATE/release-tracker.md index 1c91082024a..236142c4e5c 100644 --- a/.github/ISSUE_TEMPLATE/release-tracker.md +++ b/.github/ISSUE_TEMPLATE/release-tracker.md @@ -41,7 +41,7 @@ versions of ibc-go to guarantee that no regression is introduced --> - [ ] Bump [go package version](https://github.com/cosmos/ibc-go/blob/main/go.mod#L3). - [ ] Change all imports starting with `github.com/cosmos/ibc-go/v{x}` to ``github.com/cosmos/ibc-go/v{x+1}`. -- [ ] Branch off main to create release branch in the form of `release/vx.y.z.`. +- [ ] Branch off main to create release branch in the form of `release/vx.y.z`. - [ ] Add branch protection rules to new release branch. - [ ] Add backport task to [`mergify.yml`](https://github.com/cosmos/ibc-go/blob/main/.github/mergify.yml) - [ ] Upgrade ibc-go version in [ibctest](https://github.com/strangelove-ventures/ibctest). From 3af515df4f7fceeb6946244f7732f60203f456e9 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Thu, 21 Jul 2022 14:55:42 +0200 Subject: [PATCH 08/10] fix typo --- .github/ISSUE_TEMPLATE/release-tracker.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/ISSUE_TEMPLATE/release-tracker.md b/.github/ISSUE_TEMPLATE/release-tracker.md index 236142c4e5c..707a046b7ca 100644 --- a/.github/ISSUE_TEMPLATE/release-tracker.md +++ b/.github/ISSUE_TEMPLATE/release-tracker.md @@ -40,7 +40,7 @@ versions of ibc-go to guarantee that no regression is introduced --> - [ ] Bump [go package version](https://github.com/cosmos/ibc-go/blob/main/go.mod#L3). -- [ ] Change all imports starting with `github.com/cosmos/ibc-go/v{x}` to ``github.com/cosmos/ibc-go/v{x+1}`. +- [ ] Change all imports starting with `github.com/cosmos/ibc-go/v{x}` to `github.com/cosmos/ibc-go/v{x+1}`. - [ ] Branch off main to create release branch in the form of `release/vx.y.z`. - [ ] Add branch protection rules to new release branch. - [ ] Add backport task to [`mergify.yml`](https://github.com/cosmos/ibc-go/blob/main/.github/mergify.yml) From c12789d882a3a8ea4ca078ca9783a677e0a56ea4 Mon Sep 17 00:00:00 2001 From: Charly Date: Thu, 21 Jul 2022 15:37:12 +0200 Subject: [PATCH 09/10] feat: allow governance to update the TrustingPeriod of the 07-tendermint light client (#1713) * initial commit * format imports * update docs * update CHANGELOG * update upgrade dev docs * update re: pr comments --- CHANGELOG.md | 1 + .../adr-026-ibc-client-recovery-mechanisms.md | 4 ++++ docs/ibc/upgrades/developer-guide.md | 2 +- .../07-tendermint/types/proposal_handle.go | 8 +++++++- .../07-tendermint/types/proposal_handle_test.go | 11 ++++++++++- 5 files changed, 23 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 83703b5140d..03fd745da6d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -101,6 +101,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (transfer) [\#1414](https://github.com/cosmos/ibc-go/pull/1414) Emitting Sender address from `fungible_token_packet` events in `OnRecvPacket` and `OnAcknowledgementPacket`. * (modules/core/04-channel) [\#1464](https://github.com/cosmos/ibc-go/pull/1464) Emit a channel close event when an ordered channel is closed. * (modules/light-clients/07-tendermint) [\#1118](https://github.com/cosmos/ibc-go/pull/1118) Deprecating `AllowUpdateAfterExpiry` and `AllowUpdateAfterMisbehaviour`. See ADR-026 for context. +* (modules/light-clients/07-tendermint) [\#1713](https://github.com/cosmos/ibc-go/pull/1713) Allow client upgrade proposals to update `TrustingPeriod`. See ADR-026 for context. ### Features diff --git a/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md b/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md index 516002ed1b9..bec25a3aad9 100644 --- a/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md +++ b/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md @@ -7,6 +7,7 @@ - 2021/01/15: Revision to support substitute clients for unfreezing - 2021/05/20: Revision to simplify consensus state copying, remove initial height - 2022/04/08: Revision to deprecate AllowUpdateAfterExpiry and AllowUpdateAfterMisbehaviour +- 2022/07/15: Revision to allow updating of TrustingPeriod ## Status @@ -51,6 +52,9 @@ We elect not to deal with chains which have actually halted, which is necessaril Previously, `AllowUpdateAfterExpiry` and `AllowUpdateAfterMisbehaviour` were used to signal the recovery options for an expired or frozen client, and governance proposals were not allowed to overwrite the client if these parameters were set to false. However, this has now been deprecated because a code migration can overwrite the client and consensus states regardless of the value of these parameters. If governance would vote to overwrite a client or consensus state, it is likely that governance would also be willing to perform a code migration to do the same. + In addition, `TrustingPeriod` was initally not allowed to be updated by a client upgrade proposal. However, due to the number of situations experienced in production where the `TrustingPeriod` of a client should be allowed to be updated because of ie: initial misconfiguration for a canonical channel, governance should be allowed to update this client parameter. + + Note that this should NOT be lightly updated, as there may be a gap in time between when misbehaviour has occured and when the evidence of misbehaviour is submitted. For example, if the `UnbondingPeriod` is 2 weeks and the `TrustingPeriod` has also been set to two weeks, a validator could wait until right before `UnbondingPeriod` finishes, submit false information, then unbond and exit without being slashed for misbehaviour. Therefore, we recommend that the trusting period for the 07-tendermint client be set to 2/3 of the `UnbondingPeriod`. Note that clients frozen due to misbehaviour must wait for the evidence to expire to avoid becoming refrozen. diff --git a/docs/ibc/upgrades/developer-guide.md b/docs/ibc/upgrades/developer-guide.md index d41b3346d4f..73a19b93664 100644 --- a/docs/ibc/upgrades/developer-guide.md +++ b/docs/ibc/upgrades/developer-guide.md @@ -36,7 +36,7 @@ Developers must ensure that the new client adopts all of the new Client paramete Upgrades must adhere to the IBC Security Model. IBC does not rely on the assumption of honest relayers for correctness. Thus users should not have to rely on relayers to maintain client correctness and security (though honest relayers must exist to maintain relayer liveness). While relayers may choose any set of client parameters while creating a new `ClientState`, this still holds under the security model since users can always choose a relayer-created client that suits their security and correctness needs or create a Client with their desired parameters if no such client exists. -However, when upgrading an existing client, one must keep in mind that there are already many users who depend on this client's particular parameters. We cannot give the upgrading relayer free choice over these parameters once they have already been chosen. This would violate the security model since users who rely on the client would have to rely on the upgrading relayer to maintain the same level of security. Thus, developers must make sure that their upgrade mechanism allows clients to upgrade the chain-specified parameters whenever a chain upgrade changes these parameters (examples in the Tendermint client include `UnbondingPeriod`, `ChainID`, `UpgradePath`, etc.), while ensuring that the relayer submitting the `UpgradeClientMsg` cannot alter the client-chosen parameters that the users are relying upon (examples in Tendermint client include `TrustingPeriod`, `TrustLevel`, `MaxClockDrift`, etc). +However, when upgrading an existing client, one must keep in mind that there are already many users who depend on this client's particular parameters. We cannot give the upgrading relayer free choice over these parameters once they have already been chosen. This would violate the security model since users who rely on the client would have to rely on the upgrading relayer to maintain the same level of security. Thus, developers must make sure that their upgrade mechanism allows clients to upgrade the chain-specified parameters whenever a chain upgrade changes these parameters (examples in the Tendermint client include `UnbondingPeriod`, `TrustingPeriod`, `ChainID`, `UpgradePath`, etc.), while ensuring that the relayer submitting the `UpgradeClientMsg` cannot alter the client-chosen parameters that the users are relying upon (examples in Tendermint client include `TrustLevel`, `MaxClockDrift`, etc). Developers should maintain the distinction between Client parameters that are uniform across every valid light client of a chain (chain-chosen parameters), and Client parameters that are customizable by each individual client (client-chosen parameters); since this distinction is necessary to implement the `ZeroCustomFields` method in the `ClientState` interface: diff --git a/modules/light-clients/07-tendermint/types/proposal_handle.go b/modules/light-clients/07-tendermint/types/proposal_handle.go index b1ba592453e..85720f7eaeb 100644 --- a/modules/light-clients/07-tendermint/types/proposal_handle.go +++ b/modules/light-clients/07-tendermint/types/proposal_handle.go @@ -2,6 +2,7 @@ package types import ( "reflect" + "time" "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" @@ -70,6 +71,9 @@ func (cs ClientState) CheckSubstituteAndUpdateState( cs.LatestHeight = substituteClientState.LatestHeight cs.ChainId = substituteClientState.ChainId + // set new trusting period based on the substitute client state + cs.TrustingPeriod = substituteClientState.TrustingPeriod + // no validation is necessary since the substitute is verified to be Active // in 02-client. @@ -77,13 +81,15 @@ func (cs ClientState) CheckSubstituteAndUpdateState( } // IsMatchingClientState returns true if all the client state parameters match -// except for frozen height, latest height, and chain-id. +// except for frozen height, latest height, trusting period, chain-id. func IsMatchingClientState(subject, substitute ClientState) bool { // zero out parameters which do not need to match subject.LatestHeight = clienttypes.ZeroHeight() subject.FrozenHeight = clienttypes.ZeroHeight() + subject.TrustingPeriod = time.Duration(0) substitute.LatestHeight = clienttypes.ZeroHeight() substitute.FrozenHeight = clienttypes.ZeroHeight() + substitute.TrustingPeriod = time.Duration(0) subject.ChainId = "" substitute.ChainId = "" // sets both sets of flags to true as these flags have been DEPRECATED, see ADR-026 for more information diff --git a/modules/light-clients/07-tendermint/types/proposal_handle_test.go b/modules/light-clients/07-tendermint/types/proposal_handle_test.go index 459f38187ea..bdd2d98ead5 100644 --- a/modules/light-clients/07-tendermint/types/proposal_handle_test.go +++ b/modules/light-clients/07-tendermint/types/proposal_handle_test.go @@ -141,6 +141,8 @@ func (suite *TendermintTestSuite) TestCheckSubstituteAndUpdateState() { substitutePath := ibctesting.NewPath(suite.chainA, suite.chainB) suite.coordinator.SetupClients(substitutePath) substituteClientState := suite.chainA.GetClientState(substitutePath.EndpointA.ClientID).(*types.ClientState) + // update trusting period of substitute client state + substituteClientState.TrustingPeriod = time.Hour * 24 * 7 suite.chainA.App.GetIBCKeeper().ClientKeeper.SetClientState(suite.chainA.GetContext(), substitutePath.EndpointA.ClientID, substituteClientState) // update substitute a few times @@ -191,6 +193,7 @@ func (suite *TendermintTestSuite) TestCheckSubstituteAndUpdateState() { suite.Require().Equal(expectedIterationKey, subjectIterationKey) suite.Require().Equal(newChainID, updatedClient.(*types.ClientState).ChainId) + suite.Require().Equal(time.Hour*24*7, updatedClient.(*types.ClientState).TrustingPeriod) } else { suite.Require().Error(err) suite.Require().Nil(updatedClient) @@ -235,9 +238,15 @@ func (suite *TendermintTestSuite) TestIsMatchingClientState() { }, true, }, { - "not matching, trusting period is different", func() { + "matching, trusting period is different", func() { subjectClientState.TrustingPeriod = time.Duration(time.Hour * 10) substituteClientState.TrustingPeriod = time.Duration(time.Hour * 1) + }, true, + }, + { + "not matching, trust level is different", func() { + subjectClientState.TrustLevel = types.Fraction{2, 3} + substituteClientState.TrustLevel = types.Fraction{1, 3} }, false, }, } From 7dc9fedc9e16b054b5adef42fb746871216d1849 Mon Sep 17 00:00:00 2001 From: Cian Hatton Date: Thu, 21 Jul 2022 16:28:13 +0100 Subject: [PATCH 10/10] E2E Test: TestMsgPayPacketFeeSingleSender (#1756) --- e2e/fee_middleware_test.go | 121 +++++++++++++++++++++++++++++++++-- e2e/testconfig/testconfig.go | 2 +- e2e/testvalues/values.go | 4 ++ 3 files changed, 121 insertions(+), 6 deletions(-) diff --git a/e2e/fee_middleware_test.go b/e2e/fee_middleware_test.go index f8c05822999..4cc4ea14332 100644 --- a/e2e/fee_middleware_test.go +++ b/e2e/fee_middleware_test.go @@ -15,6 +15,8 @@ import ( "e2e/testvalues" feetypes "github.com/cosmos/ibc-go/v4/modules/apps/29-fee/types" + transfertypes "github.com/cosmos/ibc-go/v4/modules/apps/transfer/types" + clienttypes "github.com/cosmos/ibc-go/v4/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v4/modules/core/04-channel/types" ) @@ -135,7 +137,7 @@ func (s *FeeMiddlewareTestSuite) TestMsgPayPacketFeeAsyncSingleSender() { actualBalance, err := s.GetChainANativeBalance(ctx, chainAWallet) s.Require().NoError(err) - expected := testvalues.StartingTokenAmount - walletAmount.Amount - chainA.GetGasFeesInNativeDenom(chainATx.GasSpent) + expected := testvalues.StartingTokenAmount - walletAmount.Amount s.Require().Equal(expected, actualBalance) }) @@ -171,8 +173,7 @@ func (s *FeeMiddlewareTestSuite) TestMsgPayPacketFeeAsyncSingleSender() { actualBalance, err := s.GetChainANativeBalance(ctx, chainAWallet) s.Require().NoError(err) - gasFees := chainA.GetGasFeesInNativeDenom(chainATx.GasSpent) + chainA.GetGasFeesInNativeDenom(payPacketFeeTxResp.GasWanted) - expected := testvalues.StartingTokenAmount - walletAmount.Amount - gasFees - testFee.Total().AmountOf(chainADenom).Int64() + expected := testvalues.StartingTokenAmount - walletAmount.Amount - testFee.Total().AmountOf(chainADenom).Int64() s.Require().Equal(expected, actualBalance) }) }) @@ -191,9 +192,119 @@ func (s *FeeMiddlewareTestSuite) TestMsgPayPacketFeeAsyncSingleSender() { actualBalance, err := s.GetChainANativeBalance(ctx, chainAWallet) s.Require().NoError(err) - gasFees := chainA.GetGasFeesInNativeDenom(chainATx.GasSpent) + chainA.GetGasFeesInNativeDenom(payPacketFeeTxResp.GasWanted) // once the relayer has relayed the packets, the timeout fee should be refunded. - expected := testvalues.StartingTokenAmount - walletAmount.Amount - gasFees - testFee.AckFee.AmountOf(chainADenom).Int64() - testFee.RecvFee.AmountOf(chainADenom).Int64() + expected := testvalues.StartingTokenAmount - walletAmount.Amount - testFee.AckFee.AmountOf(chainADenom).Int64() - testFee.RecvFee.AmountOf(chainADenom).Int64() + s.Require().Equal(expected, actualBalance) + }) +} + +func (s *FeeMiddlewareTestSuite) TestMultiMsg_MsgPayPacketFeeSingleSender() { + t := s.T() + ctx := context.TODO() + + relayer, channelA := s.SetupChainsRelayerAndChannel(ctx, feeMiddlewareChannelOptions()) + + chainA, chainB := s.GetChains() + + var ( + chainADenom = chainA.Config().Denom + testFee = testvalues.DefaultFee(chainADenom) + multiMsgTxResponse sdk.TxResponse + ) + + transferAmount := testvalues.DefaultTransferAmount(chainA.Config().Denom) + + chainAWallet := s.CreateUserOnChainA(ctx, testvalues.StartingTokenAmount) + chainBWallet := s.CreateUserOnChainB(ctx, testvalues.StartingTokenAmount) + + t.Run("relayer wallets recovered", func(t *testing.T) { + err := s.RecoverRelayerWallets(ctx, relayer) + s.Require().NoError(err) + }) + + chainARelayerWallet, chainBRelayerWallet, err := s.GetRelayerWallets(relayer) + t.Run("relayer wallets fetched", func(t *testing.T) { + s.Require().NoError(err) + }) + + s.Require().NoError(test.WaitForBlocks(ctx, 1, chainA, chainB), "failed to wait for blocks") + + chainARelayerUser, chainBRelayerUser := s.GetRelayerUsers(ctx) + + relayerAStartingBalance, err := s.GetChainANativeBalance(ctx, chainARelayerUser) + s.Require().NoError(err) + t.Logf("relayer A user starting with balance: %d", relayerAStartingBalance) + + t.Run("register counter party payee", func(t *testing.T) { + multiMsgTxResponse, err = s.RegisterCounterPartyPayee(ctx, chainB, chainBRelayerUser, channelA.Counterparty.PortID, channelA.Counterparty.ChannelID, chainBRelayerWallet.Address, chainARelayerWallet.Address) + s.Require().NoError(err) + s.AssertValidTxResponse(multiMsgTxResponse) + }) + + t.Run("verify counter party payee", func(t *testing.T) { + address, err := s.QueryCounterPartyPayee(ctx, chainB, chainBRelayerWallet.Address, channelA.Counterparty.ChannelID) + s.Require().NoError(err) + s.Require().Equal(chainARelayerWallet.Address, address) + }) + + t.Run("no incentivized packets", func(t *testing.T) { + packets, err := s.QueryIncentivizedPacketsForChannel(ctx, chainA, channelA.PortID, channelA.ChannelID) + s.Require().NoError(err) + s.Require().Empty(packets) + }) + + payPacketFeeMsg := feetypes.NewMsgPayPacketFee(testFee, channelA.PortID, channelA.ChannelID, chainAWallet.Bech32Address(chainA.Config().Bech32Prefix), nil) + transferMsg := transfertypes.NewMsgTransfer(channelA.PortID, channelA.ChannelID, transferAmount, chainAWallet.Bech32Address(chainA.Config().Bech32Prefix), chainBWallet.Bech32Address(chainB.Config().Bech32Prefix), clienttypes.NewHeight(1, 1000), 0) + resp, err := s.BroadcastMessages(ctx, chainA, chainAWallet, payPacketFeeMsg, transferMsg) + + t.Run("transfer successful", func(t *testing.T) { + s.AssertValidTxResponse(resp) + s.Require().NoError(err) + }) + + t.Run("there should be incentivized packets", func(t *testing.T) { + packets, err := s.QueryIncentivizedPacketsForChannel(ctx, chainA, channelA.PortID, channelA.ChannelID) + s.Require().NoError(err) + s.Require().Len(packets, 1) + actualFee := packets[0].PacketFees[0].Fee + + s.Require().True(actualFee.RecvFee.IsEqual(testFee.RecvFee)) + s.Require().True(actualFee.AckFee.IsEqual(testFee.AckFee)) + s.Require().True(actualFee.TimeoutFee.IsEqual(testFee.TimeoutFee)) + }) + + t.Run("balance should be lowered by sum of recv ack and timeout", func(t *testing.T) { + // The balance should be lowered by the sum of the recv, ack and timeout fees. + actualBalance, err := s.GetChainANativeBalance(ctx, chainAWallet) + s.Require().NoError(err) + + expected := testvalues.StartingTokenAmount - testvalues.IBCTransferAmount - testFee.Total().AmountOf(chainADenom).Int64() + s.Require().Equal(expected, actualBalance) + }) + + t.Run("start relayer", func(t *testing.T) { + s.StartRelayer(relayer) + }) + + t.Run("packets are relayed", func(t *testing.T) { + packets, err := s.QueryIncentivizedPacketsForChannel(ctx, chainA, channelA.PortID, channelA.ChannelID) + s.Require().NoError(err) + s.Require().Empty(packets) + }) + + t.Run("timeout fee is refunded", func(t *testing.T) { + actualBalance, err := s.GetChainANativeBalance(ctx, chainAWallet) + s.Require().NoError(err) + + // once the relayer has relayed the packets, the timeout fee should be refunded. + expected := testvalues.StartingTokenAmount - testvalues.IBCTransferAmount - testFee.AckFee.AmountOf(chainADenom).Int64() - testFee.RecvFee.AmountOf(chainADenom).Int64() + s.Require().Equal(expected, actualBalance) + }) + + t.Run("relayerA is paid ack and recv fee", func(t *testing.T) { + actualBalance, err := s.GetChainANativeBalance(ctx, chainARelayerUser) + s.Require().NoError(err) + expected := relayerAStartingBalance + testFee.AckFee.AmountOf(chainADenom).Int64() + testFee.RecvFee.AmountOf(chainADenom).Int64() s.Require().Equal(expected, actualBalance) }) } diff --git a/e2e/testconfig/testconfig.go b/e2e/testconfig/testconfig.go index 50d49edce53..46be84e1e58 100644 --- a/e2e/testconfig/testconfig.go +++ b/e2e/testconfig/testconfig.go @@ -85,7 +85,7 @@ func newDefaultSimappConfig(tc TestConfig, name, chainID, denom string) ibc.Chai Bin: "simd", Bech32Prefix: "cosmos", Denom: denom, - GasPrices: fmt.Sprintf("0.01%s", denom), + GasPrices: fmt.Sprintf("0.00%s", denom), GasAdjustment: 1.3, TrustingPeriod: "508h", NoHostMount: false, diff --git a/e2e/testvalues/values.go b/e2e/testvalues/values.go index b91cfae1b84..7b0822c9f1c 100644 --- a/e2e/testvalues/values.go +++ b/e2e/testvalues/values.go @@ -18,3 +18,7 @@ func DefaultFee(denom string) feetypes.Fee { TimeoutFee: sdk.NewCoins(sdk.NewCoin(denom, sdk.NewInt(10))), } } + +func DefaultTransferAmount(denom string) sdk.Coin { + return sdk.Coin{Denom: denom, Amount: sdk.NewInt(IBCTransferAmount)} +}