Skip to content
This repository has been archived by the owner on Jan 19, 2021. It is now read-only.

Rename Trie.prove to Trie.createProof #120

Closed
holgerd77 opened this issue May 29, 2020 · 7 comments
Closed

Rename Trie.prove to Trie.createProof #120

holgerd77 opened this issue May 29, 2020 · 7 comments
Assignees

Comments

@holgerd77
Copy link
Member

I always had problems with "reading" the proof-related functions of MPT (in the sense of: grasp the functionality by seeing the names), see e.g. the examples. For a long time I thought that I am just not getting the whole picture.

Now on a closer look it seems to me that the name of the Trie.prove function is just plain wrong, I'll repaste the example code here:

const prove = await Trie.prove(trie, Buffer.from('test'))
const value = await Trie.verifyProof(trie.root, Buffer.from('test'), prove)
console.log(value.toString())

prove is is a noun, in the example above the const prove name makes no sense at all, but also when looking at the implementation of Trie.prove one realizes that this proves nothing at all but just creates a proof, which can then be used (therefore the double confusion on my side) to prove that an item is contained in the trie by using the Trie.verifyProof (or - here it would fit better - an imaginary Trie.prove) function. All this gets also verified by seeing the intuitively used naming conventions and descriptions in the tests.

So I would suggest to rename to:

const proof = await Trie.createProof(trie, Buffer.from('test'))
const value = await Trie.verifyProof(trie.root, Buffer.from('test'), prove)
console.log(value.toString())

Does this make sense respectively am I still missing something?

Along I would suggest the following simplifications/changes to the proof related API:

Old/current:

static async fromProof(proofNodes: Buffer[], proofTrie?: Trie): Promise<Trie>
static async prove(trie: Trie, key: Buffer): Promise<Buffer[]>
static async verifyProof(rootHash: Buffer, key: Buffer, proofNodes: Buffer[])

Suggestion:

type Proof = Buffer[]

static async fromProof(proof: Proof, trie?: Trie): Promise<Trie>
static async createProof(trie: Trie, key: Buffer): Promise<Proof>
static async verifyProof(rootHash: Buffer, key: Buffer, proof: Proof)

It would also be good if these methods could get some doc comments along for signature description.

I would really love to see this integrated in v4 if all makes sense (at least the first part), think this is really some not-to-underestimate mental blocker on using this.

//cc also @gabrocheleau

@s1na
Copy link
Contributor

s1na commented May 29, 2020

Yeah I think some documentation could probably help. createProof might be somewhat more clear. We could at first define it as an alias and mark prove as deprecated maybe to allow users to rename?

@holgerd77
Copy link
Member Author

Deprecation might also be a procedure. We need to get a bit better at managing deprecation changes though, probably by some more systematic tracking strategy on these, maybe along release planning issues or as separate permanent deprecation-topic issues. Currently this is a bit random if things really get removed on subsequent major releases.

@evertonfraga
Copy link
Contributor

Wow, that makes sense. The function name is indeed misleading. I can easily see code being created as:

if (Trie.prove(a)){}

so I think it makes sense to move to this new semantic. I also hope that the TypeScript version of MPT can provide living documentation to people in their IDEs.

@evertonfraga
Copy link
Contributor

Regarding deprecation, I agree we need a systematic approach. We can define a solid organization-level plan that can be coherently reproducible by any of us.

@gabrocheleau
Copy link
Contributor

I agree that createProof is a better name than proof. The proof method and the naming of variables in the documentation confused me initially.

I'll be happy to update the documentation and write examples if these proposed changes are included in the v4 release. I did not use the proof method in the MPT tutorial as they weren't required for the things I was trying to explain, but I definitely could add an example or two using createProof if people want.

@ryanio
Copy link
Contributor

ryanio commented May 29, 2020

ok sounds great I'll get started on this 👍

@ryanio
Copy link
Contributor

ryanio commented Jul 24, 2020

solved with #122

@ryanio ryanio closed this as completed Jul 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants