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

ECDSA Support in signer #1064

Merged
merged 19 commits into from
Jul 19, 2023
Merged

ECDSA Support in signer #1064

merged 19 commits into from
Jul 19, 2023

Conversation

jsdw
Copy link
Collaborator

@jsdw jsdw commented Jul 12, 2023

Pushing #1052 over the finish line; thankyou @LGLO for your awesome addition :)

@jsdw jsdw changed the title Jsdw ecdsa fixes ECDSA Support in signer Jul 12, 2023
@jsdw
Copy link
Collaborator Author

jsdw commented Jul 12, 2023

One thing I still need to gain clarity on is that the SECP256K1 context was used in the sign, and the Secp256k1::signing_only() was used in from_seed(). The sp_core impl uses the Secp256k1::signing_only() in both cases (when no-std), and so for now I made this code do the same in the hopes that this also provides wasm support out of the box (the impl looked like it would). @LGLO perhaps you could provide some insight into your choices here?

I added a wasm test for escda sign/verify anyway so hopefully that will catch any wasm issues there.

I also think it's prolly a good idea to have a feature flag to enable each of sr25519 and ecdsa now that there are two impls, so I'll probably also add that before getting this reviewed etc!

@jsdw
Copy link
Collaborator Author

jsdw commented Jul 13, 2023

The global signing context seems to work fine in WASM, so I switched to using that (as Substrate does when "std" is enabled. I also added feature flags so that people can opt-out of impls they don't need, a check for them in CI and a small doc test

@jsdw jsdw marked this pull request as ready for review July 13, 2023 13:21
@jsdw jsdw requested review from a team as code owners July 13, 2023 13:21
@LGLO
Copy link
Contributor

LGLO commented Jul 13, 2023

@jsdw Thank you very, very much for taking care of it!

@LGLO perhaps you could provide some insight into your choices here?
Unfortunately, I can not. It seems that discrepancy came from my lack of awareness.
I think using SECP256K1 global context is OK. This code doesn't depend on randomness. I think the problem in no-std environment would be with context randomness initialization.

@jsdw
Copy link
Collaborator Author

jsdw commented Jul 13, 2023

Thanks for your reply and contribution! That makes sense; I think I'll have a pass over making this crate no_std soonish and then might need to revisit eg the global context thing which seems to require "std", but for now supporting std and wasm is good enough for us so it's all gravy :)

@jsdw jsdw merged commit fa16080 into master Jul 19, 2023
7 checks passed
@jsdw jsdw deleted the jsdw-ecdsa-fixes branch July 19, 2023 09:17
@jsdw jsdw mentioned this pull request Jul 24, 2023
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.

5 participants