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

Extract curve25519 crate from zk-token-sdk #951

Merged
merged 17 commits into from
Jun 18, 2024

Conversation

kevinheavey
Copy link

@kevinheavey kevinheavey commented Apr 21, 2024

Problem

As the zk-token-sdk crate itself correctly notes, the code in the curve25519 folder should live in its own crate:

//! This module lives inside the zk-token-sdk for now, but should move to a general location since
//! it is independent of zk-tokens.

A more pressing motivation for me is that I have code that depends on bpf_loader, which only needs the curve25519 code from zk-token-sdk, and it takes a while to compile the whole zk-token-sdk crate.

Summary of Changes

Extracts zk-token-sdk/src/curve25519 into its own crate and re-exports it for backwards compatibility. Updates code in the monorepo to use the new crate directly.

@mergify mergify bot requested a review from a team April 21, 2024 23:13
@kevinheavey
Copy link
Author

Think the downstream checks are failing just because the new crate isn't on crates.io yet

@Xuanwo
Copy link

Xuanwo commented Apr 22, 2024

Hi, should we merge #946 first before doing this refactor?

@kevinheavey
Copy link
Author

When that's merged I will rebase and make sure the relevant changes to the curve25519 dir are ported over

@samkim-crypto
Copy link

@kevinheavey, I wanted to merge #1495 and #1693 in before this PR, but it seems like those PRs are getting delayed and it would be ideal to get this PR in before agave 2.0 is cut. Do you mind rebasing the PR to test the CI? And let's put this crate under the directory curves as was done in #1495.

Copy link

@samkim-crypto samkim-crypto left a comment

Choose a reason for hiding this comment

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

Okay thanks! I think the CI is not too bad. I can reserve the crate to take care of crate-check. Other than that, it appears that downstream tests are the main ones.

Getting rid of the target_os restriction below will take care of some failures, so let's make that change and see what we get with the CI.

curves/curve25519/src/lib.rs Show resolved Hide resolved
@samkim-crypto
Copy link

Sorry for the delay on this 🙏 . All the CI passes, so I will merge it after you rebase.

@samkim-crypto samkim-crypto self-requested a review June 18, 2024 02:35
Copy link

@samkim-crypto samkim-crypto left a comment

Choose a reason for hiding this comment

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

Thank you for the changes!

@samkim-crypto samkim-crypto merged commit b855bd0 into anza-xyz:master Jun 18, 2024
51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants