Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WASM: sdk-common #1170

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

WASM: sdk-common #1170

wants to merge 10 commits into from

Conversation

dangeross
Copy link
Collaborator

@dangeross dangeross commented Feb 18, 2025

This PR updates the sdk-common crate to be compilable to the wasm32-unknown-unknown target.

  • Disable the BIP353 DNS lookup from the WASM target. The DNS lookup dependency hickory-resolver requires low level TCP/IP access. I also tested domain alternative without luck
  • Exchange the use of the tonic transport feature in WASM for tonic_web_wasm_client
  • Update async traits to be WASM compatible
  • Add necessary wasm_bindgen/tsify_next decorators to exposed types
  • Update tests to be WASM compatible including adding a mockito alternative for WASM server mocking
  • Add CI tests for WASM and sdk-common as a WASM dependency check

Open questions:

  • BIP353: Would a Breez API to do DNS TXT lookups be acceptable?

Fixes: breez/breez-sdk-liquid#745

@roeierez
Copy link
Member

  • BIP353: Would a Breez API to do DNS TXT lookups be acceptable?

There is DNS over http protocol and services. https://developers.google.com/speed/public-dns/docs/doh
I think google has a public endpoint also.

Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

Thanks Ross for this PR! It really allows us to see what are the main issues and requires us to discuss an approach before we continue with the higher levels.
One thing we should think and discuss is how to minimize the wasm specific code changes and how to isolate it as much as possible (IMO even if we have to add more "duplicate" code).
I still don't have a silver bullet here but one think worth to consider is if we treat the SDK common as internal (not exposing structures to the browser) we might avoid wasmify all the structures.

api_key,
})
}

#[cfg(not(target_arch = "wasm32"))]
Copy link
Member

Choose a reason for hiding this comment

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

Since both grpc_client & grpc_channel implements the same required interfaces perhaps we can just abstract their creation instead of duplicate all methods?
If it is not possible to have one struct field then perhaps we can just create a generic method that returns the right interface implementations and chooses internally which structure to return?

use serde::{Deserialize, Serialize};

use crate::grpc::RatesRequest;
use crate::prelude::BreezServer;
use crate::with_connection_retry;

/// Trait covering fiat-related functionality
#[tonic::async_trait]
#[cfg_attr(not(target_arch = "wasm32"), async_trait)]
#[cfg_attr(target_arch = "wasm32", async_trait(?Send))]
Copy link
Member

Choose a reason for hiding this comment

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

Will this work also for the non wasm architecture?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, tested with both CLIs

@@ -285,6 +301,7 @@ fn extract_bip353_record(records: Vec<String>) -> Option<String> {
bip353_record.first().cloned()
}

#[cfg(not(target_arch = "wasm32"))]
async fn bip353_parse(
input: &str,
dns_resolver: &AsyncResolver<GenericConnector<TokioRuntimeProvider>>,
Copy link
Member

Choose a reason for hiding this comment

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

Seems like we can implement a DNS resolver over https and pass it as a trait to the bip353_parse function.

@dangeross
Copy link
Collaborator Author

Can test by cloning my WASM playground

@dangeross dangeross force-pushed the savage-wasm-sdk-common branch from 9ed23ec to 72c6a65 Compare February 19, 2025 09:52
Copy link
Contributor

@JssDWt JssDWt left a comment

Choose a reason for hiding this comment

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

Great milestone this one!

In general I think we should try keeping the wasm feature flags as compact as possible. Like moving things that need it to their own file. Or creating our own macros for testing or model derivations for example.

] }

[target.'cfg(not(target_arch = "wasm32"))'.build-dependencies]
tonic-build = "^0.8"
Copy link
Contributor

Choose a reason for hiding this comment

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

Noting tonic-build needs an upgrade next.

@dangeross dangeross force-pushed the savage-wasm-sdk-common branch 2 times, most recently from 88e7f0e to 19ed955 Compare February 19, 2025 13:19
Copy link
Contributor

@danielgranhao danielgranhao left a comment

Choose a reason for hiding this comment

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

Awesome work! Found a couple of issues with tests, otherwise is looking good

$(CLANG_PREFIX) cargo clippy --target=wasm32-unknown-unknown -- -D warnings -A clippy::uninlined-format-args

wasm-test:
$(CLANG_PREFIX) wasm-pack test --headless --firefox
Copy link
Contributor

@danielgranhao danielgranhao Feb 20, 2025

Choose a reason for hiding this comment

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

We should add firefox to the readme prerequisites.

Also, I tried running the tests on chrome and there a couple of tests failed:

failures:

---- input_parser::tests::test_external_parsing_bitcoin_address_and_bolt11 output ----
    JS exception that was thrown:
        Error: Unrecognized input type
            at imports.wbg.__wbindgen_error_new (http://127.0.0.1:8000/wasm-bindgen-test:1070:21)
            at sdk_common-0e2972322936c71e.wasm.__wbindgen_error_new externref shim (http://127.0.0.1:8000/wasm-bindgen-test_bg.wasm:wasm-function[20745]:0x720e87)
            at sdk_common-0e2972322936c71e.wasm.wasm_bindgen::JsError::new::hd2e35b77ed3041d8 (http://127.0.0.1:8000/wasm-bindgen-test_bg.wasm:wasm-function[12491]:0x6a22bf)
            at sdk_common-0e2972322936c71e.wasm.<core::result::Result<(),E> as wasm_bindgen_test::__rt::Termination>::into_js_result::{{closure}}::h31537578390b5f52 (http://127.0.0.1:8000/wasm-bindgen-test_bg.wasm:wasm-function[3019]:0x4cf863)
            at sdk_common-0e2972322936c71e.wasm.core::result::Result<T,E>::map_err::h259c253d9c7c5cbd (http://127.0.0.1:8000/wasm-bindgen-test_bg.wasm:wasm-function[7480]:0x5fef42)
            at sdk_common-0e2972322936c71e.wasm.<core::result::Result<(),E> as wasm_bindgen_test::__rt::Termination>::into_js_result::h42b88e6f5f052ddd (http://127.0.0.1:8000/wasm-bindgen-test_bg.wasm:wasm-function[12792]:0x6a8f03)
            at sdk_common-0e2972322936c71e.wasm.wasm_bindgen_test::__rt::Context::execute_async::{{closure}}::hcd1cc5f1b4de1bf2 (http://127.0.0.1:8000/wasm-bindgen-test_bg.wasm:wasm-function[2721]:0x4abf78)
            at sdk_common-0e2972322936c71e.wasm.<wasm_bindgen_test::__rt::TestFuture<F> as core::future::future::Future>::poll::{{closure}}::{{closure}}::h923e23a775fadbaf (http://127.0.0.1:8000/wasm-bindgen-test_bg.wasm:wasm-function[7577]:0x60341f)
            at sdk_common-0e2972322936c71e.wasm.wasm_bindgen::convert::closures::invoke0_mut::h5c7b6a0834f2c50d (http://127.0.0.1:8000/wasm-bindgen-test_bg.wasm:wasm-function[9230]:0x644f6f)
            at __wbg_adapter_64 (http://127.0.0.1:8000/wasm-bindgen-test:309:10)

---- input_parser::tests::test_external_parsing_lnurlp_first_response output ----
    JS exception that was thrown:
        Error: Unrecognized input type
            at imports.wbg.__wbindgen_error_new (http://127.0.0.1:8000/wasm-bindgen-test:1070:21)
            at sdk_common-0e2972322936c71e.wasm.__wbindgen_error_new externref shim (http://127.0.0.1:8000/wasm-bindgen-test_bg.wasm:wasm-function[20745]:0x720e87)
            at sdk_common-0e2972322936c71e.wasm.wasm_bindgen::JsError::new::hd2e35b77ed3041d8 (http://127.0.0.1:8000/wasm-bindgen-test_bg.wasm:wasm-function[12491]:0x6a22bf)
            at sdk_common-0e2972322936c71e.wasm.<core::result::Result<(),E> as wasm_bindgen_test::__rt::Termination>::into_js_result::{{closure}}::h31537578390b5f52 (http://127.0.0.1:8000/wasm-bindgen-test_bg.wasm:wasm-function[3019]:0x4cf863)
            at sdk_common-0e2972322936c71e.wasm.core::result::Result<T,E>::map_err::h259c253d9c7c5cbd (http://127.0.0.1:8000/wasm-bindgen-test_bg.wasm:wasm-function[7480]:0x5fef42)
            at sdk_common-0e2972322936c71e.wasm.<core::result::Result<(),E> as wasm_bindgen_test::__rt::Termination>::into_js_result::h42b88e6f5f052ddd (http://127.0.0.1:8000/wasm-bindgen-test_bg.wasm:wasm-function[12792]:0x6a8f03)
            at sdk_common-0e2972322936c71e.wasm.wasm_bindgen_test::__rt::Context::execute_async::{{closure}}::ha848c2cec0289637 (http://127.0.0.1:8000/wasm-bindgen-test_bg.wasm:wasm-function[2720]:0x4abd62)
            at sdk_common-0e2972322936c71e.wasm.<wasm_bindgen_test::__rt::TestFuture<F> as core::future::future::Future>::poll::{{closure}}::{{closure}}::h9c7ec505bdf6b1f7 (http://127.0.0.1:8000/wasm-bindgen-test_bg.wasm:wasm-function[7579]:0x603595)
            at sdk_common-0e2972322936c71e.wasm.wasm_bindgen::convert::closures::invoke0_mut::h5c7b6a0834f2c50d (http://127.0.0.1:8000/wasm-bindgen-test_bg.wasm:wasm-function[9230]:0x644f6f)
            at __wbg_adapter_64 (http://127.0.0.1:8000/wasm-bindgen-test:309:10)

failures:

    input_parser::tests::test_external_parsing_bitcoin_address_and_bolt11
    input_parser::tests::test_external_parsing_lnurlp_first_response

test result: FAILED. 43 passed; 2 failed; 0 ignored; 0 filtered out; finished in 0.45s

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added docs and fixed tests 17c1765

Can you pull and test safari for me? I get errors starting the tests. I added make wasm-test-safari

Running headless tests in Safari on `http://127.0.0.1:54724/`
Try find `webdriver.json` for configure browser's capabilities:
Ok
driver status: signal: 9 (SIGKILL)                
Error: http://127.0.0.1:54724/session: status code 500

Copy link
Contributor

Choose a reason for hiding this comment

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

I get the same error when trying to run headless. Running on the browser works fine.

$(CLANG_PREFIX) cargo clippy --target=wasm32-unknown-unknown -- -D warnings -A clippy::uninlined-format-args

wasm-test:
$(CLANG_PREFIX) wasm-pack test --headless --firefox
Copy link
Contributor

Choose a reason for hiding this comment

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

I get the same error when trying to run headless. Running on the browser works fine.

@dangeross dangeross force-pushed the savage-wasm-sdk-common branch 5 times, most recently from 4d906a6 to 48a9b5d Compare February 22, 2025 08:32
@dangeross dangeross force-pushed the savage-wasm-sdk-common branch from 48a9b5d to 3c0df21 Compare February 22, 2025 09:24
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.

WASM: sdk-common
4 participants