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.

Sign up for GitHub

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

(test) Test that tokens are successfully forwarded from A to C through B #6758

Merged
merged 8 commits into from
Jul 8, 2024
Merged

(test) Test that tokens are successfully forwarded from A to C through B #6758

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2325143
Create new forwarding test
Jul 3, 2024
7b162da
Use a struct rather than raw fields
Jul 3, 2024
328d4d0
better docstring
Jul 3, 2024
bf7cc72
Merge branch 'main' into bznein/6578/forwardinge2etest
Jul 3, 2024
c7f066b
Merge branch 'main' into bznein/6578/forwardinge2etest
Jul 4, 2024
811c707
Change a type and remove callback
Jul 8, 2024
4bd7ecf
Merge branch 'main' into bznein/6578/forwardinge2etest
Jul 8, 2024
57d9877
Fix type
Jul 8, 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
29 changes: 23 additions & 6 deletions e2e/tests/transfer/forwarding_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,28 @@ func (s *TransferForwardingTestSuite) SetupSuite() {
// TestForwarding_WithLastChainBeingICS20v1_Succeeds tests the case where a token is forwarded and successfully
// received on a destination chain that is on ics20-v1 version.
func (s *TransferForwardingTestSuite) TestForwarding_WithLastChainBeingICS20v1_Succeeds() {
channelBToCCallback := func() ibc.ChannelOutput {
ctx := context.TODO()
// Creating a new path between chain B and chain C with a ICS20-v1 channel
opts := s.TransferChannelOptions()
opts.Version = transfertypes.V1
chains := s.GetAllChains()
channelBtoC, _ := s.CreatePath(ctx, chains[1], chains[2], ibc.DefaultClientOpts(), opts)
s.Require().Equal(transfertypes.V1, channelBtoC.Version, "the channel version is not ics20-1")
return channelBtoC
}
s.testForwardingThreeChains(channelBToCCallback)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two tests are identical, they only differ in the fact that the channel from B to C in one is the regular one we create in the setup, while in this one is a v1 channel.

I extracted all to a common function which accepts a parameter that specifies how the channel from B to C is obtained. This test overrides it, while the other one (below) simply returns the one already created.


// TestForwarding_Succeeds tests the case where a token is forwarded and successfully
// received on a destination chain.
func (s *TransferForwardingTestSuite) TestForwarding_Succeeds() {
s.testForwardingThreeChains(func() ibc.ChannelOutput {
return s.GetChainChannel(testsuite.ChainChannelPair{ChainIdx: 1, ChannelIdx: 1})
})
}

func (s *TransferForwardingTestSuite) testForwardingThreeChains(channelBToCCallback func() ibc.ChannelOutput) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like callback is more appropriate when you want to execute something based on something that has happened. But this is just an option, happy to go with it:

Suggested change
func (s *TransferForwardingTestSuite) testForwardingThreeChains(channelBToCCallback func() ibc.ChannelOutput) {
func (s *TransferForwardingTestSuite) testForwardingThreeChains(createChannelBToC func() ibc.ChannelOutput) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes sense, though I was talking to Cian on Thursday and it does not actually make sense to over-complicate by passing a function, we can leave the logic in the test (see newest commit)

ctx := context.TODO()
t := s.T()

Expand All @@ -41,12 +63,7 @@ func (s *TransferForwardingTestSuite) TestForwarding_WithLastChainBeingICS20v1_S
chainA, chainB, chainC := chains[0], chains[1], chains[2]

channelAtoB := s.GetChainAChannel()

// Creating a new path between chain B and chain C with a ICS20-v1 channel
opts := s.TransferChannelOptions()
opts.Version = transfertypes.V1
channelBtoC, _ := s.CreatePath(ctx, chainB, chainC, ibc.DefaultClientOpts(), opts)
s.Require().Equal(transfertypes.V1, channelBtoC.Version, "the channel version is not ics20-1")
channelBtoC := channelBToCCallback()

chainAWallet := s.CreateUserOnChainA(ctx, testvalues.StartingTokenAmount)
chainAAddress := chainAWallet.FormattedAddress()
Expand Down
20 changes: 18 additions & 2 deletions e2e/testsuite/testsuite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,12 +258,28 @@ func (s *E2ETestSuite) CreatePath(
return channelsA[len(channelsA)-1], channelsB[len(channelsB)-1]
}

type ChainChannelPair struct {
ChainIdx int
ChannelIdx int
Copy link
Contributor

Choose a reason for hiding this comment

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

think negative values aren't handled atm (plus uints just make sense as the type for indices)

Suggested change
ChainIdx int
ChannelIdx int
ChainIdx uint64
ChannelIdx uint64

}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might bloat a bit the file, but I'm not a fan of passing magic numbers around (it would be easy to switch them by mistake)

Copy link
Contributor

Choose a reason for hiding this comment

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

agree. don't know if we have consistency on this atm, though. might need replacements elsewhere (admittedly mixed usage would be worse)

// GetChainAChannel returns the ibc.ChannelOutput for the current test.
// this defaults to the first entry in the list, and will be what is needed in the case of
// a single channel test.
func (s *E2ETestSuite) GetChainAChannel() ibc.ChannelOutput {
chainA := s.GetAllChains()[0]
return s.GetChannels(chainA)[0]
return s.GetChainChannel(ChainChannelPair{ChainIdx: 0, ChannelIdx: 0})
}

// GetChainChannel returns the ibc.ChannelOutput for a specific
// entry in the list of chains.
func (s *E2ETestSuite) GetChainChannel(id ChainChannelPair) ibc.ChannelOutput {
chains := s.GetAllChains()
s.Require().Less(id.ChainIdx, len(chains), "required index %d is larger than the last index in the list of chains (%d)", id.ChainIdx, len(chains)-1)

chain := chains[id.ChainIdx]
channels := s.GetChannels(chain)
s.Require().Less(id.ChannelIdx, len(channels), "required channel index %d is larger than the last index in the list of channels (%d)", id.ChannelIdx, len(channels)-1)
return channels[id.ChannelIdx]
}

// GetChannels returns all channels for the current test.
Expand Down
Loading