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

e2e: adding e2e upgrade test for ibc-go v7 #2902

Merged
merged 27 commits into from
Dec 12, 2022
Merged

e2e: adding e2e upgrade test for ibc-go v7 #2902

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
b75938b
scaff
charleenfei Dec 6, 2022
75db3a9
update test
charleenfei Dec 7, 2022
bd2834e
add test for automatic migration of solomachine clientstate version
charleenfei Dec 7, 2022
b3b63e2
Merge branch 'main' into charly/e2e-6-7
charleenfei Dec 7, 2022
59546d8
fix comment on util function
charleenfei Dec 7, 2022
9013bcb
Merge branch 'main' into charly/e2e-6-7
charleenfei Dec 8, 2022
376b7cd
Merge branch 'main' into charly/e2e-6-7
charleenfei Dec 8, 2022
8a24eef
add clientID generation functions
charleenfei Dec 8, 2022
f3be122
Merge branch 'main' into charly/e2e-6-7
charleenfei Dec 8, 2022
bb07835
update
charleenfei Dec 8, 2022
546c4a6
Merge branch 'charly/e2e-6-7' of github.com:cosmos/ibc-go into charly…
charleenfei Dec 8, 2022
01e50c1
update re: pr comments
charleenfei Dec 8, 2022
86cccb9
update godoc
charleenfei Dec 8, 2022
d5cca8b
update ibctest to latest commit
charleenfei Dec 8, 2022
623b6b7
Merge branch 'main' into charly/e2e-6-7
charleenfei Dec 8, 2022
deaa5ca
update chainconfig
charleenfei Dec 8, 2022
e6f1d2b
Merge branch 'main' into charly/update_ibctest
charleenfei Dec 9, 2022
c9cc0b5
update to commit fixing broadcastTx
charleenfei Dec 11, 2022
7960c22
Merge branch 'charly/update_ibctest' into charly/e2e-6-7
charleenfei Dec 11, 2022
1788963
update e2e upgrades workflow
charleenfei Dec 12, 2022
d30011b
update workflow to set relayer tag as not required
charleenfei Dec 12, 2022
f13dc99
Merge branch 'main' into charly/e2e-6-7
charleenfei Dec 12, 2022
b21ebe1
Merge branch 'main' into charly/e2e-6-7
charleenfei Dec 12, 2022
d25710b
update sprintf message
charleenfei Dec 12, 2022
1a43b17
Merge branch 'charly/e2e-6-7' of github.com:cosmos/ibc-go into charly…
charleenfei Dec 12, 2022
e037284
Merge branch 'main' into charly/e2e-6-7
charleenfei Dec 12, 2022
a2bf19c
Merge branch 'main' into charly/e2e-6-7
charleenfei Dec 12, 2022
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
Prev Previous commit
Next Next commit
add test for automatic migration of solomachine clientstate version
  • Loading branch information
@charleenfei
charleenfei committed Dec 7, 2022
commit bd2834e590d03ee310bcb596bafded2956290f10
102 changes: 81 additions & 21 deletions e2e/tests/upgrades/upgrade_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
v7upgrades "github.com/cosmos/ibc-go/v6/testing/simapp/upgrades/v7"
v6upgrades "github.com/cosmos/interchain-accounts/app/upgrades/v6"
intertxtypes "github.com/cosmos/interchain-accounts/x/inter-tx/types"
"github.com/gogo/protobuf/proto"
Expand All @@ -25,9 +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"
Copy link
Contributor

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 just v7 to make it explicit in the tests. The same way we have v7upgrades

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 (
Expand Down Expand Up @@ -72,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
Copy link
Contributor

Choose a reason for hiding this comment

The 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 restartChain function. If we ever have another function which stops a chain, we will need to realize to initialize the grpc clients again. I'm very fine with this for now


timeoutCtx, timeoutCtxCancel = context.WithTimeout(ctx, time.Minute*2)
defer timeoutCtxCancel()

Expand Down Expand Up @@ -389,18 +392,6 @@ func (s *UpgradeTestSuite) TestV5ToV6ChainUpgrade() {
}

func (s *UpgradeTestSuite) TestV6ToV7ChainUpgrade() {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

// Pre-upgrade logic
// Create multiple tendermint clients, ideally update both at least once
// Create a channel, send a transfer and receive a transfer
// Create a solo machine client (no need to perform actions with the solo machine client)

// Post upgrade logic
// Send a packet on the existing channel
// Query the tendermint clients
// Query the solo machine client
// Optional: send a MsgUpdate in reference to the solo machine client (proof can be invalid, mostly want to ensure the msg does not panic when referencing a solo machine clientID)

t := s.T()
testCfg := testconfig.FromEnv()

Expand All @@ -419,16 +410,14 @@ func (s *UpgradeTestSuite) TestV6ToV7ChainUpgrade() {
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, "07-tendermint-1")
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

// chainBDenom = chainB.Config().Denom
// chainAIBCToken = testsuite.GetIBCToken(chainBDenom, channelA.PortID, channelA.ChannelID) // IBC token sent to chainA
chainADenom = chainA.Config().Denom
chainBIBCToken = testsuite.GetIBCToken(chainADenom, channelA.Counterparty.PortID, channelA.Counterparty.ChannelID) // IBC token sent to chainB
soloMachineClientID = "06-solomachine-2"
Copy link
Contributor

Choose a reason for hiding this comment

The 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)
Expand All @@ -437,9 +426,46 @@ func (s *UpgradeTestSuite) TestV6ToV7ChainUpgrade() {
chainBWallet := s.CreateUserOnChainB(ctx, testvalues.StartingTokenAmount)
chainBAddress := chainBWallet.Bech32Address(chainB.Config().Bech32Prefix)

// create solo machine client
Copy link
Contributor

Choose a reason for hiding this comment

The 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 ibctesting instead. i.e. we don't have a real implementation so we can use this to simulate it.

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,
}
Copy link
Contributor

Choose a reason for hiding this comment

The 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)
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 there is a constant we can use for Active

Copy link
Contributor

Choose a reason for hiding this comment

The 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")
Copy link
Contributor

Choose a reason for hiding this comment

The 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("native IBC token transfer from chainA to chainB, sender is source of tokens", func(t *testing.T) {
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)
Expand Down Expand Up @@ -479,10 +505,31 @@ func (s *UpgradeTestSuite) TestV6ToV7ChainUpgrade() {
s.Require().NoError(err)
s.Require().Equal("Active", status)

status, err = s.Status(ctx, chainA, "07-tendermint-1")
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")
Copy link
Contributor

Choose a reason for hiding this comment

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

is "/ibc.lightclients.solomachine.v3.ClientState" a constant somewhere we can use? Maybe we should create one if not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Normally we could use MsgTypeURL from the sdk and pass a new/empty default instance of the type, but this won't work I think because ClientState doesn't implement the sdk.Msg interface.

You could use what the impl of that function does tho, something like:

fmt.Sprint("/", proto.MessageName(&solomachine.ClientState{}))

}

// RegisterInterchainAccount will attempt to register an interchain account on the counterparty chain.
Expand Down Expand Up @@ -517,3 +564,16 @@ func (s *UpgradeTestSuite) Status(ctx context.Context, chain ibc.Chain, clientID

return res.Status, nil
}

// Status queries the current status of the client
func (s *UpgradeTestSuite) ClientState(ctx context.Context, chain ibc.Chain, clientID string) (*clienttypes.QueryClientStateResponse, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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 E2ETestSuite WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah you mean to return QueryClientStatusRequest and use one function?

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
}
10 changes: 5 additions & 5 deletions e2e/testsuite/testsuite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -363,7 +363,7 @@ func (s *E2ETestSuite) GetChainGRCPClients(chain ibc.Chain) GRPCClients {

// initGRPCClients establishes GRPC clients with the given chain.
Copy link
Contributor

Choose a reason for hiding this comment

The 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(),
Expand Down
2 changes: 2 additions & 0 deletions testing/values.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ const (
FirstChannelID = "channel-0"
FirstConnectionID = "connection-0"

SecondClientID = "07-tendermint-1"
Copy link
Contributor

Choose a reason for hiding this comment

The 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)

Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand Down