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
02-client routing: implement
VerifyUpgradeAndUpdateState
in light client modules #582702-client routing: implement
VerifyUpgradeAndUpdateState
in light client modules #5827Changes from 22 commits
98230b3
83ce8cc
fb77005
a4e8693
79fb56f
623d8c6
3e31163
be1dfdc
96a133f
0758398
e92393b
4dfcf27
70a1182
1bad03b
fbe8b92
a9d6113
bcc8273
1408152
5937285
cfa5eec
a65ab8b
290a237
f7b2192
3b3256a
6113b5b
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 we will want to add this back once
LatestHeight
is added to the light client moduleThere 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.
discussed async, this check must be moved to the light client module, and this functionality should likely only exist in the tendermint client module.
At first glance, we do not have the
clientID
of theupgradedClientState
. However, it should be the sameclientID
as theclientState
(target). However, we need the latest height from theupgradedClientState
concrete implementation here - this is the client which the counterparty stores in it upgrade path and we prove against within the tm client module.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 deleted this test because this is tested in the light client module.
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.
for some reason it doesnt look like this change is triggering the error code path we would expect, does anyone else see that in the testing code coverage?
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 follow up this post merge, maybe? I think all these tests should be using
expError
and notexpPass
(bool)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 moved this test to the light client module, but can move it back here in the PR where
LatestHeight
is added to the light client module and then we can move the check back to 02-client.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.
#5827 (comment)
I guess it can just be removed entirely now?
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.
Sounds good. At the moment this check is performed in 07-tendermint and 08-wasm light client modules. Based on your comment, is that ok? Any reason why the check should not be done in 08-wasm?
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 can have these tests in the lcm modules for 07-tendermint and 08-wasm already, i think! is that fine @crodriguezvega?
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.
Yes, correct! The check and the tests are currently in the light client module. Happy to leave it there if that's the right place. :)