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: schedule IBC software upgrade #4585

Merged
merged 21 commits into from
Sep 11, 2023
Merged

e2e: schedule IBC software upgrade #4585

Changes from 6 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
5550dfa
wip e2e test
charleenfei Sep 5, 2023
6d2d16b
deps: update the sdk 50 branch to main (#4391)
charleenfei Sep 5, 2023
fd45b73
query proposal
charleenfei Sep 6, 2023
1b648a1
merge conflict
charleenfei Sep 6, 2023
e704c0a
Merge branch 'feat/govv1' into charly/e2e-schedule-ibc-upgrad
charleenfei Sep 6, 2023
5d04194
update upgrade height in plan
charleenfei Sep 6, 2023
ab2bd3d
rm unnecessary wait/authority
charleenfei Sep 7, 2023
ad00d75
Merge branch 'feat/govv1' into charly/e2e-schedule-ibc-upgrad
charleenfei Sep 7, 2023
c85f06d
rm test artifact from merge
charleenfei Sep 7, 2023
371d0a4
add checks for scheduled plan
charleenfei Sep 7, 2023
c9872e9
hook up upgrade query client
charleenfei Sep 7, 2023
a71a3b5
plan height
charleenfei Sep 7, 2023
ce48d0b
pr fixes
charleenfei Sep 7, 2023
a395691
update test
charleenfei Sep 8, 2023
c52b67a
import space
charleenfei Sep 8, 2023
9a1fae9
update newchainID value
charleenfei Sep 11, 2023
1642975
update clientID upgrade
charleenfei Sep 11, 2023
ea7183a
linter
charleenfei Sep 11, 2023
ab25260
gci
charleenfei Sep 11, 2023
f8c5503
Merge branch 'feat/govv1' into charly/e2e-schedule-ibc-upgrad
charleenfei Sep 11, 2023
871de62
rm unnecessary event
charleenfei Sep 11, 2023
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
50 changes: 50 additions & 0 deletions e2e/tests/core/02-client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"testing"
"time"

"cosmossdk.io/x/upgrade/types"
"github.com/strangelove-ventures/interchaintest/v7/chain/cosmos"
"github.com/strangelove-ventures/interchaintest/v7/ibc"
test "github.com/strangelove-ventures/interchaintest/v7/testutil"
Expand Down Expand Up @@ -153,6 +154,55 @@ func (s *ClientTestSuite) TestClientUpdateProposal_Succeeds() {
})
}

// TestScheduleIBCUpgrade_Succeeds tests that a governance proposal to schedule an IBC software upgrade is successful.
func (s *ClientTestSuite) TestScheduleIBCUpgrade_Succeeds() {
t := s.T()
ctx := context.TODO()

_, _ = s.SetupChainsRelayerAndChannel(ctx)
chainA, chainB := s.GetChains()
chainAWallet := s.CreateUserOnChainA(ctx, testvalues.StartingTokenAmount)

s.Require().NoError(test.WaitForBlocks(ctx, 10, chainA, chainB))
charleenfei marked this conversation as resolved.
Show resolved Hide resolved

authority, err := s.QueryModuleAccountAddress(ctx, govtypes.ModuleName, chainA)
s.Require().NoError(err)
s.Require().NotNil(authority)

var (
originalTrustingPeriod time.Duration
newTrustingPeriod time.Duration
)

t.Run("send schedule IBC upgrade message", func(t *testing.T) {
authority, err := s.QueryModuleAccountAddress(ctx, govtypes.ModuleName, chainA)
s.Require().NoError(err)
s.Require().NotNil(authority)
charleenfei marked this conversation as resolved.
Show resolved Hide resolved

clientState, err := s.QueryClientState(ctx, chainA, ibctesting.FirstClientID)
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
s.Require().NoError(err)

originalTrustingPeriod = clientState.(*ibctm.ClientState).TrustingPeriod
newTrustingPeriod = originalTrustingPeriod + time.Duration(time.Hour*24)

upgradedClientState := clientState.(*ibctm.ClientState)
upgradedClientState.TrustingPeriod = newTrustingPeriod

latestHeight := clientState.GetLatestHeight().GetRevisionHeight()
colin-axner marked this conversation as resolved.
Show resolved Hide resolved

scheduleUpgradeMsg, err := clienttypes.NewMsgIBCSoftwareUpgrade(
authority.String(),
types.Plan{
Name: "upgrade-client",
Height: int64(latestHeight + 100),
},
upgradedClientState,
)
s.Require().NoError(err)
s.ExecuteGovProposalV1(ctx, scheduleUpgradeMsg, chainA, chainAWallet, 1)
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 wanted to query if the plan has been stored/upgraded client state was stored, but realised after digging around that the UpgradeKeeper doesn't seem to have a query client. what do you guys think? our msg_server tests do perform this check, is that sufficient?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can try to upgrade the client on the counterparty. Hermes might do it automatically. Maybe try starting the relayer and querying the client's status afterward

Copy link
Member

@damiannolan damiannolan Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

IIRC correctly hermes previously had an upgrade client cmd which actually submitted the proposal on your behalf. See here - and also here

At a glance I think this is supposed to be doing the upgrade client flow, but I'm unsure how hermes handles this. Does it block and wait(?) until upgrade height, then query the upgradedClient at upgradePath and then upgrade the counterparty.

I think their cmd would need to change for gov v1, maybe it still uses the legacy v1beta1 appraoch

edit: added additional link - looks like they have cmd to send proposal and separate to then carry out the upgrade, my thinking would be that the latter blocks and waits for upgrade height

Copy link
Member

Choose a reason for hiding this comment

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

You can try to upgrade the client on the counterparty. Hermes might do it automatically. Maybe try starting the relayer and querying the client's status afterward

Might need to check if interchaintest has built in the support for client updates. Hermes can do it based on the links in my first comment. But I'm unsure if rly also has the same support, and interchaintest would need to expose an API for it in their relayer interface.

Copy link
Contributor

Choose a reason for hiding this comment

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

We added event emission when the consensus state is written allowing the counterparty client to be upgraded, unsure if hermes started listening for that event

Copy link
Contributor

Choose a reason for hiding this comment

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

I regress on my suggestion, happy to finish the full e2e of upgrading the client in a followup pr as this isn't essential for v8 testing

Copy link
Contributor Author

@charleenfei charleenfei Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

after discussion with @colin-axner , will just merge this e2e testing the scope of the ScheduleIBCSoftwareUpgrade for now.

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 their cmd would need to change for gov v1, maybe it still uses the legacy v1beta1 appraoch

You're right, @damiannolan: hermes is using gov v1beta proposal. Let's ask them.

})
}

// TestRecoverClient_Succeeds tests that a governance proposal to recover a client using a MsgRecoverClient is successful.
func (s *ClientTestSuite) TestRecoverClient_Succeeds() {
t := s.T()
Expand Down