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

Resolve half of #67 (Part 1/2) #93

Merged
merged 7 commits into from
Feb 15, 2024
Merged

Resolve half of #67 (Part 1/2) #93

merged 7 commits into from
Feb 15, 2024

Conversation

skaunov
Copy link
Collaborator

@skaunov skaunov commented Feb 13, 2024

arkworks-rs have some issue while publishing (to <crates.io>) concerned with serde dependency. I don't feel like delving into that one is reasonable giving that it's outdated anyway. It's API have been sorted out, and documented.

The other crate can be found at https://crates.io/crates/plume_rustcrypto. Any changes to it are welcome: let's add it to this PR.

Then squash and merge.

Note from Aayush: We still need to update plume_arkworks, add simple examples, and fix the docs URLs.

@skaunov skaunov linked an issue Feb 13, 2024 that may be closed by this pull request
///
/// See <https://blog.aayushg.com/nullifier> for more information.
///
/// Find RustCrypto crate as `plume_crypto`.
Copy link
Member

Choose a reason for hiding this comment

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

Why did we name it plume_rustcrypto? plume_crypto seems to be a better name to me

Copy link
Member

Choose a reason for hiding this comment

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

This one can be plume_arkworks

Copy link
Member

@Divide-By-0 Divide-By-0 left a comment

Choose a reason for hiding this comment

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

Mostly on crate rename.

///
/// See <https://blog.aayushg.com/nullifier> for more information.
///
/// Find RustCrypto crate as `plume_crypto`.
Copy link
Member

Choose a reason for hiding this comment

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

This one can be plume_arkworks

@@ -1,7 +1,12 @@
[package]
name = "zk-nullifier"
name = "plume_rustcrypto"
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 make this one plume_crypto, and the other one plume_arkworks!

//!
//! See <https://blog.aayushg.com/nullifier> for more information.
//!
//! Find `arkworks-rs` crate as `plume_arkworks`.
Copy link
Member

Choose a reason for hiding this comment

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

Ah you already did this in some places!

@skaunov
Copy link
Collaborator Author

skaunov commented Feb 14, 2024

@Divide-By-0 ,
that plume_rustcrypto naming was actually last minute change, since I too wanted to go with "plume_crypto" for some time. But going through all the thing and some crates I felt like it's not communicating that clear that there's two versions: for arkworks-rs and for RustCrypto ecosystems. So made it a bit more specific. Let's give some time to make its way to how they both look... And it can be transferred easily to that other name. 🤷

@Divide-By-0
Copy link
Member

Divide-By-0 commented Feb 14, 2024

RustCrypto

That makes sense then, as long as the naming is consistent i.e. fix the 'Find RustCrypto crate as plume_crypto.' comment!

Regarding docs, the crates have no README so the documentation is bare on i.e. https://crates.io/crates/plume_rustcrypto -- to close this issue, we need to 1) add simple readmes, 2) add that autogenerated default rust docs that show all the pub interfaces (i.e. cargo doc) via their docstrings, and 3) some simple installation and 2 line examples in readmes with implementations. In my opinion, given the arkworks implementation is both unsafe and lower priority, there should be a big warning in that crate doc to contact us before using it.

@skaunov
Copy link
Collaborator Author

skaunov commented Feb 15, 2024

All are very valid points, I just need to come to something on how to best express them. 😅 I mean some doesn't go that smooth when trying.

  1. I don't like this bare look too. Though all I came up with is the link to this repo <README.MD> file; since that page isn't documentation yet, but it contains the link to the documentation (the one which cargo doc / pub interfaces) in the designated section. When it's not a monorepo stuff we have in the actual <README.MD> goes there. Hence the link. I don't like copying it, since it's kinda awkward to bump version with just <README.MD> changes... Any better ideas are welcome!

  2. That one is confusing, since there's a couple of quick ways to get to https://docs.rs/plume_rustcrypto/ really fast, and the link from the crate page is one of them. So people are really don't expect that.

  3. This one is really needed. I thought that the API is really small for adding an example, but they're always good! The problem is that without sign method it's hard to show the thing concisely; so I guess I'll do that one before closing this. Also I'll add the example to the other crate just so it would be present and maintained, and came naturally when that crate will be there.

For the last one I added a HAZMAT notice.

@Divide-By-0
Copy link
Member

Divide-By-0 commented Feb 15, 2024

All are very valid points, I just need to come to something on how to best express them. 😅 I mean some doesn't go that smooth when trying.

  1. I don't like this bare look too. Though all I came up with is the link to this repo <README.MD> file; since that page isn't documentation yet, but it contains the link to the documentation (the one which cargo doc / pub interfaces) in the designated section. When it's not a monorepo stuff we have in the actual <README.MD> goes there. Hence the link. I don't like copying it, since it's kinda awkward to bump version with just <README.MD> changes... Any better ideas are welcome!
  2. That one is confusing, since there's a couple of quick ways to get to https://docs.rs/plume_rustcrypto/ really fast, and the link from the crate page is one of them. So people are really don't expect that.
  3. This one is really needed. I thought that the API is really small for adding an example, but they're always good! The problem is that without sign method it's hard to show the thing concisely; so I guess I'll do that one before closing this. Also I'll add the example to the other crate just so it would be present and maintained, and came naturally when that crate will be there.

For the last one I added a HAZMAT notice.

  1. Sweet. It's totally fine to bump version with a README change -- no one's using it yet haha.
  2. Yeah, we should just include that docs.rs link in the README and in the github repo.
  3. Makes sense, lets do it in that order then.
  4. I don't see anything at https://docs.rs/plume_arkworks or https://crates.io/crates/plume_arkworks -- did it get moved?

@Divide-By-0 Divide-By-0 changed the title Resolve #67 Resolve half of #67 (Part 1/2) Feb 15, 2024
@Divide-By-0 Divide-By-0 merged commit e5febe3 into main Feb 15, 2024
11 checks passed
@Divide-By-0
Copy link
Member

Going to merge this for now so that the README shows up in the crates docs -- i expect this issue is still open however till we get sign merged etc!

@skaunov
Copy link
Collaborator Author

skaunov commented Feb 15, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crates API and documentation
2 participants