Port over ics23-iavl #276
Port over ics23-iavl #276
Conversation
|
since we are doing multiple releases of IAVL we could mark the current proofs as deprecated and add these then in the next major release remove the old ones. The sdk does not use them and other users would be using iavl the same way the sdk is so it shouldn't be a worry. Could you add deprecated flag to the old proofs godoc |
That might be a good idea. If so, we should make sure the API is final, and make sure there are no bugs that require breaking changes to proofs. We should also update the proof documentation. Could someone give me a quick summary of where IAVL proofs are used today? |
I couldn't find them being used in the sdk. I looked over a few projects that are forks of the sdk and they dont use them as well.. deadcode? |
|
They are turned into ProofOps when you query On the client side, you are supposed to parse and validate all this. Theoretically, there is a light-client mode in the cli (not sure how to turn it on) and this will validate all such queries. I was using them back in the original sdk 😉 0.8 or so.... hope it didn't all get refactored away |
I agree. I kept it separate to avoid super slow PR review cycles last summer. Now the core team will use this code it definitely should use private methods and returns the proof directly (just like current range proofs). In fact, we could replace all proofs with this, and returns I would merge this first, then add them as first class citizens to the tree, then see if we can start deprecating what is there |
|
@AdityaSripal Could you move this into the root package and fix the linter issues please? I'd also like to start discussing the plan for the final 0.15 proof API so that we can cut it soon. |
proof_ics23.go
Outdated
| return steps | ||
| } | ||
|
|
||
| func aminoVarInt(orig int64) []byte { |
There was a problem hiding this comment.
Is this really necessary? As far as I know, Amino uses standard Go variants, and we have utility functions for generating these in encoding.go - can you check if we can use any of those instead?
proof_ics23_test.go
Outdated
| for i := 0; i < size; i++ { | ||
| // create random 4 byte key | ||
| // nolint:gosec | ||
| key := []byte{byte(rand.Uint64()), byte(rand.Uint64()), byte(rand.Uint64()), byte(rand.Uint64())} |
There was a problem hiding this comment.
You can use rand.Read() as well to generate random byte slices.
proof_ics23.go
Outdated
| GetMembershipProof will produce a CommitmentProof that the given key (and queries value) exists in the iavl tree. | ||
| If the key doesn't exist in the tree, this will return an error. | ||
| */ | ||
| func (tree *ImmutableTree) GetMembershipProof(key []byte) (*ics23.CommitmentProof, error) { |
There was a problem hiding this comment.
We'll have to use t *ImmutableTree to appease the linter.
|
Thanks for updating! Added a couple of minor comments - can you update the changelog as well, and then merge this? |
Currently a simple copy of https://github.com/confio/ics23-iavl into this repo. This will remove the outside dependency sitting between iavl and SDK.
A future PR will optimize this further by directly creating the ICS proof rather than converting
cc: @ethanfrey