Skip to content

Conversation

@fspreiss
Copy link
Contributor

@fspreiss fspreiss commented Jan 8, 2025

Adapts ic-validator-ingress-message to support Javascript WASM environments for Cargo builds via a new feature js, and also makes ic-cdk optional, but still keeps ic-cdk enabled by default, to keep backwards compatibility.

Both

  • cargo build -p ic-validator-ingress-message --target=wasm32-unknown-unknown --no-default-features --features=js and
  • cargo build -p ic-validator-ingress-message --target=wasm32-unknown-unknown --features=js

succeed.

Note that cargo build -p ic-validator-ingress-message --target=wasm32-unknown-unknown fails because getrandom is not supported for wasm*-unknown-unknown targets. Canisters that want to use ic-validator-ingress-message have to manually fix this, for example by using our ic-crypto-getrandom-for-wasm crate. There is also ic-validator-ingress-message-test-canister to test the usage of ic-validator-ingress-message from a canister.

@github-actions github-actions bot added the feat label Jan 8, 2025
@fspreiss fspreiss changed the title feat(crypto): CRP-2665 support wasm+js env for ic-validator-ingress-message feat(crypto): CRP-2665 make ic-cdk optional and support wasm+js env in ic-validator-ingress-message Jan 8, 2025
@fspreiss fspreiss marked this pull request as ready for review February 3, 2025 10:42
@fspreiss fspreiss requested a review from a team as a code owner February 3, 2025 10:42
@fspreiss fspreiss requested a review from randombit February 4, 2025 09:42
@altkdf
Copy link
Contributor

altkdf commented Feb 4, 2025

Thanks @fspreiss, LGTM! I was wondering though if you see an easy way to test this in js.

@fspreiss
Copy link
Contributor Author

fspreiss commented Feb 6, 2025

I was wondering though if you see an easy way to test this in js.

Unfortunately not, no. Not sure what the most minimal way for getting a WASM+JS environment is in a Rust test. Do you have an idea?

The original issue report has a minimal example to reproduce the error, but it relies on a full-fledged cargo project and on running npx wrangler dev. Setting up such a pipeline to test this PR seems a bit much.

Would it maybe be sufficient to just ensure that
cargo build -p ic-validator-ingress-message --target=wasm32-unknown-unknown --no-default-features --features=js and
cargo build -p ic-validator-ingress-message --target=wasm32-unknown-unknown --features=js (copy/pasted from the PR description) work? If so, I'm also not sure how to do that, given we are operating with Bazel.

@altkdf
Copy link
Contributor

altkdf commented Feb 12, 2025

It seems wasm-pack provides some potentially useful testing functionality.

wasm-pack test --node --firefox --chrome --safari --headless

At the first glance, it looks like there will be some (but not too many) modifications needed in the tests. Not sure if this should be a separate PR then. But my personal preference would be to have tests in this PR if that's not too much work.

Would it maybe be sufficient to just ensure that
cargo build -p ic-validator-ingress-message --target=wasm32-unknown-unknown --no-default-features --features=js and
cargo build -p ic-validator-ingress-message --target=wasm32-unknown-unknown --features=js (copy/pasted from the PR description) work? If so, I'm also not sure how to do that, given we are operating with Bazel.

This seems to me like the minimum we should have. But making sure that the returned time is actually in some meaningful range seems better.

@fspreiss
Copy link
Contributor Author

fspreiss commented Mar 31, 2025

@altkdf, I believe we can do exactly what we need with Bazel's rust_wasm_bindgen_test, which is an extension to rules_rust. The rules_rust version we currently use is too old I believe (it doesn't have this extension yet). I talked to the developers who initially asked for this feature, and they said "Yes! Its working great with our cloudflare worker!", so I suggest to merge this as is and add the tests once we have the respective support in Bazel: for that I created CRP-2756.

@fspreiss fspreiss enabled auto-merge March 31, 2025 16:02
@fspreiss fspreiss added this pull request to the merge queue Mar 31, 2025
Merged via the queue into master with commit 4e54e57 Mar 31, 2025
20 checks passed
@fspreiss fspreiss deleted the franzstefan/ic-validator-ingress-message-js-wasm branch March 31, 2025 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants