Update paths for consistency; revise ICS 2 for client type sub-specs #348

cwgoes · 2020-01-09T22:46:48Z

Closes #345
Closes #346

fedekunze · 2020-01-10T20:17:47Z

@cwgoes any chance we could switch the channel paths from "ports/{portID}/channels/{channelID}" to "channels/{channelID}/ports/{portID}"

cwgoes · 2020-01-10T20:36:42Z

@cwgoes any chance we could switch the channel paths from "ports/{portID}/channels/{channelID}" to "channels/{channelID}/ports/{portID}"

Hmm, we could, doesn't it seem a bit confusing though (since channel identifiers are unique "per-port")? What query do you want to run that requires contiguous keys?

cwgoes · 2020-01-13T15:17:10Z

For consensus state heights, we need to pick a height at which to check that the counterparty chain has the correct consensus state. I think we should say that this must be the height at which we started the handshake, so that means we'll need to keep it in state (temporarily). That seems safest though.

cwgoes · 2020-01-13T15:45:04Z

I wonder if client types should also define an initialise function, we want to avoid paths in ICS 2.

cwgoes · 2020-01-13T15:47:27Z

Also, the spec should define what a loopback client implementation looks like.

cwgoes · 2020-01-15T14:45:59Z

Also, the spec should define what a loopback client implementation looks like.

Split to #352.

cwgoes · 2020-01-15T15:36:20Z

How to correctly sub-space with non-uniformly-prefixed keys?

@fedekunze Do you know if the SDK has a substore wrapper which can limit read/write to specific keys (instead of just a prefix)? We want to limit client type instances to reading/writing only keys belonging to them, but we altered the layout like you mentioned so that keys for a given client don't have a uniform prefix (this makes for easier iteration, I suppose).

cwgoes · 2020-01-16T14:23:10Z

Fix ICS 4, thanks Fede.

cwgoes · 2020-01-16T14:55:43Z

@cwgoes any chance we could switch the channel paths from "ports/{portID}/channels/{channelID}" to "channels/{channelID}/ports/{portID}"

Hmm, we could, doesn't it seem a bit confusing though (since channel identifiers are unique "per-port")? What query do you want to run that requires contiguous keys?

Confirmed per discussion that we don't need this.

cwgoes · 2020-01-17T14:06:41Z

For now I am going to update the paths so that everything is prefixed under clients/{identifier} - this means that we can use the prefix store for permissioning. IMO that is worth possibly having to skip items while iterating, since that only ought to be happening out of transaction processing.

cwgoes · 2020-01-17T14:35:35Z

This PR really makes me want a type-checked spec...

fedekunze · 2020-01-17T15:02:13Z

spec/ics-024-host-requirements/README.md

 | provableStore  | "clients/{identifier}/type"                                                    | ClientType        | [ICS 2](../ics-002-client-semantics) |
+| privateStore   | "clients/{identifier}"                                                         | ClientState       | [ICS 2](../ics-007-tendermint-client) |
+| provableStore  | "clients/{identifier}/consensusStates/{height}"                                | ConsensusState    | [ICS 7](../ics-007-tendermint-client) |
+| privateStore   | "clients/{identifier}/connections                                              | []Identifier      | [ICS 3](../ics-003-connection-semantics) |


what is this path for?

It's used for reverse lookup of what connections are using a client.

spec/ics-002-client-semantics/README.md

fedekunze · 2020-01-20T12:34:18Z

spec/ics-002-client-semantics/README.md

-}
-```
+Client consensus state and client internal state can be queried by identifier, but
+the specific paths which must be queried are defined by each client type.


the specific paths which must be queried are defined by each client type.

why is this now defined per client? All clients need to still be able to query the consensus and client states

The client state, yes, but the consensus state, not necessarily, e.g. for a solo machine there is not a consensus state stored for each height. Client types can be required to expose queryConsensusState I suppose - is this required? The relayer should use a client-type-agnostic method.

The client state, yes, but the consensus state, not necessarily, e.g. for a solo machine there is not a consensus state stored for each height. Client types can be required to expose queryConsensusState I suppose...

👍 It'd be awesome if we could define a standard interface for all clients with the functions that they need to implement.

Aye, agreed. I'll add that.

With regards to a standard interface, I completely agree, but the only parts of that I can reliably guess are those which will be required by relayers (querying clients), so if you want additional functions for convenience or CLI UX, let me know.

I defined a few interfaces for clients in cosmos/cosmos-sdk#5485

Yeah. I think I'll split that out to a separate issue & we can discuss first. #356

fedekunze · 2020-01-20T12:38:50Z

spec/ics-002-client-semantics/README.md

+Client types MUST define a method to fetch the current height (height of the most recent validated header).
+
+```typescript
+type latestHeight = (


this was confusing for me at least in the beginning. The naming of the third-party client consensus state vs the self consensus state was not clear. So whenever it said getConsensusState(height) it meant self consensus state (i.e block header and validator info), while the getConsensusState(clientID, height) meant the counterparty client.

I'd suggest adding the client or self prefix to the ConsensusState to clarify this item.

Suggested change

type latestHeight = (

type latestClientHeight = (

or, alternatively clientState.latestHeight().

Aye, that makes sense, I'll change the client function name to getClientConsensusState.

Actually, I can't find any uses of getConsensusState now which take a clientID and height. Can you point to a specific location which is confusing?

Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

spec/ics-002-client-semantics/README.md

Start updating paths

85f6e64

cwgoes changed the title ~~Start updating paths~~ Update paths for consistency Jan 9, 2020

Revise ICS 2; rebuild

695fdf1

cwgoes changed the title ~~Update paths for consistency~~ Update paths for consistency; revise ICS 2 for client type sub-specs Jan 13, 2020

Alter initalisation logic

b5ca351

cwgoes mentioned this pull request Jan 15, 2020

The one true master IBC implementation tracking issue cosmos/gaia#201

Closed

34 tasks

Merge branch 'master' into cwgoes/fixup-paths

2fd4010

cwgoes added 5 commits January 17, 2020 15:06

Update paths for uniform prefixing

67d7e69

Fix ICS 4; reflect in ICS 2/6/7

db96e9d

Update ICS 24; make TS linter happy

5d94828

Add missing path to ICS 24

19c6190

Add height parameter for consensus state verification

19e3ef2

cwgoes marked this pull request as ready for review January 17, 2020 14:34

cwgoes added tao Transport, authentication, & ordering layer. from-review Feedback / alterations from specification review. ready-for-review Pull requests which are ready for review. labels Jan 17, 2020

cwgoes requested review from mossid, fedekunze and AdityaSripal January 17, 2020 14:34

Fix call locations of verifyClientConsensusState

c6e6eb5

cwgoes added this to the IBC 1.0.0-rc6 milestone Jan 17, 2020

fedekunze reviewed Jan 17, 2020

View reviewed changes

cwgoes requested a review from fedekunze January 20, 2020 10:58

fedekunze reviewed Jan 20, 2020

View reviewed changes

cwgoes and others added 2 commits January 20, 2020 14:31

Update spec/ics-002-client-semantics/README.md

6455a84

Co-Authored-By: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

Rename to "latestClientHeight" everywhere

faee6f9

fedekunze approved these changes Jan 21, 2020

View reviewed changes

spec/ics-002-client-semantics/README.md Show resolved Hide resolved

This was referenced Jan 21, 2020

ICS07 Tendermint Client implementation cosmos/cosmos-sdk#5485

Merged

Standardised query interface for clients #356

Closed

cwgoes added 2 commits January 21, 2020 13:45

Add back return value (thanks @fedekunze)

7519265

Merge branch 'master' into cwgoes/fixup-paths

24a3113

cwgoes merged commit 6754b8e into master Jan 22, 2020

cwgoes deleted the cwgoes/fixup-paths branch January 22, 2020 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update paths for consistency; revise ICS 2 for client type sub-specs #348

Update paths for consistency; revise ICS 2 for client type sub-specs #348

cwgoes commented Jan 9, 2020 •

edited

Loading

fedekunze commented Jan 10, 2020

cwgoes commented Jan 10, 2020

cwgoes commented Jan 13, 2020

cwgoes commented Jan 13, 2020

cwgoes commented Jan 13, 2020

cwgoes commented Jan 15, 2020

cwgoes commented Jan 15, 2020

cwgoes commented Jan 16, 2020 •

edited

Loading

cwgoes commented Jan 16, 2020

cwgoes commented Jan 17, 2020

cwgoes commented Jan 17, 2020

fedekunze Jan 17, 2020

cwgoes Jan 17, 2020

fedekunze Jan 20, 2020

cwgoes Jan 20, 2020

fedekunze Jan 20, 2020

cwgoes Jan 20, 2020

cwgoes Jan 20, 2020

fedekunze Jan 21, 2020

cwgoes Jan 21, 2020

fedekunze Jan 20, 2020

fedekunze Jan 20, 2020

fedekunze Jan 20, 2020

cwgoes Jan 20, 2020

cwgoes Jan 20, 2020

Update paths for consistency; revise ICS 2 for client type sub-specs #348

Update paths for consistency; revise ICS 2 for client type sub-specs #348

Conversation

cwgoes commented Jan 9, 2020 • edited Loading

fedekunze commented Jan 10, 2020

cwgoes commented Jan 10, 2020

cwgoes commented Jan 13, 2020

cwgoes commented Jan 13, 2020

cwgoes commented Jan 13, 2020

cwgoes commented Jan 15, 2020

cwgoes commented Jan 15, 2020

cwgoes commented Jan 16, 2020 • edited Loading

cwgoes commented Jan 16, 2020

cwgoes commented Jan 17, 2020

cwgoes commented Jan 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes commented Jan 9, 2020 •

edited

Loading

cwgoes commented Jan 16, 2020 •

edited

Loading