fix(identity): Enable rsa for wasm32 targets

[kirillsurkov]

Description

For now, the rsa feature is not supported on the wasm32 target. However, the ring crate supports wasm32 targets by enabling the wasm32_unknown_unknown_js feature.

Notes & open questions

I know that migrating to the rsa crate is planned, but it is not expected in the near future. These changes allow us to connect to public bootstrap nodes (which use RSA by default) from the browser.

Change checklist

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• A changelog entry has been made in the appropriate crates

[dariusc93]

Thanks for the PR. Looks like there is an error with wasm32-unknown-emscripten, which I wonder if we can get around that by checking explicitly for wasm32-unknown-unknown in the Cargo.toml?

Also can you update the Cargo.lock? :)

[dariusc93]

Thanks. Left a quick comment

[dariusc93]

I don't believe we need to use "any" here. I believe using "all" would be enough. So it could be something like "#[cfg(all(feature = "rsa", all(.....)))]" so we are checking if it has the target os, although do we need to check it here?

[kirillsurkov]

We check target_os to exclude wasm32-unknown-emscripten and wasm32-wasi. If we replace any with all, non-wasm targets will no longer pass the check.

[dariusc93]

Gotcha. Does it still happen if we were to just check for the feature itself?

Edit: i.e "#[cfg(feature = "rsa")]"

Also bare with me as I'm not at my computer right now :)

[kirillsurkov]

If we use just #[cfg(feature = "rsa")], the condition will be true on all platforms, which will break on wasm32-unknown-emscripten and wasm32-wasi.

[elenaf9]

We check target_os to exclude wasm32-unknown-emscripten and wasm32-wasi. If we replace any with all, non-wasm targets will no longer pass the check.

Are there any other target_os we want to reject? If not, couldn't we just do not(any(target_os = "wasm32-unknown-emscripten", target_os = "wasm32-wasi"))?
Or else not(all(target_arch = "wasm32", not(target_os = "unknown")))?

I don't feel strongly about it, but IMO the current multi-line feature gate is a bit difficult to understand. If you want to stick with the current impl the it would be great to have at least a comment on one of the feature gates(e.g., the one that gates the rsa module) that explains why it can only be enabled on wasm for target_os = "unknown".

[kirillsurkov]

@elenaf9 Hi! Sorry for the delay, I was on vacation for the past 2 weeks.

I tried simplifying the expression, and the best I could get is:

#[cfg(all(
    feature = "rsa",
    any(not(target_arch = "wasm32"), target_os = "unknown")
))]

It's still multi-line, so I'm proposing introducing a build.rs script that creates an alias rsa_supported using cfg_aliases crate.

With that, the usage would look like this:

#[cfg(all(feature = "rsa", rsa_supported))]

What do you think?

[dariusc93]

Also an fyi, no need to rebase the PR :)

[elenaf9]

I think we can just merge master into this branch once, before merging the whole PR.
Otherwise everyone who subscribed gets a GitHub notification every single time the master is merged :)

[dariusc93]

Im not sure if we should have another dependency in our tree (even though it's only for the building process). Thoughts @elenaf9 ?