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

feat(examples): add merkle tree package #631

Merged
merged 7 commits into from
May 24, 2023
Merged

Conversation

albttx
Copy link
Member

@albttx albttx commented Mar 21, 2023

Why this PR

I'm currently working on a Merkle Airdrop contract in Gno. you can see the ongoing progress on my fork albttx#1

This PR needs crypto/sha256 implemented #580

Description

This PR add a new package p/merkle for creating Merkle Tree.

I did my best to make it as simple as possible and to provide the minimum features required.

How has this been tested?

using gnodev

@albttx albttx requested a review from a team as a code owner March 21, 2023 16:59
@thehowl thehowl changed the title add p/merkle for merkletree feat(examples): add merkle tree package Mar 28, 2023
@thehowl
Copy link
Member

thehowl commented Mar 28, 2023

CI seems to be failing for the issue in #668

Could you add sha256 and hex in the whitelist in the meantime, for this PR?

@moul moul marked this pull request as draft March 29, 2023 10:43
@albttx albttx marked this pull request as ready for review April 20, 2023 14:39
@moul moul marked this pull request as draft April 20, 2023 21:49
@moul moul marked this pull request as ready for review April 20, 2023 21:50
@albttx albttx requested review from jaekwon and moul as code owners May 9, 2023 09:29
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merkle.gno also needs more documentation and comments

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add a go equivalent to the js code?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the merkle.gno is 100% go compatible.. with a quick look at the gno code, you can have the go code... is it really mendatory ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not necessarily. But it seems weird then that there is an example of JS code inside of the directory of a gno package :)

examples/gno.land/p/demo/merkle/merkle.gno Outdated Show resolved Hide resolved
examples/gno.land/p/demo/merkle/merkle.gno Outdated Show resolved Hide resolved
examples/gno.land/p/demo/merkle/merkle_test.gno Outdated Show resolved Hide resolved
examples/gno.land/p/demo/merkle/merkle.gno Outdated Show resolved Hide resolved
@albttx
Copy link
Member Author

albttx commented May 15, 2023

@thehowl thanks for your review, everything has beed fixed :)

wdyt ?

@ajnavarro ajnavarro added 🧾 package/realm Tag used for new Realms or Packages. 🌟 improvement performance improvements, refactors ... and removed 🌟 improvement performance improvements, refactors ... labels May 16, 2023
@ajnavarro ajnavarro requested a review from thehowl May 16, 2023 09:38
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small performance suggestion but code-wise looks good. However, I do think it would be better if the exported symbols had proper documentation. I know most packages in p/demo have little-to-no documentation as well, but I think it's also a trend that needs to change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not necessarily. But it seems weird then that there is an example of JS code inside of the directory of a gno package :)

examples/gno.land/p/demo/merkle/merkle.gno Outdated Show resolved Hide resolved
@albttx albttx force-pushed the p/merkle branch 2 times, most recently from 68407db to fa1ee30 Compare May 17, 2023 15:03
Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate this PR. However, since Gno already utilizes an automatic Merkle tree, it would be beneficial to compare the performance of this Merkle tree implementation against alternatives such as the avl package or built-in native types that are automatically merkelized.

The evaluation may yield a nuanced result, indicating that for groups of over N entries, this package outperforms others in terms of speed or cost-effectiveness.

If this package proves to be less efficient than alternative solutions, I suggest considering a different name to avoid misleading users into assuming similar advantages to those found in other platforms.

Overall, it would be beneficial to create more packages like this to provide diverse options for content management. Thank you!

@harry-hov harry-hov self-requested a review May 22, 2023 17:52
@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label May 24, 2023
@albttx
Copy link
Member Author

albttx commented May 24, 2023

@moul rebased

@albttx
Copy link
Member Author

albttx commented May 24, 2023

it would be beneficial to compare the performance of this Merkle tree implementation against alternatives such as the avl package or built-in native types that are automatically merkelized.

I don't understand your point, an avl tree will be stored on chain, which could be a lot of memory.
The goal of a Merkle tree is that the tree is stored off-chain, only the Merkle Root is on chain, so to ensure a key is good, we compute off-chain a merkle proof, witch will be validate on-chain.

Maybe a different approach would have been to add a "Proof" system on the avl tree ? 🤔 But AFAIK, most project use merkletreejs on client side to generate the merkle proof in the browser.

cc: @moul

@moul
Copy link
Member

moul commented May 24, 2023

Understood. This package wasn't initially designed as an internal proof verification system for an alternative tree storage. Its primary purpose is to facilitate proof generation in the browser and enable contract usage of the Verify function.

Naming it "merkletree" effectively highlights the included proof system. Let's proceed with the current name and focus on showcasing practical usage examples.

To maintain simplicity and composability, we can extend avl.Tree with a proof system, possibly named avl.ProofTree, or even consider making merkletree dependent on avl.Tree. However, let's prioritize obtaining usage examples first.

@moul moul merged commit 2469cbd into gnolang:master May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🤖 gnovm Issues or PRs gnovm related 🧾 package/realm Tag used for new Realms or Packages.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants