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(smt): add smt crate #41

Merged
merged 11 commits into from
Aug 6, 2024

Conversation

waddaboo
Copy link
Contributor

@waddaboo waddaboo commented Jul 30, 2024

Description

Added the Sparse Merkle Tree (SMT) implementation in Rust.

Related Issue(s)

Closes: #8

Other information

  • Please do let me know if anything can be improved.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have run yarn format and yarn compile without getting any errors
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Additional Questions

  • Need to add actual hash functions to test?

@waddaboo waddaboo marked this pull request as ready for review July 30, 2024 09:47
@waddaboo waddaboo requested review from cedoor and sripwoud as code owners July 30, 2024 09:47
@cedoor cedoor requested review from 0xjei and curryrasul July 30, 2024 15:26
Copy link
Member

@sripwoud sripwoud left a comment

Choose a reason for hiding this comment

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

tACK

Thanks @waddaboo
Nice you added docstring and a README.

I

  • checked tests locally
  • checked the function signatures and overall code to see if it matches the original TS implementation

I suggested some improvements but they aren't blocking merging this PR imo

Let's wait for someone else review too cc @curryrasul
Don't forget to format the code in order to have the ci check pass

Copy link
Member

Choose a reason for hiding this comment

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

do you think this should be included in the published crates?
If not we need to add an exclude or include field to the crate's manifest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review!

I saw some other libraries included examples and some did not, so I'm not sure either. Since the README Usage section is the same as in examples, I think it's ok to remove it?

Comment on lines +12 to +15
pub enum Node {
Str(String),
BigInt(BigInt),
}
Copy link
Member

Choose a reason for hiding this comment

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

I like adding the flexibility of working with String or BigInt.

What do you (and other reviewers) think of using generics instead?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it might make sense to avoid doing these checks at runtime, but 90% of things can be represented by String and BigInt. I don't know if the game is worth the candle. Anyone has any additional comments here?

crates/smt/src/smt.rs Show resolved Hide resolved
/// # Returns
///
/// A new instance of the SMT.
pub fn new(hash: HashFunction, big_numbers: bool) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

see previous comment, wouldn't it be nicer to rely on generics instead of making a runtime check on big_numbers?

I am thinking about something like:

pub struct SMT<T>  
where
    T: Clone + PartialEq + Eq + Hash + Display + FromStr {}

pub enum Node<T>
where
    T: Clone + PartialEq + Eq + Hash + Display + FromStr,
{
    Value(T),
}

impl<T> SMT<T> {}


let smt_string = SMT::<String>::new(hash_function);
let smt_bigint = SMT::<BigInt>::new(hash_function);

Copy link
Member

Choose a reason for hiding this comment

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

let's not block that PR based on my previous comment. My comment on generics probably apply to the TS implementation too.
And this PR should mirror the TS implementation, which is does 👌

crates/smt/src/smt.rs Show resolved Hide resolved
crates/smt/src/smt.rs Outdated Show resolved Hide resolved
@sripwoud sripwoud added the feature 🚀 This is enhancing something existing or creating something new label Jul 31, 2024
Copy link
Member

@0xjei 0xjei left a comment

Choose a reason for hiding this comment

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

tACK - thank you @waddaboo the code looks polished and precise. I don't have additional comments beside the @sripwoud ones.

@waddaboo
Copy link
Contributor Author

waddaboo commented Aug 6, 2024

@0xjei Thanks for the review! As for the comment on using generics, do I implement it now or wait for further comments?

@sripwoud
Copy link
Member

sripwoud commented Aug 6, 2024

@0xjei Thanks for the review! As for the comment on using generics, do I implement it now or wait for further comments?

No need to implement it now.

Let's merge. @cedoor we need your approval too.
@curryrasul can make post merge comments. If necessary we ll implement other requests for changes later.

Thanks @waddaboo

@cedoor
Copy link
Member

cedoor commented Aug 6, 2024

@sripwoud thanks. Ready to be merged 👍🏽

@sripwoud sripwoud merged commit 41d20a6 into privacy-scaling-explorations:main Aug 6, 2024
4 checks passed
@github-actions github-actions bot mentioned this pull request Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🚀 This is enhancing something existing or creating something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a crate for SMT based on JS implementation
4 participants