Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests(e2e): ics20 v2 multidenom #6290

Merged
merged 56 commits into from
Jun 17, 2024
Merged
Show file tree
Hide file tree
Changes from 48 commits
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
c07bca9
Adding proto files for ics20-v2 (#6110)
chatton Apr 8, 2024
e66bd89
update amount -> string (#6120)
charleenfei Apr 9, 2024
034f472
Update MsgTransfer to accept sdk.Coins instead of sdk.Coin (#6113)
chatton Apr 9, 2024
4cc6a85
fix: allow base denom with trailing slash (#6148)
crodriguezvega Apr 15, 2024
71f830c
imp: add CurrentVersion, EscrowVersion (#6160)
charleenfei Apr 16, 2024
28ff9b6
chore: add function for converting packet data from v1 to v3 (#6116)
chatton Apr 16, 2024
4e55137
chore: implement required `FungibleTokenPacketData` v3 interface meth…
charleenfei Apr 22, 2024
ca056cf
imp: `getMultiDenomFungibleTokenPacketData`to be used in packet unmar…
charleenfei Apr 29, 2024
147cf17
chore: implement version checking for channel handshake application c…
charleenfei May 6, 2024
9bbfa1a
imp: update transfer authz implementation to account for multi denom …
charleenfei May 6, 2024
4f57916
ics20-v2: backwards compatibility for transfer rpc and packet callbac…
crodriguezvega May 10, 2024
b4244f9
multidenom transfer test + ics20-v2 channel upgrade test
crodriguezvega May 10, 2024
b09f5d7
some fixes
crodriguezvega May 12, 2024
4c333c5
changes to transfer tx CLI to support multiple denoms
crodriguezvega May 14, 2024
8b55295
lint
crodriguezvega May 14, 2024
0478cb9
import renaming
crodriguezvega May 14, 2024
f39d173
Use type with V2 suffix for package data (#6330)
chatton May 21, 2024
9b39944
Adding additional comments and changing version variable names (#6345)
chatton May 21, 2024
06ca9a5
Merge branch 'main' into merge-main
chatton May 21, 2024
7897ef3
chore: correctly claim capability
chatton May 21, 2024
786a4f1
lint
colin-axner May 21, 2024
3a162ea
Merge branch 'feat/ics20-v2' into carlos/e2e-tests-multidenom
crodriguezvega May 21, 2024
5747756
Merge pull request #6346 from cosmos/merge-main
DimitrisJim May 21, 2024
7e2e6df
Merge branch 'main' into feat/ics20-v2
chatton May 22, 2024
a84b0e7
imp: change ics20 events to emit token set (#6348)
colin-axner May 22, 2024
43877df
imp: check length tokens array against maximum allowed (#6349)
crodriguezvega May 22, 2024
8eae033
Modify UnmarshalPacketData interface to allow additional args (#6341)
DimitrisJim May 22, 2024
dbcff45
Refactor packet data unmarshalling to use specific version (#6354)
chatton May 23, 2024
bb69698
Merge branch 'main' into merge-main-2
chatton May 23, 2024
f19a145
chore: fixing tests
chatton May 23, 2024
8f86dda
Merge pull request #6359 from cosmos/merge-main-2
chatton May 23, 2024
d4b06c8
imp: self review comments for ics20-v2 (#6360)
colin-axner May 23, 2024
a9391a4
imp: self review on ics20-v2 part 2 (#6364)
colin-axner May 23, 2024
575403e
chore: move functions from internal/denom back to trace.go (#6368)
DimitrisJim May 23, 2024
50ccd94
imp: ics20 v2 self review part 3 (#6373)
colin-axner May 23, 2024
87eb32e
chore: remove duplicate test case
colin-axner May 23, 2024
e8b9d5a
chore: address minor nits (#6374)
DimitrisJim May 23, 2024
6d40bc6
Refactor msgs_test.go to use expError (#6367)
chatton May 24, 2024
c171059
chore: remove unused chain variable in setup (#6371)
chatton May 24, 2024
a276219
Merge branch 'feat/ics20-v2' into carlos/e2e-tests-multidenom
crodriguezvega May 27, 2024
d55c817
use new queries in e2e
crodriguezvega May 27, 2024
3319e42
add test for error ack multidenom transfer
crodriguezvega May 27, 2024
0d0b5fc
Merge branch 'main' into carlos/e2e-tests-multidenom
crodriguezvega May 28, 2024
a284934
Merge branch 'main' into carlos/e2e-tests-multidenom
crodriguezvega May 29, 2024
2dafbd5
Merge branch 'main' into carlos/e2e-tests-multidenom
crodriguezvega May 30, 2024
2485db0
Merge branch 'main' into carlos/e2e-tests-multidenom
crodriguezvega Jun 7, 2024
6886e3b
downgrade test to ics20-1
crodriguezvega Jun 8, 2024
14d17ff
add test to upgrade channel to fee middleware and ICS20 v2
crodriguezvega Jun 9, 2024
3f12a8e
revert some unnecessary changes
crodriguezvega Jun 10, 2024
087d52f
add transfer failure multidenom test to compatibility
crodriguezvega Jun 10, 2024
feb2383
Merge branch 'main' into carlos/e2e-tests-multidenom
crodriguezvega Jun 10, 2024
bdd4ab6
updates to multidenom invalid adress test
crodriguezvega Jun 17, 2024
70f9b8c
Merge branch 'main' into carlos/e2e-tests-multidenom
crodriguezvega Jun 17, 2024
7d42ff3
fix value comparison
crodriguezvega Jun 17, 2024
d4bd933
review comments
crodriguezvega Jun 17, 2024
fbb6994
Merge branch 'main' into carlos/e2e-tests-multidenom
crodriguezvega Jun 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"chain-a": [
"main"
],
"chain-b": [
"main"
],
"entrypoint": [
"TestTransferTestSuite"
],
"test": [
"TestMsgTransfer_Succeeds_Nonincentivized_MultiDenom"
],
"relayer-type": [
"hermes"
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"chain-a": [
"main"
],
"chain-b": [
"main"
],
"entrypoint": [
"TestTransferChannelUpgradesTestSuite"
],
"test": [
"TestChannelUpgrade_WithICS20v2_Succeeds",
"TestChannelUpgrade_WithFeeMiddlewareAndICS20v2_Succeeds",
"TestChannelDowngrade_WithICS20v1_Succeeds"
],
"relayer-type": [
"hermes"
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"chain-a": [
"release-v9.0.x"
],
"chain-b": [
"release-v9.0.x"
],
"entrypoint": [
"TestTransferChannelUpgradesTestSuite"
],
"test": [
"TestChannelUpgrade_WithICS20v2_Succeeds",
"TestChannelUpgrade_WithFeeMiddlewareAndICS20v2_Succeeds",
"TestChannelDowngrade_WithICS20v1_Succeeds"
],
"relayer-type": [
"hermes"
]
}
17 changes: 17 additions & 0 deletions .github/compatibility-test-matrices/unreleased/transfer-v2.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"chain-a": [
"release-v9.0.x"
],
"chain-b": [
"release-v9.0.x"
],
"entrypoint": [
"TestTransferTestSuite"
],
"test": [
"TestMsgTransfer_Succeeds_Nonincentivized_MultiDenom"
],
"relayer-type": [
"hermes"
]
}
211 changes: 210 additions & 1 deletion e2e/tests/transfer/base_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,215 @@ func (s *TransferTestSuite) TestMsgTransfer_Succeeds_Nonincentivized() {
}
}

// TestMsgTransfer_Succeeds_MultiDenom will test sending successful IBC transfers from chainA to chainB.
// A multidenom transfer with native chainB tokens and IBC tokens from chainA is executed from chainB to chainA.
func (s *TransferTestSuite) TestMsgTransfer_Succeeds_Nonincentivized_MultiDenom() {
t := s.T()
ctx := context.TODO()

relayer, channelA := s.SetupChainsRelayerAndChannel(ctx, nil)
chainA, chainB := s.GetChains()

chainADenom := chainA.Config().Denom
chainBDenom := chainB.Config().Denom

chainAWallet := s.CreateUserOnChainA(ctx, testvalues.StartingTokenAmount)
chainAAddress := chainAWallet.FormattedAddress()

chainBWallet := s.CreateUserOnChainB(ctx, testvalues.StartingTokenAmount)
chainBAddress := chainBWallet.FormattedAddress()

chainBIBCToken := testsuite.GetIBCToken(chainADenom, channelA.Counterparty.PortID, channelA.Counterparty.ChannelID)
chainAIBCToken := testsuite.GetIBCToken(chainBDenom, channelA.PortID, channelA.ChannelID)

t.Run("native IBC token transfer from chainA to chainB, sender is source of tokens", func(t *testing.T) {
transferTxResp := s.Transfer(ctx, chainA, chainAWallet, channelA.PortID, channelA.ChannelID, sdk.NewCoins(testvalues.DefaultTransferAmount(chainADenom)), chainAAddress, chainBAddress, s.GetTimeoutHeight(ctx, chainB), 0, "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] there is a testvaluesDefaultTransferCoins fn that does this sdk.NewCoins call to reduce boilerplate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to do this, but I prefer to do it in a separate PR, since there is already a bunch of places that could benefit from that.

s.AssertTxSuccess(transferTxResp)
})

s.Require().NoError(test.WaitForBlocks(ctx, 5, chainA, chainB), "failed to wait for blocks")

t.Run("native chainA tokens are escrowed", func(t *testing.T) {
actualBalance, err := s.GetChainANativeBalance(ctx, chainAWallet)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

food for thought: I feel like there's a lot of repeated code in our e2e's. Would cut down review time on new pr's if we deduplicated this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On my list of ideas for a focus week 👀

s.Require().NoError(err)

expected := testvalues.StartingTokenAmount - testvalues.IBCTransferAmount
s.Require().Equal(expected, actualBalance)

actualTotalEscrow, err := query.TotalEscrowForDenom(ctx, chainA, chainADenom)
s.Require().NoError(err)

expectedTotalEscrow := sdk.NewCoin(chainADenom, sdkmath.NewInt(testvalues.IBCTransferAmount))
s.Require().Equal(expectedTotalEscrow, actualTotalEscrow)
})

t.Run("start relayer", func(t *testing.T) {
s.StartRelayer(relayer)
})

t.Run("packets are relayed", func(t *testing.T) {
s.AssertPacketRelayed(ctx, chainA, channelA.PortID, channelA.ChannelID, 1)

actualBalance, err := query.Balance(ctx, chainB, chainBAddress, chainBIBCToken.IBCDenom())
s.Require().NoError(err)

expected := testvalues.IBCTransferAmount
s.Require().Equal(expected, actualBalance.Int64())
})

t.Run("metadata for IBC denomination exists on chainB", func(t *testing.T) {
s.AssertHumanReadableDenom(ctx, chainB, chainADenom, channelA)
})

// send the native chainB denom and also the ibc token from chainA
denoms := []string{chainBIBCToken.IBCDenom(), chainBDenom}
var transferCoins []sdk.Coin
for _, denom := range denoms {
transferCoins = append(transferCoins, testvalues.DefaultTransferAmount(denom))
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] with just two items it might be more readable to just do 2 appends instead of a for loop


t.Run("native token from chain B and non-native IBC token from chainA, both to chainA", func(t *testing.T) {
transferTxResp := s.Transfer(ctx, chainB, chainBWallet, channelA.Counterparty.PortID, channelA.Counterparty.ChannelID, transferCoins, chainBAddress, chainAAddress, s.GetTimeoutHeight(ctx, chainA), 0, "")
s.AssertTxSuccess(transferTxResp)
})

s.Require().NoError(test.WaitForBlocks(ctx, 5, chainA, chainB), "failed to wait for blocks")

t.Run("packets are relayed", func(t *testing.T) {
s.AssertPacketRelayed(ctx, chainB, channelA.Counterparty.PortID, channelA.Counterparty.ChannelID, 1)

t.Run("chain A native denom", func(t *testing.T) {
actualBalance, err := s.GetChainANativeBalance(ctx, chainAWallet)
s.Require().NoError(err)

expected := testvalues.StartingTokenAmount
s.Require().Equal(expected, actualBalance)
})

t.Run("chain B IBC denom", func(t *testing.T) {
actualBalance, err := query.Balance(ctx, chainA, chainAAddress, chainAIBCToken.IBCDenom())
s.Require().NoError(err)

expected := testvalues.IBCTransferAmount
s.Require().Equal(expected, actualBalance.Int64())
})
})

t.Run("native chainA tokens are un-escrowed", func(t *testing.T) {
actualTotalEscrow, err := query.TotalEscrowForDenom(ctx, chainA, chainADenom)
s.Require().NoError(err)
s.Require().Equal(sdk.NewCoin(chainADenom, sdkmath.NewInt(0)), actualTotalEscrow) // total escrow is zero because tokens have come back
})
}

// TestMsgTransfer_Fails_InvalidAddress_MultiDenom attempts to send a multidenom IBC transfer
// to an invalid address and ensures that the tokens on the sending chain are returned to the sender.
func (s *TransferTestSuite) TestMsgTransfer_Fails_InvalidAddress_MultiDenom() {
t := s.T()
ctx := context.TODO()

relayer, channelA := s.SetupChainsRelayerAndChannel(ctx, nil)
chainA, chainB := s.GetChains()

chainADenom := chainA.Config().Denom
chainBDenom := chainB.Config().Denom

chainAWallet := s.CreateUserOnChainA(ctx, testvalues.StartingTokenAmount)
chainAAddress := chainAWallet.FormattedAddress()

chainBWallet := s.CreateUserOnChainB(ctx, testvalues.StartingTokenAmount)
chainBAddress := chainBWallet.FormattedAddress()

chainBIBCToken := testsuite.GetIBCToken(chainADenom, channelA.Counterparty.PortID, channelA.Counterparty.ChannelID)

t.Run("native IBC token transfer from chainA to chainB, sender is source of tokens", func(t *testing.T) {
transferTxResp := s.Transfer(ctx, chainA, chainAWallet, channelA.PortID, channelA.ChannelID, sdk.NewCoins(testvalues.DefaultTransferAmount(chainADenom)), chainAAddress, chainBAddress, s.GetTimeoutHeight(ctx, chainB), 0, "")
s.AssertTxSuccess(transferTxResp)
})

s.Require().NoError(test.WaitForBlocks(ctx, 5, chainA, chainB), "failed to wait for blocks")

t.Run("native chainA tokens are escrowed", func(t *testing.T) {
actualBalance, err := s.GetChainANativeBalance(ctx, chainAWallet)
s.Require().NoError(err)

expected := testvalues.StartingTokenAmount - testvalues.IBCTransferAmount
s.Require().Equal(expected, actualBalance)

actualTotalEscrow, err := query.TotalEscrowForDenom(ctx, chainA, chainADenom)
s.Require().NoError(err)

expectedTotalEscrow := sdk.NewCoin(chainADenom, sdkmath.NewInt(testvalues.IBCTransferAmount))
s.Require().Equal(expectedTotalEscrow, actualTotalEscrow)
})

t.Run("start relayer", func(t *testing.T) {
s.StartRelayer(relayer)
})

t.Run("packets are relayed", func(t *testing.T) {
s.AssertPacketRelayed(ctx, chainA, channelA.PortID, channelA.ChannelID, 1)

actualBalance, err := query.Balance(ctx, chainB, chainBAddress, chainBIBCToken.IBCDenom())
s.Require().NoError(err)

expected := testvalues.IBCTransferAmount
s.Require().Equal(expected, actualBalance.Int64())
})

t.Run("metadata for IBC denomination exists on chainB", func(t *testing.T) {
s.AssertHumanReadableDenom(ctx, chainB, chainADenom, channelA)
})

// send the native chainB denom and also the ibc token from chainA
denoms := []string{chainBIBCToken.IBCDenom(), chainBDenom}
var transferCoins []sdk.Coin
for _, denom := range denoms {
transferCoins = append(transferCoins, testvalues.DefaultTransferAmount(denom))
}

t.Run("native token from chain B and non-native IBC token from chainA, both to chainA", func(t *testing.T) {
transferTxResp := s.Transfer(ctx, chainB, chainBWallet, channelA.Counterparty.PortID, channelA.Counterparty.ChannelID, transferCoins, chainBAddress, testvalues.InvalidAddress, s.GetTimeoutHeight(ctx, chainA), 0, "")
s.AssertTxSuccess(transferTxResp)
})

s.Require().NoError(test.WaitForBlocks(ctx, 5, chainA, chainB), "failed to wait for blocks")

t.Run("native chainB tokens are escrowed", func(t *testing.T) {
Copy link
Contributor

@DimitrisJim DimitrisJim Jun 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

def seems like a race condition that could lead to flakyness, why can't this be placed right after the s.AssertTxSuccess call on line 322?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's to wait on the relayer. When you s.StartRelayer that blocks on clearing all packets, but as the relayer is already started, we need some wait condition. I think the quick trick is to wait on blocks. There's probably an issue somewhere to wait on the relayer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at that #1981

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but is this needed to query the balance on chain B right after the msg transfer is submitted to it? this should be query-able right after the tx succeeds? waiting for blocks means that this query might happen right after the tokens are returned to sender hence failing the assertion on lin 332?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like if we are checking the escrow amounts, the wait for block could actually be moved down

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup! think that would take care of failure in this test.

actualBalance, err := s.GetChainBNativeBalance(ctx, chainBWallet)
s.Require().NoError(err)

expected := testvalues.StartingTokenAmount - testvalues.IBCTransferAmount
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) {
s.AssertPacketRelayed(ctx, chainA, channelA.PortID, channelA.ChannelID, 1)
})

t.Run("token are returned to sender on chainB", func(t *testing.T) {
t.Run("non-native chainA ibc denom", func(t *testing.T) {
actualBalance, err := query.Balance(ctx, chainB, chainBAddress, chainBIBCToken.IBCDenom())
s.Require().NoError(err)

expected := testvalues.IBCTransferAmount
s.Require().Equal(expected, actualBalance.Int64())
})

t.Run("native chainB denom", func(t *testing.T) {
actualBalance, err := s.GetChainBNativeBalance(ctx, chainBWallet)
s.Require().NoError(err)

expected := testvalues.StartingTokenAmount
s.Require().Equal(expected, actualBalance)
})
})
}

// TestMsgTransfer_Fails_InvalidAddress attempts to send an IBC transfer to an invalid address and ensures
// that the tokens on the sending chain are unescrowed.
func (s *TransferTestSuite) TestMsgTransfer_Fails_InvalidAddress() {
Expand Down Expand Up @@ -200,7 +409,7 @@ func (s *TransferTestSuite) TestMsgTransfer_Timeout_Nonincentivized() {
t := s.T()
ctx := context.TODO()

relayer, channelA := s.SetupChainsRelayerAndChannel(ctx, s.TransferChannelOptions())
relayer, channelA := s.SetupChainsRelayerAndChannel(ctx, nil)
chainA, _ := s.GetChains()

chainAWallet := s.CreateUserOnChainA(ctx, testvalues.StartingTokenAmount)
Expand Down
Loading
Loading