Add ChanUpgradeCancel core handler #3846

chatton · 2023-06-14T13:29:38Z

Description

~~Depends on #3752~~

closes: #3753

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.

Targeted PR against correct branch (see CONTRIBUTING.md).
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards and Go style guide.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

…torechannel

…thub.com/cosmos/ibc-go into cian/issue#3649-implement-restorechannel

…ue#3753-implement-chanupgradecancel-handler-for-04-channel-keeper

…torechannel

…ue#3752-implement-writeupgradecancelchannel

…teupgradecancelchannel

chatton · 2023-06-14T15:04:38Z

modules/core/04-channel/keeper/upgrade.go

+	previousState := channel.State
+
 	k.restoreChannel(ctx, portID, channelID, channel)

+	k.Logger(ctx).Info("channel state updated", "port-id", portID, "channel-id", channelID, "previous-state", previousState, "new-state", types.OPEN.String())


unrelated quick fix to make it consistent with the other write functions.

modules/core/04-channel/keeper/upgrade.go

charleenfei · 2023-06-15T09:58:10Z

modules/core/04-channel/keeper/upgrade_test.go

+			path.EndpointA.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion
+			path.EndpointB.ChannelConfig.ProposedUpgrade.Fields.Version = mock.UpgradeVersion
+
+			err := path.EndpointA.ChanUpgradeInit()


nit

Suggested change

err := path.EndpointA.ChanUpgradeInit()

suite.Require().NoError(path.EndpointA.ChanUpgradeInit())

charleenfei · 2023-06-15T09:58:26Z

modules/core/04-channel/keeper/upgrade_test.go

+			}
+
+			err = path.EndpointB.ChanUpgradeTry()
+			suite.Require().NoError(err)


same nit as above

charleenfei · 2023-06-15T09:58:52Z

modules/core/04-channel/keeper/upgrade_test.go

+
+			suite.Require().NoError(path.EndpointB.UpdateClient())
+
+			// cause the upgrade to fail on chain b so an error receipt is written.


@damiannolan gets the credit for this one ❤️

charleenfei · 2023-06-15T10:01:29Z

modules/core/04-channel/keeper/upgrade.go

+		return errorsmod.Wrap(types.ErrInvalidUpgradeSequence, "error sequence must be less than current sequence")
+	}
+
+	channel.UpgradeSequence = errorReceipt.Sequence + 1


thinking about this more, should we actually update the upgrade sequences in the Write functions also? this is the one mutation we make in these handler functions. cc @damiannolan @colin-axner

this did feel a little off, when I was first re-reading what I had written, I was confused for a minute as the channel state didn't change. I think I would be in favour of maybe either returning the desired sequence or just doing the receipt + 1 in the write fn.

As discussed today, I think we can remove the +1 in a followup pr since channel upgrade initialization should bump the upgrade sequence

charleenfei · 2023-06-15T10:02:37Z

modules/core/04-channel/types/errors.go

@@ -47,4 +47,5 @@ var (
 	ErrIncompatibleCounterpartyUpgrade = errorsmod.Register(SubModuleName, 31, "incompatible counterparty upgrade")
 	ErrInvalidUpgradeError             = errorsmod.Register(SubModuleName, 32, "invalid upgrade error")
 	ErrInvalidFlushStatus              = errorsmod.Register(SubModuleName, 33, "invalid flush status")
+	ErrInvalidUpgradeErrorReceipt      = errorsmod.Register(SubModuleName, 34, "invalid error receipt")


are we using this anywhere?

we are not! This actually was introduced when I was doing the empty error receipt check in the keeper function (which was removed since this happens in validate basic.)

We can remove this if we don't do that check.

crodriguezvega

Looks good, @chatton!

modules/core/04-channel/keeper/upgrade_test.go

crodriguezvega · 2023-06-15T10:03:56Z

modules/core/04-channel/types/errors.go

@@ -47,4 +47,5 @@ var (
 	ErrIncompatibleCounterpartyUpgrade = errorsmod.Register(SubModuleName, 31, "incompatible counterparty upgrade")
 	ErrInvalidUpgradeError             = errorsmod.Register(SubModuleName, 32, "invalid upgrade error")
 	ErrInvalidFlushStatus              = errorsmod.Register(SubModuleName, 33, "invalid flush status")
+	ErrInvalidUpgradeErrorReceipt      = errorsmod.Register(SubModuleName, 34, "invalid error receipt")


Is this error actually used?

it used to be!

…nupgradecancel-handler-for-04-channel-keeper

…-04-channel-keeper' of https://github.com/cosmos/ibc-go into an/issue#3753-implement-chanupgradecancel-handler-for-04-channel-keeper

charleenfei · 2023-06-16T10:28:50Z

modules/core/04-channel/keeper/upgrade_test.go

+				channel := path.EndpointA.GetChannel()
+				suite.Require().Equal(errorReceipt.Sequence+1, channel.UpgradeSequence, "upgrade sequence should be incremented")
+			} else {
+				suite.Require().ErrorIs(tc.expError, err)


Suggested change

suite.Require().ErrorIs(tc.expError, err)

suite.Require().ErrorIs(err, tc.expError)

chatton · 2023-06-16T13:14:07Z

@colin-axner / @damiannolan if you have any concerns about these changes we can include them in #3848

colin-axner

LGTM, I can do a followup pr with the small nits/suggestions I found, #3901

colin-axner · 2023-06-19T10:36:42Z

modules/core/04-channel/keeper/upgrade.go

+	currentSequence := channel.UpgradeSequence
+	counterpartySequence := errorReceipt.Sequence
+	if counterpartySequence < currentSequence {
+		return errorsmod.Wrap(types.ErrInvalidUpgradeSequence, "error sequence must be less than current sequence")


I think this should say:

error receipt sequence must be greater than or equal to current sequence

And maybe we should add the sequence values?

colin-axner · 2023-06-19T10:38:00Z

modules/core/04-channel/keeper/upgrade.go

+		return errorsmod.Wrap(types.ErrInvalidUpgradeSequence, "error sequence must be less than current sequence")
+	}
+
+	channel.UpgradeSequence = errorReceipt.Sequence + 1


As discussed today, I think we can remove the +1 in a followup pr since channel upgrade initialization should bump the upgrade sequence

colin-axner · 2023-06-19T10:38:41Z

modules/core/04-channel/keeper/upgrade_test.go

+			expError: connectiontypes.ErrConnectionNotFound,
+		},
+		{
+			name: "counter partyupgrade sequence less than current sequence",


typo, should be:

counterparty upgrade

colin-axner · 2023-06-19T11:15:18Z

modules/core/04-channel/keeper/upgrade_test.go

+
+				suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, errorReceipt)
+
+				suite.coordinator.CommitBlock(suite.chainB)


context: #3900

When CommitBlock is called, it commits the current header set within the test chain. This is created on a call to NextBlock. The state changes are not being reflected to the AppHash being committed. So when CommitBlock is being called the first time, it doesn't include the provable state changes (which is why calling it once results in an invalid proof). Committing the current block should reflect the current state changes. We should update it. I think the difficulty is getting the working app hash as this is currently private in the sdk

colin-axner · 2023-06-19T11:16:13Z

modules/core/04-channel/keeper/upgrade_test.go

+
+				suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, errorReceipt)
+
+				suite.coordinator.CommitBlock(suite.chainB)


The testing pkg has some cleanup in its future. I'd like to simplify the API a bit (there shouldn't be CommitBlock and NextBlock)

colin-axner · 2023-06-19T11:16:27Z

modules/core/04-channel/keeper/upgrade_test.go

+
+			suite.Require().NoError(path.EndpointA.ChanUpgradeInit())
+
+			suite.Require().NoError(path.EndpointB.UpdateClient())


can be removed

chatton and others added 30 commits June 1, 2023 16:40

wip: adding restoreChannel and writeErrorReceipt functions

5cd004e

merge 04-channel-upgrades

cbc8e32

emit error receipt events on abort

d01355f

remove unused function

6f7940b

added happy path test for abort

6d50848

fleshed out TestAbortHandshake

795cd05

corrected docstring and removed unrequired checks in test

dfbc5d4

merge feature branch

b2a99ca

chore: added implementation of WriteUpgradeCancelChannel

d429514

Merge branch '04-channel-upgrades' into cian/issue#3649-implement-res…

4330c51

…torechannel

fix linting

c1e05b8

Merge branch 'cian/issue#3649-implement-restorechannel' of https://gi…

619381b

…thub.com/cosmos/ibc-go into cian/issue#3649-implement-restorechannel

addressing PR feedback

91605c1

merge with restore branch

d050ee8

addressing PR feedback

eaa2b91

merge 04-channel-upgrades

d6bd3bf

Merge branch 'cian/issue#3649-implement-restorechannel' into cian/iss…

cfe59ae

…ue#3753-implement-chanupgradecancel-handler-for-04-channel-keeper

chore: adding implementation of ChanUpgradeCancel

7d2c3d4

Merge branch '04-channel-upgrades' into cian/issue#3649-implement-res…

099b147

…torechannel

added scaffolding for TestChanUpgradeCancel

759474f

addressing PR feedback

3916332

merge 04-channel-upgrades

700acba

Merge branch 'cian/issue#3649-implement-restorechannel' into cian/iss…

6b2ce2e

…ue#3752-implement-writeupgradecancelchannel

addressing PR feedback

811c418

Merge branch '04-channel-upgrades' into cian/issue#3752-implement-wri…

0f0305f

…teupgradecancelchannel

merge 04-channel-upgrades

d6bd69c

fix merge conflicts

51e9070

ran linter

3e0bf6e

merge 04-channel-upgrades

4e12bc0

merge 04-channel-upgrades

df123ba

chatton added 3 commits June 14, 2023 11:49

happy path test

098d0a3

adding tests for ChanUpgradeCancel

853eae2

chore: merge 04-channel-upgrades

8ced59b

chatton commented Jun 14, 2023

View reviewed changes

chatton marked this pull request as ready for review June 14, 2023 15:04

chatton requested review from colin-axner, AdityaSripal and damiannolan as code owners June 14, 2023 15:04

chatton mentioned this pull request Jun 14, 2023

Implement MsgChannelUpgradeCancel message server handler #3848

Merged

9 tasks

crodriguezvega changed the title ~~Add ChanUpgradeCancel handler~~ Add ChanUpgradeCancel core handler Jun 15, 2023

charleenfei reviewed Jun 15, 2023

View reviewed changes

crodriguezvega approved these changes Jun 15, 2023

View reviewed changes

chatton and others added 3 commits June 16, 2023 09:32

addresing PR feedback

bf10856

Merge branch '04-channel-upgrades' into cian/issue#3753-implement-cha…

0e34c52

…nupgradecancel-handler-for-04-channel-keeper

Merge branch 'cian/issue#3753-implement-chanupgradecancel-handler-for…

8405547

…-04-channel-keeper' of https://github.com/cosmos/ibc-go into an/issue#3753-implement-chanupgradecancel-handler-for-04-channel-keeper

charleenfei reviewed Jun 16, 2023

View reviewed changes

corrected assertion arguments

2f540ce

charleenfei approved these changes Jun 16, 2023

View reviewed changes

chatton merged commit c0943cc into 04-channel-upgrades Jun 16, 2023

chatton deleted the cian/issue#3753-implement-chanupgradecancel-handler-for-04-channel-keeper branch June 16, 2023 13:11

colin-axner reviewed Jun 19, 2023

View reviewed changes

This was referenced Jun 19, 2023

Commit pending state changes before updating client in testing pkg #3900

Open

chore: upgrade cancel handler minor followups #3901

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ChanUpgradeCancel core handler #3846

Add ChanUpgradeCancel core handler #3846

chatton commented Jun 14, 2023 •

edited

Loading

chatton Jun 14, 2023

charleenfei Jun 15, 2023

charleenfei Jun 15, 2023

charleenfei Jun 15, 2023

chatton Jun 16, 2023

charleenfei Jun 15, 2023

chatton Jun 16, 2023

colin-axner Jun 19, 2023

charleenfei Jun 15, 2023

chatton Jun 16, 2023

crodriguezvega left a comment

crodriguezvega Jun 15, 2023

chatton Jun 16, 2023

charleenfei Jun 16, 2023

chatton commented Jun 16, 2023

colin-axner left a comment •

edited

Loading

colin-axner Jun 19, 2023

colin-axner Jun 19, 2023

colin-axner Jun 19, 2023

colin-axner Jun 19, 2023

colin-axner Jun 19, 2023

colin-axner Jun 19, 2023

	err := path.EndpointA.ChanUpgradeInit()
	suite.Require().NoError(path.EndpointA.ChanUpgradeInit())


		suite.Require().NoError(path.EndpointB.UpdateClient())

		// cause the upgrade to fail on chain b so an error receipt is written.

	suite.Require().ErrorIs(tc.expError, err)
	suite.Require().ErrorIs(err, tc.expError)


		suite.chainB.GetSimApp().IBCKeeper.ChannelKeeper.SetUpgradeErrorReceipt(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, errorReceipt)

		suite.coordinator.CommitBlock(suite.chainB)

Add ChanUpgradeCancel core handler #3846

Add ChanUpgradeCancel core handler #3846

Conversation

chatton commented Jun 14, 2023 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chatton commented Jun 16, 2023

colin-axner left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chatton commented Jun 14, 2023 •

edited

Loading

colin-axner left a comment •

edited

Loading