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

Fix keychain to work with non-Copy secp::SecretKey #2851

Closed
wants to merge 2 commits into from

Conversation

eupn
Copy link
Contributor

@eupn eupn commented May 27, 2019

This PR allows keychain to work with secp::SecretKey which made non-Copyable in mimblewimble/rust-secp256k1-zkp#51

This fix should be merged as soon as mimblewimble/rust-secp256k1-zkp#51 is.

Update: mimblewimble/rust-secp256k1-zkp#51 is merged, so this PR can be merged as well.

@antiochp
Copy link
Member

antiochp commented May 27, 2019

Why Clone and not Copy? What's the benefit?

From the other PR -

impossible to implement both Drop and Copy at the same time.

Edit: I get it now.

@eupn
Copy link
Contributor Author

eupn commented May 27, 2019

@antiochp

Why Clone and not Copy? What's the benefit?

It is impossible to implement both Copy and Drop. Therefore, if one wants to use a custom implementation of Drop (which zeroes underlying data, for example), he should remove Copy derive. Since Copy trait is no longer implemented, the only option left is to use .clone to satisfy borrow-checker. Hope this makes sense.

@eupn eupn changed the title [DNM] Fix keychain to work with non-Copy secp::SecretKey Fix keychain to work with non-Copy secp::SecretKey May 30, 2019
@garyyu
Copy link
Contributor

garyyu commented May 31, 2019

Similar comment as in mimblewimble/grin-wallet#121 (comment)

@garyyu
Copy link
Contributor

garyyu commented Jun 5, 2019

Related: #2847 (comment)

@yeastplume yeastplume modified the milestones: 2.x.x, 2.0.0 Jun 6, 2019
@yeastplume
Copy link
Member

the PR in rust-secp256k1 may have been merged, but a new version hasn't been released. @jaspervdm I assume this will need to go in for 2.0.0 when we do a new release of rust-secp for all of the other 2.0.0 changes?

@jaspervdm
Copy link
Contributor

If it has been merged in rust-secp already then yes we need this for v2.0.0

@yeastplume
Copy link
Member

Fixed via #2848

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.

6 participants