imp: add defensive check on WriteErrorReceipt in addition to more in-line docs on edge cases #5734

colin-axner · 2024-01-25T15:58:59Z

Description

closes: #XXXX

Commit Message / Changelog Entry

type: commit message

see the guidelines for commit messages. (view raw markdown for examples)

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 the 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.

colin-axner · 2024-01-25T16:02:48Z

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

+// hasCounterpartyUpgrade returns true if a counterparty upgrade exists in store
+func (k Keeper) hasCounterpartyUpgrade(ctx sdk.Context, portID, channelID string) bool {


copied over same func @AdityaSripal used for HasUpgrade but chose to leave the hasCounterpartyUpgrade private since I don't see a reason to add it to the public API when it is used only for defensive checks in edge case scenarios (in general I think our get/set/has funcs should be private)

codecov-commenter · 2024-01-25T16:07:35Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (310a4bd) 81.49% compared to head (dd0751a) 81.52%.
Report is 5 commits behind head on main.

❗ Current head dd0751a differs from pull request most recent head ef5faa5. Consider uploading reports for the commit ef5faa5 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5734      +/-   ##
==========================================
+ Coverage   81.49%   81.52%   +0.03%     
==========================================
  Files         199      199              
  Lines       15203    15229      +26     
==========================================
+ Hits        12389    12415      +26     
  Misses       2348     2348              
  Partials      466      466

Files	Coverage Δ
modules/core/04-channel/keeper/keeper.go	`90.90% <100.00%> (+0.13%)`	⬆️
modules/core/04-channel/keeper/upgrade.go	`92.53% <100.00%> (+0.22%)`	⬆️

DimitrisJim

🙏

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

…ne-docs-chanupgrade

colin-axner · 2024-01-29T11:40:04Z

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

@@ -55,6 +55,7 @@ func (k Keeper) WriteUpgradeInitChannel(ctx sdk.Context, portID, channelID strin

 	if k.hasUpgrade(ctx, portID, channelID) {
 		// invalidating previous upgrade
+		k.deleteUpgradeInfo(ctx, portID, channelID)


Discussed with @damiannolan. We decided to add an explicit deletion of the upgrade info when invalidating a previous attempt. Functionally this does not change the code as it stands right now because:

we overwrite the upgrade shortly after

no counterparty upgrade exists

but we need this change in order to keep the defensive check in WriteErrorReceipt and it prevents an accidental mistake of forgetting to delete information if more info is added to the upgrade process. So we change deletion of previous upgrade info from implicit to explicit at the gas of some extra reads/write but it'll make it harder for developers to make mistakes when changing any logic in channel upgradability

no counterparty upgrade exists because we can only meaningfully run init (and by extent, its write) before we enter flushing/flushcomplete, correct?

correct, since we assert the channel.State is OPEN

This looks good to me, but I am just wondering that if we do deleteUpgradeInfo + WriteErrorReceipt now, would it be possible to basically call abortUpgrade?

think you could @crodriguezvega but would definitely incur more gas in addition to not semantically fitting an abort which is used when channel state is either flushing/flush-complete. (maybe a refactoring could eventually be made tho)

Yea, it would do extra writes to set the channel state and upgrade sequence (to the existing values). Leaving as is

damiannolan

The final PR 🤞🏻 LFG!!!!!! 🚀

Thank you @colin-axner, amazing doc comments to save our future selves from many head scratchers ❤️

crodriguezvega

Last PR before release! Exciting!

I left just a nit question.

crodriguezvega · 2024-01-29T13:03:07Z

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

@@ -55,6 +55,7 @@ func (k Keeper) WriteUpgradeInitChannel(ctx sdk.Context, portID, channelID strin

 	if k.hasUpgrade(ctx, portID, channelID) {
 		// invalidating previous upgrade
+		k.deleteUpgradeInfo(ctx, portID, channelID)


This looks good to me, but I am just wondering that if we do deleteUpgradeInfo + WriteErrorReceipt now, would it be possible to basically call abortUpgrade?

…line docs on edge cases (#5734) * imp: add defensive check + extra documentation for edge case scenarios * imp: gas optimization * self nit * imp: HasUpgrade -> hasUpgrade * fix: add explicit upgrade deletion in init --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit 4727e2c)

…line docs on edge cases (#5734) (#5765) * imp: add defensive check + extra documentation for edge case scenarios * imp: gas optimization * self nit * imp: HasUpgrade -> hasUpgrade * fix: add explicit upgrade deletion in init --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> (cherry picked from commit 4727e2c) Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

imp: add defensive check + extra documentation for edge case scenarios

d842b81

colin-axner added priority PRs that need prompt reviews channel-upgradability Channel upgradability feature backport-to-v8.1.x labels Jan 25, 2024

colin-axner added 2 commits January 25, 2024 17:00

imp: gas optimization

d685e40

self nit

a3b133a

colin-axner commented Jan 25, 2024

View reviewed changes

colin-axner marked this pull request as ready for review January 25, 2024 16:04

colin-axner requested review from AdityaSripal and damiannolan as code owners January 25, 2024 16:04

colin-axner requested review from chatton, charleenfei and DimitrisJim January 25, 2024 16:12

colin-axner added this to the 04-channel upgrades RC milestone Jan 25, 2024

colin-axner added 3 commits January 25, 2024 17:58

imp: HasUpgrade -> hasUpgrade

abac302

Merge branch 'main' into colin/audit-inline-docs-chanupgrade

dd0751a

Merge branch 'main' into colin/audit-inline-docs-chanupgrade

ef5faa5

DimitrisJim approved these changes Jan 25, 2024

View reviewed changes

crodriguezvega approved these changes Jan 26, 2024

View reviewed changes

crodriguezvega added 2 commits January 26, 2024 13:09

Merge branch 'main' into colin/audit-inline-docs-chanupgrade

68a879d

Merge branch 'main' into colin/audit-inline-docs-chanupgrade

a4f74d4

crodriguezvega reviewed Jan 26, 2024

View reviewed changes

modules/core/04-channel/keeper/upgrade.go Show resolved Hide resolved

crodriguezvega self-requested a review January 26, 2024 14:57

colin-axner added 2 commits January 29, 2024 12:07

Merge branch 'main' of github.com:cosmos/ibc-go into colin/audit-inli…

2f7d87b

…ne-docs-chanupgrade

fix: add explicit upgrade deletion in init

a745025

colin-axner commented Jan 29, 2024

View reviewed changes

damiannolan approved these changes Jan 29, 2024

View reviewed changes

crodriguezvega approved these changes Jan 29, 2024

View reviewed changes

colin-axner merged commit 4727e2c into main Jan 29, 2024
63 checks passed

colin-axner deleted the colin/audit-inline-docs-chanupgrade branch January 29, 2024 13:49

mergify bot mentioned this pull request Jan 29, 2024

imp: add defensive check on WriteErrorReceipt in addition to more in-line docs on edge cases (backport #5734) #5765

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

imp: add defensive check on WriteErrorReceipt in addition to more in-line docs on edge cases #5734

imp: add defensive check on WriteErrorReceipt in addition to more in-line docs on edge cases #5734

colin-axner commented Jan 25, 2024

colin-axner Jan 25, 2024 •

edited

Loading

codecov-commenter commented Jan 25, 2024 •

edited

Loading

DimitrisJim left a comment •

edited

Loading

colin-axner Jan 29, 2024

DimitrisJim Jan 29, 2024

colin-axner Jan 29, 2024

crodriguezvega Jan 29, 2024

DimitrisJim Jan 29, 2024 •

edited

Loading

colin-axner Jan 29, 2024

damiannolan left a comment

crodriguezvega left a comment

crodriguezvega Jan 29, 2024

		// hasCounterpartyUpgrade returns true if a counterparty upgrade exists in store
		func (k Keeper) hasCounterpartyUpgrade(ctx sdk.Context, portID, channelID string) bool {

imp: add defensive check on WriteErrorReceipt in addition to more in-line docs on edge cases #5734

imp: add defensive check on WriteErrorReceipt in addition to more in-line docs on edge cases #5734

Conversation

colin-axner commented Jan 25, 2024

Description

Commit Message / Changelog Entry

colin-axner Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Jan 25, 2024 • edited Loading

Codecov Report

DimitrisJim left a comment • edited Loading

Choose a reason for hiding this comment

colin-axner Jan 29, 2024

Choose a reason for hiding this comment

DimitrisJim Jan 29, 2024

Choose a reason for hiding this comment

colin-axner Jan 29, 2024

Choose a reason for hiding this comment

crodriguezvega Jan 29, 2024

Choose a reason for hiding this comment

DimitrisJim Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

colin-axner Jan 29, 2024

Choose a reason for hiding this comment

damiannolan left a comment

Choose a reason for hiding this comment

crodriguezvega left a comment

Choose a reason for hiding this comment

crodriguezvega Jan 29, 2024

Choose a reason for hiding this comment

colin-axner Jan 25, 2024 •

edited

Loading

codecov-commenter commented Jan 25, 2024 •

edited

Loading

DimitrisJim left a comment •

edited

Loading

DimitrisJim Jan 29, 2024 •

edited

Loading