E2E: adding incentivized interchain accounts bank send test#2023
E2E: adding incentivized interchain accounts bank send test#2023damiannolan merged 39 commits intomainfrom
Conversation
https://github.com/cosmos/ibc-go into cian/issue#1911-support-backwards-compatibility-tests
…terchain-accounts-sample-test
| "github.com/cosmos/ibc-go/e2e/testconfig" | ||
| "github.com/cosmos/ibc-go/e2e/testsuite" | ||
| "github.com/cosmos/ibc-go/e2e/testvalues" | ||
| feetypes "github.com/cosmos/ibc-go/v5/modules/apps/29-fee/types" | ||
| ibctesting "github.com/cosmos/ibc-go/v5/testing" |
There was a problem hiding this comment.
How is this import grouping for e2e? Is this okay, or should we separate ibc-go imports?
There was a problem hiding this comment.
i dont have a strong opinion, but separation seems clearer to me
damiannolan
requested review from
AdityaSripal,
charleenfei,
chatton,
colin-axner,
crodriguezvega and
seantking
as code owners
This reverts commit 789cef4.
charleenfei
left a comment
charleenfei
left a comment
There was a problem hiding this comment.
adding my approving review, although since i also worked on this branch we should have at least one more reviewer, cc @seantking
chatton
left a comment
chatton
left a comment
There was a problem hiding this comment.
Looks good just a few small comments.
I think we can hold off for my PR to be merged and then change the base branch to main.
| func (s *InterchainAccountsTestSuite) QueryIncentivizedPacketsForChannel( | ||
| ctx context.Context, | ||
| chain *cosmos.CosmosChain, | ||
| portId, | ||
| channelId string, | ||
| ) ([]*feetypes.IdentifiedPacketFees, error) { | ||
| queryClient := s.GetChainGRCPClients(chain).FeeQueryClient | ||
| res, err := queryClient.IncentivizedPacketsForChannel(ctx, &feetypes.QueryIncentivizedPacketsForChannelRequest{ | ||
| PortId: portId, | ||
| ChannelId: channelId, | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return res.IncentivizedPackets, err | ||
| } | ||
|
|
||
| // RegisterCounterPartyPayee broadcasts a MsgRegisterCounterpartyPayee message. | ||
| func (s *InterchainAccountsTestSuite) RegisterCounterPartyPayee(ctx context.Context, chain *cosmos.CosmosChain, | ||
| user *ibctest.User, portID, channelID, relayerAddr, counterpartyPayeeAddr string) (sdk.TxResponse, error) { | ||
| msg := feetypes.NewMsgRegisterCounterpartyPayee(portID, channelID, relayerAddr, counterpartyPayeeAddr) | ||
| return s.BroadcastMessages(ctx, chain, user, msg) | ||
| } | ||
|
|
||
| // QueryCounterPartyPayee queries the counterparty payee of the given chain and relayer address on the specified channel. | ||
| func (s *InterchainAccountsTestSuite) QueryCounterPartyPayee(ctx context.Context, chain ibc.Chain, relayerAddress, channelID string) (string, error) { | ||
| queryClient := s.GetChainGRCPClients(chain).FeeQueryClient | ||
| res, err := queryClient.CounterpartyPayee(ctx, &feetypes.QueryCounterpartyPayeeRequest{ | ||
| ChannelId: channelID, | ||
| Relayer: relayerAddress, | ||
| }) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| return res.CounterpartyPayee, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Think these are duplicates of the functions belonging to FeeMiddlewareTestSuite
If they are needed in two test suites, maybe they should go into testsuite.E2ETestSuite?
Either that or we could extract a type that has these functions and compose both of the other test suites of that type.
There was a problem hiding this comment.
Yeah, I agree. Do you think we should do this in a follow up PR?
There was a problem hiding this comment.
yeah I think that's no problem!
| func (s *E2ETestSuite) QueryClientState(ctx context.Context, chain ibc.Chain, clientID string) (ibcexported.ClientState, error) { | ||
| queryClient := s.GetChainGRCPClients(chain).ClientQueryClient | ||
| res, err := queryClient.ClientState(ctx, &clienttypes.QueryClientStateRequest{ | ||
| ClientId: clientID, | ||
| }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| clientState, err := clienttypes.UnpackClientState(res.ClientState) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return clientState, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
this function was going to be used in one of @colin-axner 's coming tests I believe. I don't think we should remove this.
There was a problem hiding this comment.
Wow... I actually have no idea how this got removed. I never touched it 🤣
I'll put it back!
| } | ||
|
|
||
| // QueryIncentivizedPacketsForChannel queries the incentivized packets on the specified channel. | ||
| func (s *InterchainAccountsTestSuite) QueryIncentivizedPacketsForChannel( |
There was a problem hiding this comment.
I think we should put all incentivized tests under one test suite. Since ics29 was added in v4 but ics27 was added in v3. We will want to do base ics27 tests using v3 for cross compatibility purposes, but incentivization tests should only occur as far back as v4
| err := relayer.StopRelayer(ctx, s.GetRelayerExecReporter()) | ||
| s.Require().NoError(err) |
There was a problem hiding this comment.
for simplicity, I wonder if we should just make this a suite function so it matches s.StartRelayer(relayer)
| s.Require().NoError(err) | ||
| }) | ||
|
|
||
| t.Run("verify balance is the same", func(t *testing.T) { | ||
| balance, err := chainB.GetBalance(ctx, chainBAccount.Bech32Address(chainB.Config().Bech32Prefix), chainB.Config().Denom) | ||
| s.Require().NoError(err) | ||
|
|
||
| expected := testvalues.StartingTokenAmount | ||
| s.Require().Equal(expected, balance) | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Why was the packet relay assertion removed? Without knowing that the packet was relayed, we can't be certain that the balance didn't change because a failed transfer or because the packet wasn't relayed since we are checking the receiving balance and not the sender
chatton
changed the title
5 participants
Description
banktypes.MsgSendfor happy path scenariocloses: #XXXX
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes