wasm-js!: Move all implementations into sdk-wasm-js#244
Conversation
#### Problem As outlined in anza-xyz#118, the sdk incorrectly assumes that all wasm builds are targeting a js environment, which breaks builds in non-js targets. #### Summary of changes The wasm-js code isn't strictly needed to make things work, so instead, move all of the code into different modules. This causes some copy-pasta, but keeps wasm builds lean. If people are using wasm-js, most likely they'll roll their own code anyway. As part of this, since solana-keypair now uses rand 0.8, we can upgrade the getrandom backend to 0.2.
joncinque
changed the title
grod220
left a comment
grod220
left a comment
There was a problem hiding this comment.
Small comments, up to you!
| .map_err(|err| format!("Invalid Hash value: {err:?}").into()) | ||
| } else if let Some(array) = value.dyn_ref::<Array>() { | ||
| let mut bytes = vec![]; | ||
| let iterator = js_sys::try_iter(&array.values())?.expect("array to be iterable"); |
There was a problem hiding this comment.
I know this is copied over and this probably beyond the scope of this PR, but there are a few unwraps and expects in the code. A panic here can't be caught on the JS side of things and causes an abort which could crash a node/browser process.
There was a problem hiding this comment.
I see two unwrap()s:
- console_log::init_with_level
- bincode::serialize(...)
We can probably fix those, since I could imagine those being fallible.
And the expects are both from js_sys::try_iter, after checking the underlying type is an Array and passing in array.values(), which is an Iterator. I think the expect there makes more sense because it should likely be a programmer error if the conversion fails.
Since @febo is waiting on this, I'll fix the unwraps in a follow-up
There was a problem hiding this comment.
It doesn't seem like it existed before, but instruction.rs looks like the only file in sdk-wasm-js with missing tests.
There was a problem hiding this comment.
It's mostly tested through transaction.mjs, where it actually gets compiled and all that. We could certainly add more tests in the future though.
wasm-js: Move all implementations into sdk-wasm-js As outlined in anza-xyz#118, the sdk incorrectly assumes that all wasm builds are targeting a js environment, which breaks builds in non-js targets. The wasm-js code isn't strictly needed to make things work, so instead, move all of the code into different modules. This causes some copy-pasta, but keeps wasm builds lean. If people are using wasm-js, most likely they'll roll their own code anyway. As part of this, since solana-keypair now uses rand 0.8, we can upgrade the getrandom backend to 0.2.
3 participants
Problem
As outlined in #118, the sdk incorrectly assumes that all wasm builds are targeting a js environment, which breaks builds in non-js targets.
Summary of changes
The wasm-js code isn't strictly needed to make things work, so instead, move all of the code into different modules. This causes some copy-pasta, but keeps wasm builds lean. If people are using wasm-js, most likely they'll roll their own code anyway.
As part of this, since solana-keypair now uses rand 0.8, we can upgrade the getrandom backend to 0.2.
NOTE: Leaving in draft until CI passesAll good!Closes #118 Fixes #117 Fixes #241