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
Enforce ProofSpecs in 23-Commitment #6374
Enforce ProofSpecs in 23-Commitment #6374
Changes from 15 commits
3d2795c
a716e20
3a5bc6a
1723648
e0d574d
74dd904
e2f3537
57e0a84
5a9782d
6d759cf
8e8bfd8
761fa7b
4bb1e56
bd71a76
99c8700
10c978a
3e5b687
f170cf9
f414ad8
236ab96
92e8315
f441fb1
3731d8a
3241a2d
f6749b5
0737e09
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.
Do we want there to be a way for the client creator to provide the full
ics23.ProofSpec
?If we don't have that, then we're limited to whatever specs the SDK has hard-coded "by name".
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.
Hmm.. for now this is not that useful. Since we're still using
merkle.Proof
andProofRuntime
to verify proofs, we cannot actually verify a proof that isn't registered and hard-coded into the proofruntime anyway.Think this is something to revisit once we pull out
ProofRuntime
from ics23 verificationThere 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.
Ahh but I suppose we can change the interfaces now, so we don't have to change them again
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.
Hmm after thinking about it, realized there isn't a simple way to do this without completely pulling out
merkle.ProofRuntime
from the `23-commitment package.I can do this if there's consensus that this is preferable, but it does involve more work since I have to manually verify the proof with passed in specs, rather than just calling into
ProofRuntime
. Not too hard thoThere 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 is probably worthwhile, since it will allow SDK chains to support new
ProofSpec
s without manual upgradesThere 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.
Ok, so now 23-commitment no longer uses
ProofRuntime
. Instead we use the ics23 Verify functions directly with the ProofSpecs passed in from the client.Now, ibc can verify Commitment proofs even if the chain does not have them hardcoded into its proof runtime