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

Clean up misplaced items in stdlib #4845

Open
Tracked by #1258
TomAFrench opened this issue Apr 19, 2024 · 3 comments
Open
Tracked by #1258

Clean up misplaced items in stdlib #4845

TomAFrench opened this issue Apr 19, 2024 · 3 comments

Comments

@TomAFrench
Copy link
Member

It seems like a fair number of function have been placed a bit haphazardly, e.g.

  • grumpkin_scalar_mul.nr vs scalar_mul.nr, why do we have this duplication? It's aztec code leaking into stdlib by the look of the serialization logic.
  • why aren't sha256 and sha512 included in the hash module? The compression function is defined there so why not the rest?

We should do a pass of placing things where they should be to address these and similar issues. We can make this change non-breaking by adding reexports such as

// stdlib/lib.nr
mod sha256 {
    use crate::hash::sha256::digest
}
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Apr 19, 2024
@jfecher
Copy link
Contributor

jfecher commented Apr 19, 2024

It's also worth discussing if we want the hashers like crate::hash::poseidon2::Poseidon2Hasher to be in the same module as their actual hash function which is usually placed directly in crate::hash. Perhaps they should originate in the more specific modules and just be reexported into crate::hash, or maybe crate::hash should just be an empty module containing other more specific hash modules.

@TomAFrench
Copy link
Member Author

Yeah, I was thinking along the same lines when writing this issue. I'm very much in favour of splitting a lot of these files out and using reexports liberally. I'd generally say that any trait in the stdlib should be in its own file due to the amount of boilerplate trait implementations for the primitive types it'll include, e.g. Eq and Ord

@TomAFrench
Copy link
Member Author

Worth considering implementing #4860 around the same time.

github-merge-queue bot pushed a commit that referenced this issue May 7, 2024
# Description

## Problem\*

Part of #4845 

## Summary\*

This PR splits the `ops` module into `arith` and `bit` submodules
similarly to the Rust stdlib.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 📋 Backlog
Development

No branches or pull requests

2 participants