Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ICS2: Generic Verification Methods #687

Merged
merged 2 commits into from
Mar 17, 2022
Merged

ICS2: Generic Verification Methods #687

merged 2 commits into from
Mar 17, 2022

Conversation

AdityaSripal
Copy link
Member

Closes: #684

mpoke
mpoke approved these changes Mar 16, 2022
View reviewed changes
Copy link
Contributor

@mpoke mpoke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Some minor comments bellow. The only issue that I see is that the two functions are also defined in Implementation strategies.

spec/core/ics-002-client-semantics/README.md Outdated Show resolved Hide resolved
spec/core/ics-002-client-semantics/README.md Outdated Show resolved Hide resolved
spec/core/ics-002-client-semantics/README.md Outdated Show resolved Hide resolved
spec/core/ics-002-client-semantics/README.md Outdated Show resolved Hide resolved
spec/core/ics-002-client-semantics/README.md Outdated
@@ -429,26 +429,40 @@ type verifyNextSequenceRecv = (
=> boolean
```

#### Optional Functions
`verifyMembership` is a generic proof verification method which verifies a proof of the existence a value at a given `CommitmentPath` at the specified height.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The names of these functions, i.e., verifyMembership and verifyNonMembership, are already used in ICS2 (see the Implementation strategies section). Also, both names are used in ICS23, but I guess that's okay.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarified that the example in this section is the ICS-23 methods

@AdityaSripal @mpoke
Apply suggestions from code review
34be3ab 
Co-authored-by: Marius Poke <marius.poke@posteo.de>
@AdityaSripal AdityaSripal merged commit 30ad8c1 into master Mar 17, 2022
@AdityaSripal AdityaSripal deleted the aditya/generic-ics2-verify branch March 17, 2022 13:28
@damiannolan damiannolan mentioned this pull request Mar 21, 2022
Closed
7 tasks
@AdityaSripal AdityaSripal mentioned this pull request Apr 25, 2022
Closed
@bluele bluele mentioned this pull request Mar 24, 2023
Closed
@crodriguezvega crodriguezvega mentioned this pull request Jun 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpoke mpoke mpoke approved these changes

@cwgoes cwgoes Awaiting requested review from cwgoes

@colin-axner colin-axner Awaiting requested review from colin-axner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ICS2: Replace specific VerifyFunctions with generic VerifyMembership and VerifyNonMembership
2 participants
@AdityaSripal @mpoke