Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add ChanUpgradeCancel core handler #3846
Add ChanUpgradeCancel core handler #3846
Changes from 33 commits
5cd004e
cbc8e32
d01355f
6f7940b
6d50848
795cd05
dfbc5d4
b2a99ca
d429514
4330c51
c1e05b8
619381b
91605c1
d050ee8
eaa2b91
d6bd3bf
cfe59ae
7d2c3d4
099b147
759474f
3916332
700acba
6b2ce2e
811c418
0f0305f
d6bd69c
51e9070
3e0bf6e
4e12bc0
df123ba
098d0a3
853eae2
8ced59b
bf10856
0e34c52
8405547
2f540ce
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 this should say:
And maybe we should add the sequence values?
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.
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-axnerThere 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.
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.
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.
As discussed today, I think we can remove the +1 in a followup pr since channel upgrade initialization should bump the upgrade sequence
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.
unrelated quick fix to make it consistent with the other write functions.
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 am not sure this line is necessary bc the
UpdateClient
function below commits a block to chainB alreadyThere 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.
very good catch, this makes total sense to me, but the test fails when this commit block is removed.
I don't understand this behaviour
do you or maybe @colin-axner understand why this would be the case ?
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.
context: #3900
When
CommitBlock
is called, it commits the current header set within the test chain. This is created on a call toNextBlock
. The state changes are not being reflected to theAppHash
being committed. So whenCommitBlock
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 sdkThere 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.
The testing pkg has some cleanup in its future. I'd like to simplify the API a bit (there shouldn't be
CommitBlock
andNextBlock
)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.
nit
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.
can be removed
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.
slick
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.
@damiannolan gets the credit for this one ❤️
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.
same nit as above
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.
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.
are we using this anywhere?
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.
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.
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.
Is this error actually used?
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.
it used to be!