Skip to content

[zk-sdk] Separate out wasm components in zk-sdk to zk-sdk-wasm-js#78

Merged
samkim-crypto merged 7 commits intosolana-program:mainfrom
samkim-crypto:zk-sdk-wasm-js
Sep 23, 2025
Merged

[zk-sdk] Separate out wasm components in zk-sdk to zk-sdk-wasm-js#78
samkim-crypto merged 7 commits intosolana-program:mainfrom
samkim-crypto:zk-sdk-wasm-js

Conversation

@samkim-crypto
Copy link
Copy Markdown
Contributor

@samkim-crypto samkim-crypto commented Sep 19, 2025

I am starting to tackle #69.

As a preliminary step, I created zk-sdk-wasm-js as was done in anza-xyz/solana-sdk#138. The following is the motivation:

  • Adding all the wasm components in a dedicated crate makes the code a lot cleaner
  • If a crate has both cdylib and rlib declared as crate types, then we can't apply some link-time optimizations. Currently, zk-sdk has both cdylib and rlib, but we can't remove cdylib if we want to use wasm-bindgen inside.

So I went ahead and removed wasm-bindgen in the zk-sdk (b78d644). I am basically starting over from the beginning. I left in cdylib for now because this seems to require some additional changes to the ci script that I have to figure out. I'll address this in a follow-up.

Then, I created zk-sdk-wasm-js (449eee1). This crate should contain the wrapper types/functions of the zk-sdk types/functions that we want to include in our wasm build. For now, I just started with the types in the encryption module of the zk-sdk. In follow-up PRs, I will add wrapper types for the rest of the components in the zk-sdk.

Then I updated the CI accordingly to test the zk-sdk-wasm-js crate (88ff541). I added some simple wasm-bindgen tests. These tests are definitely not sufficient for the "Testing and CI/CD" as mentioned in #69, but I think it should do the job of sanity checking for now.

@samkim-crypto samkim-crypto force-pushed the zk-sdk-wasm-js branch 3 times, most recently from 065c9c7 to 88ff541 Compare September 19, 2025 10:30
@samkim-crypto samkim-crypto marked this pull request as ready for review September 19, 2025 10:42
Copy link
Copy Markdown
Member

@grod220 grod220 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good! Just a few questions.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add pnpm zk-sdk-wasm-js:test-wasm to the workflow as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, good idea. This is now added to the workflow.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like these artifacts should be .gitignore'd?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the sloppiness. These files should be now removed and the gitgnore updated.

mod tests {
use {super::*, wasm_bindgen_test::*};

wasm_bindgen_test_configure!(run_in_browser);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry this was sloppiness from my side. I was playing around with both node and the browser setting and forgot to delete these lines. It seems like we can't reuse unit tests for both node and the browser setting. The tests run by default in node and then if we have the wasm_bindgen_test_configure!(run_in_browser) line, then the tests run in the browser setting.

If we refactor the unit tests into a dedicated test file, then it seems possible to reuse the tests for both node and the browser setting, but since these unit tests are for sanity check any way, I removed wasm_bindgen_test_configure!(run_in_browser) to run the tests in node for now.

Copy link
Copy Markdown
Member

@grod220 grod220 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done!

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.

2 participants