-
Notifications
You must be signed in to change notification settings - Fork 586
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.
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
ICS4: Write ErrorReceipt for previous upgrade on Reinitialization #5732
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5732 +/- ##
=======================================
Coverage 81.49% 81.49%
=======================================
Files 199 199
Lines 15203 15208 +5
=======================================
+ Hits 12389 12394 +5
Misses 2348 2348
Partials 466 466
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Nice tests :)
Left one suggestion for a HasUpgrade
function (which I don't think we have in keeper atm) which would save on some gas and look a little bit cleaner.
_, priorUpgradeExists := k.GetUpgrade(ctx, portID, channelID) | ||
if priorUpgradeExists { | ||
// invalidating previous upgrade | ||
k.WriteErrorReceipt(ctx, portID, channelID, types.NewUpgradeError(channel.UpgradeSequence, types.ErrInvalidUpgrade)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can add HasUpgrade
to keeper to save on minimal gas
if k.HasUpgrade(ctx, portId, channelID) {
k.WriteErrorReceipt(ctx, portID, channelID, upgradeErr)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful! 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!! LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perfect! left one comment re API
* fix reinitialization * write error receipt on reinitialization * gofumpt * switch to Has instead of Get * imp: HasUpgrade -> hasUpgrade --------- Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> (cherry picked from commit 574a639)
…) (#5743) * fix reinitialization * write error receipt on reinitialization * gofumpt * switch to Has instead of Get * imp: HasUpgrade -> hasUpgrade --------- Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> (cherry picked from commit 574a639) Co-authored-by: Aditya <adityasripal@gmail.com>
Description
Writes error receipt for previous upgrade attempt if we initialize a new one. This allows the counterparty to cancel their upgrade and retry on the latest attempt
closes: #XXXX
Commit Message / Changelog Entry
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.
docs/
) or specification (x/<module>/spec/
).godoc
comments.Files changed
in the Github PR explorer.Codecov Report
in the comment section below once CI passes.