Enable wasm32 compilation and runtime of the binary-port-access lib #1
Enable wasm32 compilation and runtime of the binary-port-access lib #1
Conversation
…rt-client into rustSDK-feat-2.0 # Conflicts: # Cargo.lock
…rt-client into rustSDK-feat-2.0 # Conflicts: # Cargo.lock # binary_port_access/Cargo.toml
# Conflicts: # Cargo.toml # binary_port_access/Cargo.toml
# Conflicts: # Makefile
…rt-client into rustSDK-feat-2.0 # Conflicts: # .github/workflows/ci-cd.yml # Makefile
wojcik91
left a comment
There was a problem hiding this comment.
LGTM, great documentation. I'd prefer if someone with more subject matter experience also looked through this
There was a problem hiding this comment.
Since we don't use a --release profile I think the make build step is currently just duplicating stuff that is later done by other steps.
There was a problem hiding this comment.
I added a
build-for-release:
cargo build --release
there was not real release process here but I don't believe other steps include a build, like test / lint / fmt / audit, test does not build
Correct me please if I miss the point.
| node_address: &str, | ||
| request: BinaryRequest, | ||
| ) -> Result<BinaryResponseAndRequest, Error> { | ||
| let request_id = rand::thread_rng().gen::<u16>(); |
There was a problem hiding this comment.
Not a fan of using a random number generator here... u16 is small enough to where you will run into collisions quickly -- from my testing, around 317 iterations. Given the size of this type, it's plausible to experience collisions way sooner than that too. Not a big deal, but there should be a simpler way of tackling this. Would a counter not be enough?
There was a problem hiding this comment.
Hi, That's a good point. usually it's a u64 in sidecar/node, not here as per request def in node. (It does not really matter to have repetition as soon as they are not to much successive, I think previous code was sending 0.)
I added the counter as suggested.
let request_id = COUNTER.fetch_add(1, Ordering::SeqCst); // Atomically increment the counter
There was a problem hiding this comment.
@igor-casper Actually counter is not that great because in CLI it will be always 0, so collision very fast. The counter starts at 0 at every request. A collision every 317 iterations was actually probably better than always 0.
There was a problem hiding this comment.
@gRoussac You're probably right for the purposes of the client, but that's a CLI issue; As a standalone library, I think it should do its best to work in any context and shouldn't cater to just the client. How about you expose a method in the lib for manually assigning a value to the counter, so that downstream has the ability to decide for themselves? (CLI could use this to initialize it to a random value etc.)
|
Hi @gRoussac , |
…rt-client into rustSDK-feat-2.0 # Conflicts: # Cargo.lock # binary-port-access/src/utils.rs
|
@wojcik91 @igor-casper Thanks for your good review I appreciate. I think I treated it please give a last check at commit 94c871e |
570bc15 to
d0b29d3
Compare
1c1736f to
89799f1
Compare
…rt-client into rustSDK-feat-2.0 # Conflicts: # binary_port_access/src/communication.rs
|
@zajko @devendran-m I think I ported your last changes but I didn't implement |
As MVP Enable wasm32 compilation and runtime of the nested binary-port-access library.
Basically expose the binary-port-access lib as intended for Rust (non wasm32) / NodeJs (wasm32) / Browser WebSockets (wasm32)
netmodule.Note that browsers have CORS (Cross-Origin Resource Sharing) restrictions, so the WebSocket requests should/may be addressed to a WebSocket proxy.
The added
communicationmodule provides both shared and platform-specific functionalities for managing communication processes.-
common.rs-
wasm32.rsContains WebAssembly-specific (wasm32target) communication utilities, available only when compiled for WebAssembly.A TODO remains to replace usage of
js_sys::evalon Node.js (script is local, not remote so no security issue but nice to change to global function execution withjs_sys::Reflect::applywhich should be more appropriate)NodeJs
Chromium Browser