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

ICS23: Generic Merkle types #5082

Closed
mossid opened this issue Sep 20, 2019 · 8 comments
Closed

ICS23: Generic Merkle types #5082

mossid opened this issue Sep 20, 2019 · 8 comments
Assignees

Comments

@mossid
Copy link
Contributor

mossid commented Sep 20, 2019

The commitment proofs are defined as opaque objects dependent on the concrete types. For Merkle commitment proofs, the implementation is currently using tendermint/crypto/merkle, but it should use ICS23 implementation instead.

@mossid
Copy link
Contributor Author

mossid commented Oct 1, 2019

https://github.com/confio/ics23-tendermint provides an easy conversion method, can/should be used

To reduce onchain overhead all of the conversion should happen offchain side; the relayer retrieves crypto/merkle format from the chain(simply using CLIContext), convert it into ICS23 format, submit it into the other chain.

@mossid mossid mentioned this issue Oct 1, 2019
5 tasks
@alexanderbez alexanderbez added this to the IBC Implementation & Integration milestone Oct 1, 2019
@fedekunze
Copy link
Collaborator

fedekunze commented Oct 2, 2019

@mossid are you saying that we should remove the 23-commitment package in favor of https://github.com/confio/ics23/tree/master/go instead?

@AdityaSripal
Copy link
Member

AdityaSripal commented Nov 14, 2019

Should the confio package just be copied over wholesale? Or should I wrap the confio package with the interfaces we currently have in 23-commitment such that 23 no longer relies on crypto/merkle.Proof.

I also notice there are changes in the confio implementation that I'm not sure if it can be accepted as is, or have to be wrapped to conform to the previous interfaces.

  1. confio uses a ProofSpec in the VerifyMembership functions, does this break expected interface in a way that needs to be hidden. For example, the proof Spec can be part of the struct implementing proof interface to maintain old interface.

  2. There is no ApplyPrefix function with the ExistenceProof as far as i can tell, is this no longer necessary with the proofs in confio? Should i implement an ApplyPrefix function as a no-op or remove it entirely?

@fedekunze

@cwgoes
Copy link
Contributor

cwgoes commented Nov 14, 2019

ProofSpec will become part of either the client type or the connection version in IBC.

(both are possible, the latter is necessary only if the ProofSpec would vary across connections to the same chain using the same client - I believe ApplyPrefix is subsumed by ProofSpec, which can contain prefix information, so if we want that flexibility ProofSpec should go in the connection version)

confio/ics23 implements the ICS 23 interface pretty directly, I think I would recommend trying to fuse it and the current 23-commitment interface - we don't want to rely on crypto/merkle.Proof at all (in the 1.0.0 production IBC version).

cc @mossid - Do you agree with the above?

@fedekunze
Copy link
Collaborator

confio/ics23 implements the ICS 23 interface pretty directly, I think I would recommend trying to fuse it and the current 23-commitment interface - we don't want to rely on crypto/merkle.Proof at all (in the 1.0.0 production IBC version).

++

@ethanfrey thoughts on 1. and 2. above?

@alexanderbez alexanderbez modified the milestones: IBC Implementation & Integration, v0.38.0 Nov 30, 2019
@fedekunze fedekunze modified the milestones: IBC Implementation & Integration, IBC Implementation Dec 10, 2019
@fedekunze fedekunze mentioned this issue Dec 10, 2019
6 tasks
@cwgoes
Copy link
Contributor

cwgoes commented Jan 15, 2020

Blocked on ICS 8.

@cwgoes
Copy link
Contributor

cwgoes commented Mar 17, 2020

This is still blocked on spec work & cosmos/ics23#28.

@cwgoes
Copy link
Contributor

cwgoes commented Jun 16, 2020

Done.

@cwgoes cwgoes closed this as completed Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants