-
Notifications
You must be signed in to change notification settings - Fork 624
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
e2e: adding e2e upgrade test for ibc-go v7 #2902
Changes from 7 commits
b75938b
75db3a9
bd2834e
b3b63e2
59546d8
9013bcb
376b7cd
8a24eef
f3be122
bb07835
546c4a6
01e50c1
86cccb9
d5cca8b
623b6b7
deaa5ca
e6f1d2b
c9cc0b5
7960c22
1788963
d30011b
f13dc99
b21ebe1
d25710b
1a43b17
e037284
a2bf19c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,8 +24,11 @@ import ( | |
"github.com/cosmos/ibc-go/e2e/testvalues" | ||
controllertypes "github.com/cosmos/ibc-go/v6/modules/apps/27-interchain-accounts/controller/types" | ||
icatypes "github.com/cosmos/ibc-go/v6/modules/apps/27-interchain-accounts/types" | ||
v7 "github.com/cosmos/ibc-go/v6/modules/core/02-client/migrations/v7" | ||
clienttypes "github.com/cosmos/ibc-go/v6/modules/core/02-client/types" | ||
ibctesting "github.com/cosmos/ibc-go/v6/testing" | ||
simappupgrades "github.com/cosmos/ibc-go/v6/testing/simapp/upgrades" | ||
v7upgrades "github.com/cosmos/ibc-go/v6/testing/simapp/upgrades/v7" | ||
) | ||
|
||
const ( | ||
|
@@ -70,6 +73,8 @@ func (s *UpgradeTestSuite) UpgradeChain(ctx context.Context, chain *cosmos.Cosmo | |
err = chain.StartAllNodes(ctx) | ||
s.Require().NoError(err, "error starting upgraded node(s)") | ||
|
||
s.InitGRPCClients(chain) | ||
chatton marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just a note for the future, it might make sense to bundle these into a |
||
|
||
timeoutCtx, timeoutCtxCancel = context.WithTimeout(ctx, time.Minute*2) | ||
defer timeoutCtxCancel() | ||
|
||
|
@@ -386,6 +391,147 @@ func (s *UpgradeTestSuite) TestV5ToV6ChainUpgrade() { | |
}) | ||
} | ||
|
||
func (s *UpgradeTestSuite) TestV6ToV7ChainUpgrade() { | ||
colin-axner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
t := s.T() | ||
testCfg := testconfig.FromEnv() | ||
|
||
ctx := context.Background() | ||
relayer, channelA := s.SetupChainsRelayerAndChannel(ctx) | ||
chainA, chainB := s.GetChains() | ||
|
||
createClientOptions := ibc.CreateClientOptions{ | ||
TrustingPeriod: time.Duration(time.Hour * 24 * 7).String(), | ||
colin-axner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
// create second tendermint client | ||
s.SetupClients(ctx, relayer, createClientOptions) | ||
|
||
status, err := s.Status(ctx, chainA, ibctesting.FirstClientID) | ||
s.Require().NoError(err) | ||
s.Require().Equal("Active", status) | ||
colin-axner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
status, err = s.Status(ctx, chainA, ibctesting.SecondClientID) | ||
s.Require().NoError(err) | ||
s.Require().Equal("Active", status) | ||
|
||
var ( | ||
chainADenom = chainA.Config().Denom | ||
chainBIBCToken = testsuite.GetIBCToken(chainADenom, channelA.Counterparty.PortID, channelA.Counterparty.ChannelID) // IBC token sent to chainB | ||
soloMachineClientID = "06-solomachine-2" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I left a comment later about a function to name client IDs, I think this is a good place to use it! |
||
) | ||
|
||
chainAWallet := s.CreateUserOnChainA(ctx, testvalues.StartingTokenAmount) | ||
chainAAddress := chainAWallet.Bech32Address(chainA.Config().Bech32Prefix) | ||
|
||
chainBWallet := s.CreateUserOnChainB(ctx, testvalues.StartingTokenAmount) | ||
chainBAddress := chainBWallet.Bech32Address(chainB.Config().Bech32Prefix) | ||
|
||
// create solo machine client | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this comment is very useful. Maybe we can outline why we're using the solomachine type from |
||
solo := ibctesting.NewSolomachine(t, testsuite.Codec(), "solomachine", "testing", 1) | ||
|
||
legacyConsensusState := &v7.ConsensusState{ | ||
PublicKey: solo.ConsensusState().PublicKey, | ||
Diversifier: solo.ConsensusState().Diversifier, | ||
Timestamp: solo.ConsensusState().Timestamp, | ||
} | ||
|
||
legacyClientState := &v7.ClientState{ | ||
Sequence: solo.ClientState().Sequence, | ||
IsFrozen: solo.ClientState().IsFrozen, | ||
ConsensusState: legacyConsensusState, | ||
AllowUpdateAfterProposal: true, | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could we add a comment explaining why we need to construct the legacy types? That is, the ibc testing package uses the newest version of the solo machine which isn't supported by the chain we are broadcasting the message to, so we need to construct the legacy type in order to setup the pre upgraded chain with legacy state |
||
|
||
msgCreateSoloMachineClient, err := clienttypes.NewMsgCreateClient(legacyClientState, legacyConsensusState, chainAAddress) | ||
s.Require().NoError(err) | ||
|
||
resp, err := s.BroadcastMessages( | ||
ctx, | ||
chainA, | ||
chainAWallet, | ||
msgCreateSoloMachineClient, | ||
) | ||
|
||
s.AssertValidTxResponse(resp) | ||
s.Require().NoError(err) | ||
|
||
status, err = s.Status(ctx, chainA, soloMachineClientID) | ||
s.Require().NoError(err) | ||
s.Require().Equal("Active", status) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there is a constant we can use for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this seems like a good candidate for a subtest rather than being at the root level of the test |
||
|
||
res, err := s.ClientState(ctx, chainA, soloMachineClientID) | ||
s.Require().NoError(err) | ||
s.Require().Equal(res.ClientState.TypeUrl, "/ibc.lightclients.solomachine.v2.ClientState") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. another great candidate for t.Run !
colin-axner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
s.Require().NoError(test.WaitForBlocks(ctx, 1, chainA, chainB), "failed to wait for blocks") | ||
|
||
t.Run("IBC token transfer from chainA to chainB, sender is source of tokens", func(t *testing.T) { | ||
transferTxResp, err := s.Transfer(ctx, chainA, chainAWallet, channelA.PortID, channelA.ChannelID, testvalues.DefaultTransferAmount(chainADenom), chainAAddress, chainBAddress, s.GetTimeoutHeight(ctx, chainB), 0, "") | ||
s.Require().NoError(err) | ||
s.AssertValidTxResponse(transferTxResp) | ||
}) | ||
|
||
t.Run("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) | ||
}) | ||
|
||
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 := chainB.GetBalance(ctx, chainBAddress, chainBIBCToken.IBCDenom()) | ||
s.Require().NoError(err) | ||
|
||
expected := testvalues.IBCTransferAmount | ||
s.Require().Equal(expected, actualBalance) | ||
}) | ||
|
||
// create separate user specifically for the upgrade proposal to more easily verify starting | ||
// and end balances of the chainA users. | ||
chainAUpgradeProposalWallet := s.CreateUserOnChainA(ctx, testvalues.StartingTokenAmount) | ||
|
||
t.Run("upgrade chainA", func(t *testing.T) { | ||
s.UpgradeChain(ctx, chainA, chainAUpgradeProposalWallet, v7upgrades.UpgradeName, testCfg.ChainAConfig.Tag, testCfg.UpgradeTag) | ||
}) | ||
|
||
status, err = s.Status(ctx, chainA, ibctesting.FirstClientID) | ||
s.Require().NoError(err) | ||
s.Require().Equal("Active", status) | ||
|
||
status, err = s.Status(ctx, chainA, ibctesting.SecondClientID) | ||
s.Require().NoError(err) | ||
s.Require().Equal("Active", status) | ||
|
||
// send another transfer from chainA to chainB to make sure the upgrade did not break the packet flow | ||
t.Run("IBC token transfer from chainA to chainB, sender is source of tokens", func(t *testing.T) { | ||
transferTxResp, err := s.Transfer(ctx, chainA, chainAWallet, channelA.PortID, channelA.ChannelID, testvalues.DefaultTransferAmount(chainADenom), chainAAddress, chainBAddress, s.GetTimeoutHeight(ctx, chainB), 0, "") | ||
s.Require().NoError(err) | ||
s.AssertValidTxResponse(transferTxResp) | ||
}) | ||
|
||
t.Run("packets are relayed", func(t *testing.T) { | ||
s.AssertPacketRelayed(ctx, chainA, channelA.PortID, channelA.ChannelID, 1) | ||
|
||
actualBalance, err := chainB.GetBalance(ctx, chainBAddress, chainBIBCToken.IBCDenom()) | ||
s.Require().NoError(err) | ||
|
||
expected := testvalues.IBCTransferAmount * 2 | ||
s.Require().Equal(expected, actualBalance) | ||
}) | ||
|
||
// check that the v2 solo machine clientstate has been updated to the v3 solo machine clientstate | ||
res, err = s.ClientState(ctx, chainA, soloMachineClientID) | ||
s.Require().NoError(err) | ||
s.Require().Equal(res.ClientState.TypeUrl, "/ibc.lightclients.solomachine.v3.ClientState") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Normally we could use You could use what the impl of that function does tho, something like:
|
||
} | ||
|
||
// RegisterInterchainAccount will attempt to register an interchain account on the counterparty chain. | ||
func (s *UpgradeTestSuite) RegisterInterchainAccount(ctx context.Context, chain *cosmos.CosmosChain, user *ibc.Wallet, msgRegisterAccount *intertxtypes.MsgRegisterAccount) error { | ||
txResp, err := s.BroadcastMessages(ctx, chain, user, msgRegisterAccount) | ||
|
@@ -405,3 +551,29 @@ func getICAVersion(chainAVersion, chainBVersion string) string { | |
// explicitly set the version string because the host chain might not yet support incentivized channels. | ||
return icatypes.NewDefaultMetadataString(ibctesting.FirstConnectionID, ibctesting.FirstConnectionID) | ||
} | ||
|
||
// Status queries the status of the client by clientID | ||
func (s *UpgradeTestSuite) Status(ctx context.Context, chain ibc.Chain, clientID string) (string, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe this can be renamed to |
||
queryClient := s.GetChainGRCPClients(chain).ClientQueryClient | ||
res, err := queryClient.ClientStatus(ctx, &clienttypes.QueryClientStatusRequest{ | ||
ClientId: clientID, | ||
}) | ||
if err != nil { | ||
return "", err | ||
} | ||
|
||
return res.Status, nil | ||
} | ||
|
||
// ClientState queries the current ClientState by clientID | ||
func (s *UpgradeTestSuite) ClientState(ctx context.Context, chain ibc.Chain, clientID string) (*clienttypes.QueryClientStateResponse, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fetching client state might be something we want to do in other tests, I'm happy to leave it here for now, but it might be worth moving this to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i could move this one and Status as well, I think they are generic enough functions There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah you mean to return
colin-axner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
queryClient := s.GetChainGRCPClients(chain).ClientQueryClient | ||
res, err := queryClient.ClientState(ctx, &clienttypes.QueryClientStateRequest{ | ||
ClientId: clientID, | ||
}) | ||
if err != nil { | ||
return res, err | ||
} | ||
|
||
return res, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,8 +76,8 @@ type GRPCClients struct { | |
InterTxQueryClient intertxtypes.QueryClient | ||
|
||
// SDK query clients | ||
GovQueryClient govtypesv1beta1.QueryClient | ||
GovQueryClientV1 govtypesv1.QueryClient | ||
GovQueryClient govtypesv1beta1.QueryClient | ||
GovQueryClientV1 govtypesv1.QueryClient | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh weird this kinda seems like we're not auto formatting the test code if linting was passing without the correct indentation. |
||
GroupsQueryClient grouptypes.QueryClient | ||
ParamsQueryClient paramsproposaltypes.QueryClient | ||
AuthQueryClient authtypes.QueryClient | ||
|
@@ -167,8 +167,8 @@ func (s *E2ETestSuite) SetupChainsRelayerAndChannel(ctx context.Context, channel | |
time.Sleep(time.Second * 10) | ||
} | ||
|
||
s.initGRPCClients(chainA) | ||
s.initGRPCClients(chainB) | ||
s.InitGRPCClients(chainA) | ||
s.InitGRPCClients(chainB) | ||
|
||
chainAChannels, err := r.GetChannels(ctx, eRep, chainA.Config().ChainID) | ||
s.Require().NoError(err) | ||
|
@@ -363,7 +363,7 @@ func (s *E2ETestSuite) GetChainGRCPClients(chain ibc.Chain) GRPCClients { | |
|
||
// initGRPCClients establishes GRPC clients with the given chain. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [nit] can we update the docstring too to indicate it's now exported? |
||
// The created GRPCClients can be retrieved with GetChainGRCPClients. | ||
func (s *E2ETestSuite) initGRPCClients(chain *cosmos.CosmosChain) { | ||
func (s *E2ETestSuite) InitGRPCClients(chain *cosmos.CosmosChain) { | ||
// Create a connection to the gRPC server. | ||
grpcConn, err := grpc.Dial( | ||
chain.GetHostGRPCAddress(), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,8 @@ const ( | |
FirstChannelID = "channel-0" | ||
FirstConnectionID = "connection-0" | ||
|
||
SecondClientID = "07-tendermint-1" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. now that solomachine clients are in the mix, what do you think about instead of this constant, we have a function like secondClientID := TendermintClientID(2) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this idea! |
||
|
||
// Default params constants used to create a TM client | ||
TrustingPeriod time.Duration = time.Hour * 24 * 7 * 2 | ||
UnbondingPeriod time.Duration = time.Hour * 24 * 7 * 3 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe the alias should be
v7migrations
instead of justv7
to make it explicit in the tests. The same way we havev7upgrades