-
Notifications
You must be signed in to change notification settings - Fork 690
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: use libsecp256k1 in wasm #5910
base: develop
Are you sure you want to change the base?
refactor: use libsecp256k1 in wasm #5910
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, my only concern right now is to see whether it would be possible to get CI to check:
- That the wasm target builds at all
- To run some tests to cover
util::secp256k1::wasm
.
There's a handful of tests for util::secp256k1
:
cargo test -p stacks-common -- secp256k1 --list
Finished `test` profile [optimized + debuginfo] target(s) in 0.06s
Running unittests src/libcommon.rs (/target/debug/deps/stacks_common-c11773a9960b3696)
util::secp256k1::tests::sk_from_seed: test
util::secp256k1::tests::test_parse_serialize: test
util::secp256k1::tests::test_parse_serialize_compressed: test
util::secp256k1::tests::test_verify: test
util::secp256k1::tests::test_verify_benchmark_roundtrip: test
5 tests, 0 benchmarks
Doc-tests stacks_common
0 tests, 0 benchmarks
But they don't cover much -- I think we'd want to get closer to full coverage of the module code for both the native and wasm modules. But either way, I think it'd require supplying --target
arguments to cargo test
.
How do you build the wasm outputs? Does cargo test -p stacks-common --target wasm32-unknown-unknown
work?
So it's not the case yet, as described in #5891, I prefer to have an iterative approach and open multiple smaller PRs instead of having 1 big. The downside is that it implies merging PRs that, by themselves, don't get us to the point. And yes, the point is to ultimately run the check in the CI.
In Clarinet I've been using wasm-pack-test, simulating a full node env. Note: I haven't worked on this repo CI in the past, but I know it can get a bit more complex that it is on simpler projects, some help will be more than welcomed |
Okay, that makes sense to me, I'll just create an issue in this repo to capture the CI checks we want.
That pointer is helpful, I'll mention it in the CI issue. You probably don't need to write the CI stuff directly, I think someone who has worked with this repo's CI and actions can take it on, but they may end up pinging you with some questions when the time comes. |
Alright sounds good. Once it compiles to wasm, I'll add wasm-pack tests, and just tell which command to run. Should be straightforward to implement to implement in CI next |
Context
As part of #5891, I'm extracting code from the
feat/clarity-wasm-develop
branch to makedevelop
compilable to wasm.It involves removing some libs that don't work in node.js or the browser when compiling the code to Wasm. This is the case for sepc256k1.
Solution
Use a secp256k1 lib written in rust, that can compile to wasm.
This is handled using conditionnal compilation based on
cfg(target_family = "wasm")
In this case, we picked
libseckp256k1
a few years ago, but it looks like some other lib could be good alternative. Such ask256
. I'm open to suggestion regarding which lib we should use when targetting wasm.