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

Figure out prefixing in coordination with SDK implementation #387

Closed
cwgoes opened this issue Mar 5, 2020 · 9 comments
Closed

Figure out prefixing in coordination with SDK implementation #387

cwgoes opened this issue Mar 5, 2020 · 9 comments
Assignees
Labels
tao Transport, authentication, & ordering layer.

Comments

@cwgoes
Copy link
Contributor

cwgoes commented Mar 5, 2020

Seems like the SDK implementation adds an extra prefix to paths after the store key but before the ICS24-specified path. Thanks @ancazamfir.

@cwgoes cwgoes added the tao Transport, authentication, & ordering layer. label Mar 5, 2020
@cwgoes cwgoes self-assigned this Mar 5, 2020
@ancazamfir
Copy link
Contributor

I looked at more paths, here is what I think we currently have (ICS vs SDK). I checked some of these with the debugger on some of the packet and handshake tests.

ICS24: clients/{identifier}/type -> ClientType ICS 2
SDK: clientType/{identifier}

ICS24: clients/{identifier}/consensusStates/{height} -> ConsensusState ICS 7
SDK: consensusState/{identifier}/{height}

ICS24: connections/{identifier} -> ConnectionEnd ICS 3
SDK: connection/{identifier}

ICS24: ports/{identifier}/channels/{identifier} -> ChannelEnd ICS 4
should this be ports/{identifier}/channels/{identifier}/channelEnd in the ICS?
SDK: {KeyChannelPrefix}/ports/{identifier}/channels/{identifier}

ICS24: ports/{identifier}/channels/{identifier}/key -> CapabilityKey ICS 4
SDK: {KeyChannelCapabilityPrefix}/ports/channel/{identifier}/key

ICS24: ports/{identifier}/channels/{identifier}/nextSequenceSend -> uint64 ICS 4
SDK: {KeyNextSeqSendPrefix}/ports/{identifier}/channels/{identifier}/nextSequenceSend

ICS24: ports/{identifier}/channels/{identifier}/nextSequenceRecv -> uint64 ICS 4
SDK: {KeyNextSeqRecvPrefix}/ports/{identifier}/channels/{identifier}/nextSequenceRecv

ICS24: ports/{identifier}/channels/{identifier}/packets/{sequence} -> commitment hash bytes ICS 4
SDK: {KeyPacketCommitmentPrefix}/ports/{identifier}/channels/{identifier}/packets/{sequence}

ICS24: ports/{identifier}/channels/{identifier}/acknowledgements/{sequence} -> bytes ICS 4
SDK: {KeyPacketAckPrefix}/ports/{identifier}/channels/{identifier}/acknowledgements/{sequence}

@ancazamfir
Copy link
Contributor

ancazamfir commented Mar 5, 2020

also, from the private store:

ICS24: clients/{identifier} -> ClientState ICS 2
should this be clients/{identifier}/clientState in the ICS?
SDK: clientState/{identifier}

ICS24: clients/{identifier}/connections ->[]connIdentifier ICS 3
SDK: clientConnections/{identifier}

BTW, the nextSequenceSend above is not specified in ICS24

@cwgoes cwgoes added this to the IBC Specification 1.0.0 milestone Mar 18, 2020
@cwgoes
Copy link
Contributor Author

cwgoes commented Mar 18, 2020

@fedekunze or @AdityaSripal - do you recall why we did this in the SDK?

Was it for some ease-of-query reason?

Ideally we would avoid unnecessary prefixes, I think.

@AdityaSripal
Copy link
Member

AdityaSripal commented Mar 27, 2020

Don't know why prefixes were added in the first place, can't see a good reason to have them.
Removed them all.

The only KeyFormat that remains different from spec is clientState, which is stored under:

clients/{identifier}/clientState

Didn't find a good way to conform to spec here since clients/identifier is the prefix for the entire client store. I would prefer spec is changed to have something like the above

EDIT: Figured it out, the prefixes were added to allow for iteration over all channels, all packet commitments, all acknowledgements, etc. efficiently

I took out the prefixes, and don't have an efficient way to iterate over these objects anymore without also iterating over many other objects that we're not trying to fetch.

For example:

With the prefix, iterating over channels is as simple as creating a PrefixIterator(KeyChannelPrefix)

However without the prefixes, there isn't a good way to do this apart from iterating over all objects that has a prefix channelPath, i.e. all channels, all channel capabilities, all sequences, ports, packet commitments, and acknowledgements
and simply skip over everything that isn't a channel by checking the final key we get.

This makes me lean towards keeping the prefixes, at least the ones on Channels, Ports, and Packets.

@cwgoes
Copy link
Contributor Author

cwgoes commented Mar 27, 2020

Alright - thanks for the clarification; prefixes are fine as long as they are consistent - do you want to update the spec, or can you point me to where all of these are defined in the code & I can?

@AdityaSripal
Copy link
Member

They're defined here: https://github.com/cosmos/cosmos-sdk/blob/ibc-alpha/x/ibc/types/keys.go

I can update them but currently busy with routing/dynamic capabilities. Think prefixes for the all the stuff in ICS4 makes a lot of sense. I don't think they should be numbers created by iota though. Since that's dependent on the order they're instantiated in the cosmos-sdk implementation. Think they should be human-understandable string prefixes

@cwgoes
Copy link
Contributor Author

cwgoes commented May 18, 2020

@AdityaSripal @ancazamfir I have updated ICS 24 in d107eb2, does that look accurate to you?

However, I wonder if we should make these paths client-type-specific - there is no particular reason every Tendermint chain would need to use the same paths.

@ancazamfir
Copy link
Contributor

Not sure I understand, would this be for all paths? And something different than store prefix? could you give an example?

@cwgoes
Copy link
Contributor Author

cwgoes commented May 19, 2020

Not sure I understand, would this be for all paths? And something different than store prefix? could you give an example?

Perhaps it is simpler to standardise, but in principle the particular paths used for acknowledgements, packet commitments, etc. are unrelated to the light client validity predicate, so different Tendermint chains could use different paths (in addition to the store prefix being different), yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tao Transport, authentication, & ordering layer.
Projects
Status: Backlog
Development

No branches or pull requests

3 participants