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

Fixup rand #110

Closed
wants to merge 1 commit into from
Closed

Fixup rand #110

wants to merge 1 commit into from

Conversation

cbeck88
Copy link
Contributor

@cbeck88 cbeck88 commented Jan 7, 2020

I tried to uprev our version of ed25519-dalek but I ran into two issues:

1: missing #[cfg(feature = rand)] in secret.rs ( I think this is a further case of #108 ), we are building with these features:

..., default-features = false, features = ["alloc", "nightly", "serde", "u64_backend"] }

2: we lost Default trait on SecretKey, I don't know if that was intentional API change? but atm we are using that anyways, please lmk! thank you

@tarcieri
Copy link
Contributor

tarcieri commented Jan 7, 2020

I don't know if that was intentional API change?

IMO SecretKey should NOT have a Default

@cbeck88
Copy link
Contributor Author

cbeck88 commented Jan 7, 2020

@tarcieri yah I guess I agree -- but if it's going to be removed, I think there should be semantic versioning around the change, otherwise cargo is going to do bad things

@cbeck88
Copy link
Contributor Author

cbeck88 commented May 24, 2020

regarding the "Default" thing:
at this point we've been patching for like 4 months -- it will have little impact on mobilecoin how you decide to think about semver here. i'm happy to remove the Default thing if that will help the PR land

@isislovecruft
Copy link
Member

I removed Default on SecretKey on purpose. It was there because clear_on_drop required it, but with it, I would worry about mistakes in accidentally initialising a SecretKey to all zeros. I'm happy to take the rest of this PR.

@isislovecruft isislovecruft self-requested a review June 30, 2020 21:25
Copy link
Member

@isislovecruft isislovecruft left a comment

Choose a reason for hiding this comment

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

Is there a reason why you wanted Default for SecretKey?

@cbeck88
Copy link
Contributor Author

cbeck88 commented Jul 3, 2020

not really, we were just using it, we can stop using it

all i'm pointing out is, casually removing derive(Default) from public structs is a breaking change

The build is still broken when rand is disabled, we are building with

```
..., default-features = false, features = ["alloc", "nightly", "serde", "u64_backend"] }
```
@cbeck88
Copy link
Contributor Author

cbeck88 commented Jul 4, 2020

rebased on master and removed the re-addition of Default

@cbeck88
Copy link
Contributor Author

cbeck88 commented Jul 4, 2020

hmm, after rebase, I again can't build at this revision due to std/no_std issues :(

Will investigate further

@cbeck88
Copy link
Contributor Author

cbeck88 commented Aug 4, 2020

closing in favor of #139 (comment)

@cbeck88 cbeck88 closed this Aug 4, 2020
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.

None yet

3 participants