chore: implement msg server for `ChannelUpgradeTry` #3791

damiannolan · 2023-06-07T16:02:43Z

Description

Adding msg server implementation for ChannelUpgradeTry.
NOTE: testing app changes have been pulled out to #3812

closes: #3740

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.

…3739-upgrade-try-handler

…c-go into colin/3739-upgrade-try-handler

modules/core/keeper/msg_server.go

damiannolan · 2023-06-07T16:05:59Z

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

@@ -203,6 +203,10 @@ func (k Keeper) WriteUpgradeTryChannel(
 	emitChannelUpgradeTryEvent(ctx, portID, channelID, channel, proposedUpgrade)
 }

+func (k Keeper) AbortUpgrade(ctx sdk.Context, portID, channelID string, err error) error {


Pending #3728

modules/core/keeper/msg_server.go

…/3740-chan-upgrade-try-msg-server

…msg-server

…tructure

damiannolan · 2023-06-11T11:47:48Z

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

+	// TODO: determine flush status
+	// channel.FlushStatus = flushStatus


should be addressed in a future PR.

damiannolan · 2023-06-11T11:49:07Z

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

-	proposedUpgrade types.Upgrade,
-	flushStatus types.FlushStatus,
-) {
+func (k Keeper) WriteUpgradeTryChannel(ctx sdk.Context, portID, channelID string, upgrade types.Upgrade, upgradeVersion string) {


I can open another PR to replicate this on INIT - accepting the upgradeVersion as an arg and updating the upgrade inside this method as opposed to in the msg server

+1 for mutations in one function. Docstring could also mention that we set the upgrade in addition to the channel.

damiannolan · 2023-06-11T11:50:40Z

modules/core/keeper/msg_server.go

+		return nil, err
+	}
+
+	cacheCtx, writeFn := ctx.CacheContext()


state changes made the by app are not committed on error of the app callback, and instead, an error receipt is written for the current upgrade sequence.

…msg-server

damiannolan · 2023-06-11T12:03:49Z

modules/core/keeper/msg_server.go

+	return &channeltypes.MsgChannelUpgradeTryResponse{
+		Result:    channeltypes.SUCCESS,
+		ChannelId: msg.ChannelId,
+		Upgrade:   upgrade,
+	}, nil


Upgrade sequence is defined in the response but requires a channel lookup in the msg server as its no longer returned from ChanUpgradeTry. I'm not a massive fan of doing that lookup at this level. Upgrade sequence is still emitted in events.

anyone have any thoughts on this?

Im ok with removing that from response

I agree with not doing the lookup here, but maybe we could abstract into a function:

return formatUpgradeTrySuccessResponse()

or something. No strong preference, happy to remove now and add in the future upon request

DimitrisJim · 2023-06-11T14:41:47Z

modules/core/keeper/msg_server.go

+	upgradeVersion, err := cbs.OnChanUpgradeTry(cacheCtx, msg.PortId, msg.ChannelId, upgrade.Fields.Ordering, upgrade.Fields.ConnectionHops, upgrade.Fields.Version)
+	if err != nil {
+		if err := k.ChannelKeeper.AbortUpgrade(ctx, msg.PortId, msg.ChannelId, err); err != nil {
+			return nil, err


guess we should also be logging this error case? Similarly log a successful upgradetry before we return in 796.

AdityaSripal · 2023-06-12T10:18:41Z

modules/core/keeper/msg_server.go

+	return &channeltypes.MsgChannelUpgradeTryResponse{
+		Result:    channeltypes.SUCCESS,
+		ChannelId: msg.ChannelId,
+		Upgrade:   upgrade,


It looks like this will return an upgrade with the passed-in version. Doesn't it make sense to return with the version set by callback?

aha, yes indeed! nice catch

Agreed. Upgrade set here should have updated version

Two potential ways of handling this:

pass Upgrade to the WriteUpgradeTryChannel method as a reference, pointer type.

Return the Upgrade or even possibly the entire response from the WriteUpgradeTryChannel method.

do you guys favour either?

We could also go for this returning the upgrade and the sequence for the response

func (k Keeper) WriteUpgradeTryChannel(...) (Upgrade, uint64)

You could return the entire response I think

Yeah, opted for going with returning (Channel, Upgrade) for now, opened #3825

AdityaSripal · 2023-06-12T10:19:12Z

modules/core/keeper/msg_server.go

+	return &channeltypes.MsgChannelUpgradeTryResponse{
+		Result:    channeltypes.SUCCESS,
+		ChannelId: msg.ChannelId,
+		Upgrade:   upgrade,
+	}, nil


Im ok with removing that from response

colin-axner

Ay! Looks super clean! I left several suggestions, but happy to push any you agree with into issues and handle them in followups

modules/core/keeper/msg_server.go

colin-axner · 2023-06-12T11:10:04Z

modules/core/keeper/msg_server.go

+	return &channeltypes.MsgChannelUpgradeTryResponse{
+		Result:    channeltypes.SUCCESS,
+		ChannelId: msg.ChannelId,
+		Upgrade:   upgrade,


Agreed. Upgrade set here should have updated version

colin-axner · 2023-06-12T11:12:43Z

modules/core/keeper/msg_server.go

+	return &channeltypes.MsgChannelUpgradeTryResponse{
+		Result:    channeltypes.SUCCESS,
+		ChannelId: msg.ChannelId,
+		Upgrade:   upgrade,
+	}, nil


I agree with not doing the lookup here, but maybe we could abstract into a function:

return formatUpgradeTrySuccessResponse()

or something. No strong preference, happy to remove now and add in the future upon request

modules/core/keeper/msg_server_test.go

chatton

Nice job this looks great! There were just a few small nits but I don't think anything should block this going forward. Main thing to change that's been pointed out is making sure the correct version is returned in the response 👍

modules/core/keeper/msg_server.go

chatton · 2023-06-12T11:38:58Z

modules/core/keeper/msg_server_test.go

+	cases := []struct {
+		name      string
+		malleate  func()
+		expResult func(res *channeltypes.MsgChannelUpgradeTryResponse, err error)


[nit] usually I would imagine expResult is the expected result, but this is an assertion function. Maybe assertionFn?

that's a fair point, I think we have this test setup in some extra places in the codebase using expResult, so maybe we can do one swoop to change all if people feel its worth it.

(I know we have inconsistencies with expPass, expError and expResult in different places - maybe we can keep some housekeeping work for a rainy day (?))

assertionFn sounds nice to me. Agree it should be done in a single swoop

…re not committed. use GetTimeoutHeight helper on path.Endpoint

…otocol

colin-axner

Followup ACK

…msg-server

chatton

Followup ACK 👍

colin-axner and others added 12 commits June 7, 2023 14:16

refactor: add rest of the handler code

ea76c13

Merge remote-tracking branch 'origin/04-channel-upgrades' into colin/…

c0d923f

…3739-upgrade-try-handler

fix: update upgrade tests

a5d997e

fix: test fixes for upgradeTry

d9a8999

fix: upgrade tests

fe0095d

imp: add error code checking to tests

fa6598b

Merge branch '04-channel-upgrades' into colin/3739-upgrade-try-handler

0505850

chore: add todo

75b52f1

Merge branch 'colin/3739-upgrade-try-handler' of github.com:cosmos/ib…

ec3bb0c

…c-go into colin/3739-upgrade-try-handler

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

89a8ce8

adding msg server scaffolding for MsgChannelUpgradeTry

e1a7d62

updating logging and comments

a3a2ac5

damiannolan commented Jun 7, 2023

View reviewed changes

modules/core/keeper/msg_server.go Outdated Show resolved Hide resolved

damiannolan commented Jun 7, 2023

View reviewed changes

damiannolan commented Jun 8, 2023

View reviewed changes

modules/core/keeper/msg_server.go Outdated Show resolved Hide resolved

Base automatically changed from colin/3739-upgrade-try-handler to 04-channel-upgrades June 8, 2023 09:37

colin-axner and others added 6 commits June 8, 2023 16:46

Merge remote-tracking branch 'origin/04-channel-upgrades' into damian…

7e0ced8

…/3740-chan-upgrade-try-msg-server

Merge branch '04-channel-upgrades' into damian/3740-chan-upgrade-try-…

1a326a5

…msg-server

removing flushStatus arg from WriteUpgradeTryChannel

b732ce3

move upgradeVersion refreshing to WriteUpgradeTryChannel, tidy code s…

4462bd7

…tructure

adding initial testcases for upgrade try msg server

98ce964

adding testing app overrides for upgrade handshake app callback handlers

912b8da

damiannolan mentioned this pull request Jun 11, 2023

chore: add upgrade callback overrides for mock ibc app #3812

Merged

9 tasks

adding app callback testcases and updating assertions

b3042e0

damiannolan commented Jun 11, 2023

View reviewed changes

damiannolan changed the title ~~[wip] scaffold msg server for chanUpgradeTry~~ chore: implement msg server for ChannelUpgradeTry Jun 11, 2023

Merge branch '04-channel-upgrades' into damian/3740-chan-upgrade-try-…

ab18e7e

…msg-server

damiannolan commented Jun 11, 2023

View reviewed changes

damiannolan marked this pull request as ready for review June 11, 2023 12:04

damiannolan requested review from colin-axner, AdityaSripal, crodriguezvega, charleenfei and chatton as code owners June 11, 2023 12:04

damiannolan requested a review from DimitrisJim June 11, 2023 12:04

DimitrisJim reviewed Jun 11, 2023

View reviewed changes

AdityaSripal reviewed Jun 12, 2023

View reviewed changes

colin-axner approved these changes Jun 12, 2023

View reviewed changes

chatton approved these changes Jun 12, 2023

View reviewed changes

damiannolan added 3 commits June 12, 2023 14:16

updating tests to include application state changes and assert they a…

acc6b97

…re not committed. use GetTimeoutHeight helper on path.Endpoint

adding extra context to inline comment for upgrade cancellation subpr…

d6e358c

…otocol

updating WriteUpgradetryChannel godoc

bb422e4

colin-axner mentioned this pull request Jun 12, 2023

Consider adding IsUpgradeError func for readability #3821

Closed

3 tasks

add return args to WriteUpgradeTryChannel to fulfil response args

aaee7a7

This was referenced Jun 12, 2023

Consider wrapping ChanUpgradeTry call in cache ctx #3823

Closed

Move handleAbort to msg server layer #3824

Closed

damiannolan mentioned this pull request Jun 12, 2023

Response formatting for channel upgrades msg server handlers #3825

Closed

3 tasks

colin-axner approved these changes Jun 12, 2023

View reviewed changes

Merge branch '04-channel-upgrades' into damian/3740-chan-upgrade-try-…

809ff19

…msg-server

chatton approved these changes Jun 12, 2023

View reviewed changes

damiannolan merged commit 9718a71 into 04-channel-upgrades Jun 12, 2023

damiannolan deleted the damian/3740-chan-upgrade-try-msg-server branch June 12, 2023 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: implement msg server for `ChannelUpgradeTry` #3791

chore: implement msg server for `ChannelUpgradeTry` #3791

damiannolan commented Jun 7, 2023 •

edited

Loading

damiannolan Jun 7, 2023

damiannolan Jun 11, 2023

damiannolan Jun 11, 2023

DimitrisJim Jun 11, 2023

damiannolan Jun 11, 2023

damiannolan Jun 11, 2023

AdityaSripal Jun 12, 2023

colin-axner Jun 12, 2023

DimitrisJim Jun 11, 2023

AdityaSripal Jun 12, 2023

damiannolan Jun 12, 2023

colin-axner Jun 12, 2023

damiannolan Jun 12, 2023

damiannolan Jun 12, 2023

AdityaSripal Jun 12, 2023

damiannolan Jun 12, 2023

AdityaSripal Jun 12, 2023

colin-axner left a comment

colin-axner Jun 12, 2023

colin-axner Jun 12, 2023

chatton left a comment

chatton Jun 12, 2023

damiannolan Jun 12, 2023

colin-axner Jun 12, 2023

colin-axner left a comment

chatton left a comment

		// TODO: determine flush status
		// channel.FlushStatus = flushStatus

chore: implement msg server for ChannelUpgradeTry #3791

chore: implement msg server for ChannelUpgradeTry #3791

Conversation

damiannolan commented Jun 7, 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

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

colin-axner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chatton 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

colin-axner left a comment

Choose a reason for hiding this comment

chatton left a comment

Choose a reason for hiding this comment

chore: implement msg server for `ChannelUpgradeTry` #3791

chore: implement msg server for `ChannelUpgradeTry` #3791

damiannolan commented Jun 7, 2023 •

edited

Loading