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

Refactor rand from ff to its own crate #113

Closed
ValarDragon opened this issue Dec 6, 2020 · 8 comments · Fixed by #129
Closed

Refactor rand from ff to its own crate #113

ValarDragon opened this issue Dec 6, 2020 · 8 comments · Fixed by #129
Assignees

Comments

@ValarDragon
Copy link
Member

ValarDragon commented Dec 6, 2020

its a bit odd that test_rng and uniform_rand traits are coming from ark-ff at the moment. This also introduces oddities like #112 now introducing a dev-dependency to ark-serialize on ark-ff.

Refactoring this into its own crate I think will also give more flexibility for allowing feature flags on the test_rng that don't make sense to add when its on ark-ff. (E.g. making test_rng's seed non-static if you're trying to run randomized tests on more data points)

@Pratyush
Copy link
Member

Pratyush commented Dec 6, 2020

Yeah, I was thinking of moving them to ark-std. how does that sound?

@ValarDragon
Copy link
Member Author

That sounds good to me

@ValarDragon
Copy link
Member Author

Should rand be its own crate in arkworks-rs/utils, or should it just be a single file in ark-std?

@Pratyush
Copy link
Member

just a single file in ark-std, probably a module named rand. (Basically, we would just move the rand module of ark-ff there)

@ValarDragon
Copy link
Member Author

Opened a PR to do that on ark-std. I think we should then mark the rand methods in ark-ff as deprecated (so that things won't break in release v0.2.0), and then delete the methods in ark-ff on master after the next release.

@ValarDragon ValarDragon self-assigned this Dec 11, 2020
@Pratyush
Copy link
Member

Pratyush commented Dec 11, 2020

Let's re-export ark-std's rand to avoid this breakage; you can mark it as deprecated by annotating it with the #[deprecated] attribute.

@ValarDragon
Copy link
Member Author

We can't deprecate a re-export unfortunately (rust-lang/rust#30827)

I propose for now we do the following:

  • have ark-ff re-export the UniformRand trait with a notice in the changelog that its deprecated and to update your dependency before the next release. This is because UniformRand is a trait, and multiple versions will probably cause issues AFAIU.

  • Have ark-ff re-implement test_rng(), with a deprecated flag to switch to ark_std. (Which will also remind them to switch for Uniform Rand if they use it)

@Pratyush
Copy link
Member

Hmm that's unfortunate, but your workaround is good enough, I suppose.

@ValarDragon ValarDragon mentioned this issue Dec 12, 2020
6 tasks
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 a pull request may close this issue.

2 participants