-
Notifications
You must be signed in to change notification settings - Fork 76
Add javascript language bindings #1190
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
Conversation
Pull Request Test Coverage Report for Build 19553229768Details
💛 - Coveralls |
1aa3218 to
b7e4521
Compare
d835e8b to
dcae416
Compare
DanGould
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only reviewed the first three commits so far and wanted to post it rather than sit on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these patched separately from the top level workspace patches? Do you know what unpatches them to require this second patch file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, adding this comment to the patch file for clarity:
ubrn buildgenerates a Rust wasm crate in rust_modules/wasm, withwasm-bindgenannotated functions.
That crate is used to generate TS bindings which call into thewasm-bindgenfunctions.
Because that generated crate doesn't belong to thepayjoinworkspace, we need to patch the generated Cargo.toml accordingly for local development.
The patch is applied via the ubrn.config.yamlmanifestPatchFileoption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
making brew and npm dependencies definitely seems WAY hackier than using a flake which defines these llvm, llvm-ar, and clang dependencies.
I'm not opposed to supporting preliminary scaffolding this way but I'd want to fix this sooner than later. Can see the mistake of easily building on "works on my machine" building up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love the llvm hack but replacing the brew dependency with a nix dependency seems ~equivalent, and most macOS devs already use homebrew. If the concern is reproducible builds I guess nix is better but we haven't really committed to using nix in the rest of the project or in any CI workflows, so I'm a bit reluctant to introduce it here as a dependency without using nix for other workflows myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I see this is not installing anything, it's just calling deps documented as requirements. The only difference with making nix a requirement would be removing the brew --prefix I suppose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know web_time already does the cfg switching, but if these two files (time.rs and receive/v2/mod.rs) are the only places we're importing time types I do prefer keeping the cfg switching in our code as a double sanity check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a clarifying code comment for posterity.
|
|
||
| const senderPersister = new InMemorySenderPersister(1); | ||
| const psbt = | ||
| "cHNidP8BAHMCAAAAAY8nutGgJdyYGXWiBEb45Hoe9lWGbkxh/6bNiOJdCDuDAAAAAAD+////AtyVuAUAAAAAF6kUHehJ8GnSdBUOOv6ujXLrWmsJRDCHgIQeAAAAAAAXqRR3QJbbz0hnQ8IvQ0fptGn+votneofTAAAAAAEBIKgb1wUAAAAAF6kU3k4ekGHKWRNbA1rV5tR5kEVDVNCHAQcXFgAUx4pFclNVgo1WWAdN1SYNX8tphTABCGsCRzBEAiB8Q+A6dep+Rz92vhy26lT0AjZn4PRLi8Bf9qoB/CMk0wIgP/Rj2PWZ3gEjUkTlhDRNAQ0gXwTO7t9n+V14pZ6oljUBIQMVmsAaoNWHVMS02LfTSe0e388LNitPa1UQZyOihY+FFgABABYAFEb2Giu6c4KO5YW0pfw3lGp9jMUUAAA="; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose these test vectors really need to come from a common utils library or common fixtures (and perhaps they do in following commits?) because otherwise they all need to be crosschecked in each language.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I will address this in a follow-up as there are other things I'd like to cleanup across all language bindings.
DanGould
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went through the whole thing. Your wrangling with uniffi-bindgen-react-native gets me so excited about our bindings story because 1. I know it's in talented hands and 2. javascript is such a pain compared to other languages, and this wasn't an impossible review.
I don't think there are any major offenders, so this could be merged as-is but I'd prefer if my comments were addressed before that
| # Heinous hack to pin a transitive dependency to be MSRV compatible on 1.85 | ||
| cd node_modules/uniffi-bindgen-react-native | ||
| cargo add home@=0.5.11 --package uniffi-bindgen-react-native | ||
| cd ../.. | ||
|
|
||
| rustup target add wasm32-unknown-unknown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would a not heinous 'proper' way to do this be? If known, it'd be cool to doc in the comment here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Rust had a sane way to manage MSRV it might help other library providers not accidentally breaking compatibility, but as it stands the only alternative I can think of is to ignore MSRV in payjoin-ffi. Though that might break shared dependencies in the payjoin workspace, so we'd have to exclude payjoin-ffi from the workspace.
| - name: "Install dependencies" | ||
| run: npm ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thebrandonlucas is npm the thing to use or is there another javascript package manager we should be using? Bun?
TBF, it seems like js package managers is a rotating fashion choice and I think npm is lindy and fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it ever be possible to make these bindings with uniffi or is this a wontfix issue? If this works, it's test utils and it seems like a fine hack as long as it's a documented decision, which it seems to be in the commit message.
Though I do wonder if integrations like Bull Bitcoin would use node bindings that allow I/O in closures (if wasm doesn't already)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it ever be possible to make these bindings with uniffi or is this a wontfix issue?
If we make all test-utils not require io, e.g. by replacing requests/responses with hardcoded mock values, we could just compile test utils to wasm. For the time being I consider it wontfix since this workaround seems acceptable for testing.
Not sure what you mean by allowing I/O in closures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole reason to do node.js was for native io. I wonder if native IO works if it's done in node but the interface is wasm for purposes of Bull Bitcoin exchange's persistence. I bet it would still work because the closure is just a function pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, yes I think that works for the reason you mentioned. FWIW we could just generate native node bindings using napi-rs like we did for test-utils, but I think uniffi-bindgen-react-native gives us more options if we want to extend the bindings to web wallets or React Native in the future while keeping with our uniffi-all-the-things approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only 570 lines is impressive stuff!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: put this commit before the offending commit so that no intermediate commits fail ci
|
This is some awesome work, gonna see if there's anything I can catch, probably minor things |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new README under javascript/ still opens with “Payjoin Dart Bindings”
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks!
This includes: - package.json with uniffi-bindgen-react-native and other relevant dependencies. It also includes several npm commands to generate the bindings. - A minimal ubrn.config.yaml to generate bindings for node.js. - tsconfig.json instructions for the `tsc` compiler. - wasm-manifest-patch.toml ensures that the generated WASM crate uses the local payjoin crates for development. - fix-imports.js is a sad workaround for node.js-compatible imports. - generate_bindings.sh is a helper following the pattern established in python/ and dart/ bindings. For MacOS it's necessary to use llvm's clang because Apple's clang doesn't have a WASM backend.
This is a drop-in replacement for compatibility with WASM targets wherever core components use std::time. `io` is already incompatible with WASM due to the tokio `net` dependency, so I left that import of `std::time` unchanged.
Robot ported these from the python tests.
The `io` feature is incompatible with WASM due to the tokio `net` dependency.
uniffi-dart has a built-in tokio dependency which is incompatible with WASM. We should investigate whether that dependency is necessary for uniffi-dart or how it might be feature-gate, but for now just feature gate uniffi-dart here.
This allows conditionally enabling the `getrandom/js` feature flag to avoid bloat on other platforms https://docs.rs/getrandom/latest/getrandom/#webassembly-support. getrandom is already an implicit dependency and unfortunately Cargo doesn't support enabling feature flags on transitive dependencies directly, so it must be added to the explicit dependencies.
payjoin-test-utils requires native I/O for networking operations, process spawning, and filesystem operations. WASM cannot support these, so the JS payjoin bindings do not include test utils (or any io for that matter). This is problematic for writing integration tests. This commit introduces a new, thin binding layer exclusively for payjoin-test-utils, that compiles to a native Node.js add-on. The production code stays as WASM (via uniffi-bindgen-react-native) for cross-platform compatibility, and the test infrastructure is available via platform-specific binaries generated by napi-rs.
dcae416 to
cf265f8
Compare
|
There are still two open checks here, do they need their own issues? or tracking? |
|
Good point, made an issue for |
This PR adds javascript bindings to payjoin-ffi via uniffi-bindgen-react-native. I highly recommend reviewing commit by commit for it to make sense. Claude came in clutch for porting over the unit/integration tests from other language bindings, and also for writing the test-utils wrapper via napi-rs. The remaining TODOs below can be left as follow-ups IMO.
TODOs:
netdependency.iofeature is disabled, so ideally the JS bindings should offer an alternative for fetching OHTTP keys.iofunctionality #1250modulevalues conflict jhugman/uniffi-bindgen-react-native#324Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.