ICS2: 02 client refactor#813
Conversation
AdityaSripal
requested review from
colin-axner,
cwgoes and
mpoke
as code owners
AdityaSripal
left a comment
AdityaSripal
left a comment
There was a problem hiding this comment.
This will probably be easier to review in split mode
| Client types MUST define a method on the client state to fetch the timestamp at a given height | ||
|
|
||
| ```typescript | ||
| type getTimestampAtHeight = ( |
There was a problem hiding this comment.
Why is getTimestampAtHeight necessary, given that we already have getTimestamp defined in the previous section (i.e., ConsensusState)?
There was a problem hiding this comment.
cc: @colin-axner , this was necessary to accomodate the special case of solomachines
There was a problem hiding this comment.
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)
Co-authored-by: Marius Poke <marius.poke@posteo.de>
mpoke
left a comment
mpoke
left a comment
There was a problem hiding this comment.
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)).
|
Should "State verification" and "Query interface" sections be subsections of the "Data Structures" section? |
colin-axner
left a comment
colin-axner
left a comment
There was a problem hiding this comment.
LGTM. Thanks for updating
3 participants
Closes: #773
Closes: #689
Closes: #693
Closes: #694