make getrandom behind the wasm feature flag

[shekohex]

Greetings, I am attempting to utilize genaric-ec (using cggmp21) by compiling it with the no_std feature and running it in wasm (albeit not in the browser). I initially believed that the compilation would be flawless, however, due to the requirement for getrandom (even without the wasm feature), the compilation fails and complains about the absence of the get random implementation.

Before this change, running cargo b -p generic-ec --no-default-features --target wasm32-unknown-unknown result in the following error:

Compiling getrandom v0.2.12
error: the wasm*-unknown-unknown targets are not supported by default, you may need to enable the "js" feature. For more information see: https://docs.rs/getrandom/#webassembly-support
   --> /home/codespace/.cargo/registry/src/index.crates.io-6f17d22bba15001f/getrandom-0.2.12/src/lib.rs:291:9
    |
291 | /         compile_error!("the wasm*-unknown-unknown targets are not supported by \
292 | |                         default, you may need to enable the \"js\" feature. \
293 | |                         For more information see: \
294 | |                         https://docs.rs/getrandom/#webassembly-support");
    | |________________________________________________________________________^

error[E0433]: failed to resolve: use of undeclared crate or module `imp`
   --> /home/codespace/.cargo/registry/src/index.crates.io-6f17d22bba15001f/getrandom-0.2.12/src/lib.rs:347:9
    |
347 |         imp::getrandom_inner(dest)?;
    |         ^^^ use of undeclared crate or module `imp`

For more information about this error, try `rustc --explain E0433`.

With this change, running cargo b -p generic-ec --no-default-features --target wasm32-unknown-unknown compiles without issues.

[survived]

You need to enable wasm feature by using cargo build -p generic-ec --target wasm32-unknown-unknown --features wasm,all-curves command to make it work. We continuously test that it works in GH actions. Or are you saying that enabling wasm feature doesn't work?

[shekohex]

You need to enable wasm feature by using cargo build -p generic-ec --target wasm32-unknown-unknown --features wasm,all-curves command to make it work. We continuously test that it works in GH actions. Or are you saying that enabling wasm feature doesn't work?

It works, the only issue is that requires getrandom/js and we are working in a non-browser evn, so no js. Hence, removing the need for getrandom all together, since the rng we are using does not depend on getrandom backend.

[survived]

Ah I see. In this case, I think that probably wasm feature shouldn't be there at all, the client code should either enable getrandom/js feature or disable getrandom. Could you remove this feature entirely? It'd probably require some other changes to fix CI.

[survived]

Also, could you set up commit signing on your side? Repo settings will not allow merging any unsigned/unverified code.

[shekohex]

Also, could you set up commit signing on your side? Repo settings will not allow merging any unsigned/unverified code.

I do have code signing enabled, github for some reason when using codespaces does not work well. Will sign all the commits and push first.

[shekohex]

Hey @survived is it okay to add Nix & flake devenv files to this repo? It makes development consistent across different machines and easier to set up.

[survived]

I'll let @maurges decide, he's fan of nix, so he'll probably be reviewing that part :)

[maurges]

The nix shell would only contain cargo+rustc (and libc) though? Or do you want to setup a flake for compiling to wasm as well?

[shekohex]

Also Related: LFDT-Lockness/stark-curve#4

[shekohex]

The nix shell would only contain cargo+rustc (and libc) though? Or do you want to setup a flake for compiling to wasm as well?

Just the dev shell, no packaging or build using nix are required, so the same tooling and workflow would stay the same, only the initial setup of the repo. Also, I'm using direnv so once you cd dfns/generic-ec it automatically enters the dev shell.

[shekohex]

I've pushed most of the updates, I do have one more test to add in the CI to ensure that WASM + no_std works.

[survived]

@shekohex actually, we've decided that we don't want to have nix file in the repo. I assume, in your workflow you could put nix file to parent dir and it would work fine with direnv?

[shekohex]

@shekohex actually, we've decided that we don't want to have nix file in the repo. I assume, in your workflow you could put nix file to parent dir and it would work fine with direnv?

Yeah, that works, no worries. I just did not push any nix related nor direnv files to the repo, so we are good.

[shekohex]

we will also need a new version of stark curve crate so we can get the example that uses no_std to work.

[survived]

stark-curve v0.1.1 is published

[shekohex]

Could you please now run the CI?

[survived]

I don't see a button to run a CI. Could you push another commit (or do git commit --amend && git push -f)? Maybe it'll reappear

[maurges]

@shekohex nostd example doesn't build on CI it seems. Do you have access to build logs?

[shekohex]

@shekohex nostd example doesn't build on CI it seems. Do you have access to build logs?

It builds! https://github.com/dfns/generic-ec/actions/runs/7855819173/job/21475955726?pr=17
The only issues are clippy and docs are failing since they try to build it in with std enabled. not sure how we can exclude it from clippy, tests and docs?

[survived]

You should be able to exclude them by adding --exclude cli flag, e.g. we do that here:
https://github.com/dfns/generic-ec/blob/3de295debbe61e2d0f3f4a999372da62968c00e2/.github/workflows/rust.yml#L61-L62

[shekohex]

You should be able to exclude them by adding --exclude cli flag, e.g. we do that here:

https://github.com/dfns/generic-ec/blob/3de295debbe61e2d0f3f4a999372da62968c00e2/.github/workflows/rust.yml#L61-L62

Thanks for the tip, I've updated the rust.yml file, and now it should work, could you approve the CI run?

[survived]

You can try the commands locally as well!

[survived]

Seems like it requires a workspace flag now

[shekohex]

Seems like it requires a workspace flag now

Done!

[survived]

Let me just check with our security team what's our procedure, they could also want to review PR

[C0x41lch0x41]

looks good to me