Skip to content

ECDSA Support in signer#1064

Merged
jsdw merged 19 commits intomasterfrom
jsdw-ecdsa-fixes
Jul 19, 2023
Merged

ECDSA Support in signer#1064
jsdw merged 19 commits intomasterfrom
jsdw-ecdsa-fixes

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
@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