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.

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

ICS2: 02 client refactor #813

Merged
merged 10 commits into from
Aug 31, 2022
Merged

ICS2: 02 client refactor #813

merged 10 commits into from
Aug 31, 2022

Conversation

AdityaSripal
Copy link
Member

@AdityaSripal AdityaSripal commented Aug 4, 2022

Closes: #773
Closes: #689
Closes: #693
Closes: #694

Copy link
Member Author

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

This will probably be easier to review in split mode

spec/core/ics-002-client-semantics/README.md Show resolved Hide resolved
Client types MUST define a method on the client state to fetch the timestamp at a given height

```typescript
type getTimestampAtHeight = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is getTimestampAtHeight necessary, given that we already have getTimestamp defined in the previous section (i.e., ConsensusState)?

Copy link
Member Author

Choose a reason for hiding this comment

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

cc: @colin-axner , this was necessary to accomodate the special case of solomachines

Copy link
Contributor

Choose a reason for hiding this comment

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

Clients which only have a latest consensus state would only have a latest timestamp and thus might not have a timestamp based on a consensus state. The current spec didn't work for solo machines as solo machines don't store consensus states (the consensus state is kept within the client state)

spec/core/ics-002-client-semantics/README.md Outdated Show resolved Hide resolved
spec/core/ics-002-client-semantics/README.md Show resolved Hide resolved
spec/core/ics-002-client-semantics/README.md 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 Show resolved Hide resolved
spec/core/ics-002-client-semantics/README.md Show resolved Hide resolved
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.

Looks good. See my comments below. Also, there are three comments re. submitMisbehaviourToClient that were not addressed yet (i.e., #813 (comment), #813 (comment), #813 (comment)).

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
@mpoke
Copy link
Contributor

mpoke commented Aug 18, 2022

Should "State verification" and "Query interface" sections be subsections of the "Data Structures" section?

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.

Great work @AdityaSripal

@AdityaSripal AdityaSripal merged commit f9ac787 into main Aug 31, 2022
@AdityaSripal AdityaSripal deleted the 02-client-refactor branch August 31, 2022 10:49
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for updating

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
3 participants