payjoin
Add Receiver.new_with_time
WASM bindings need to be able to pass in current time, WASM context doesn't have access to system level IO
Not in all targets, but in web targets (wasm32-unknown-unknown?) the browser has a way to get the time. We can use an optional web-time dependency to get this https://docs.rs/web-time/latest/web_time/
interesting
but WASM works in ReactNative and Node.js envs aswell, and it looks like this doesn't solve for that(?). On the bindings side I'm using web_sys::js_sys::Date and I know that works in all desired envs. maybe they are the same under the hood? maybe we could use web_sys::js_sys::Date in rust-payjoin if SystemTime::now() panics?
BDK exposes versions of functions that take time with _at appended (ex: apply_update => apply_update_at) so I figured we could follow a similar pattern, but I'm not sure what the optimal approach is. Would be great to test web-time in RN and Node envs, but want to just focus on getting the core WASM bindings functional rn
I think for Node.js we couldd target wasm32-wasi instead of wasm32-unknown-unknown, which has access to std::time, and otherwise when wasm32-unknown-unknown. I think web-time handles this. They show some CI tests for node support
The story for ReactNative is less clear to me. Talking to Zeus it seemed like they would rather consume swift/kotlin from uniffi and bind to that rather than use wasm. We may need to bring in a specific implementer to discover there preferences for sure. Either way I think we can keep it simple now and prove the concept in browser without new_with_time
I know MetaMask is using bdk-wasm in ReactNative, so it's just an option for that we have available now. Yeah it'd be great to have swift and kotlin bindings and all like for bdk but I think it's worth supporting.
Anyway, I'll make a pr for the web_time version and we can go with that for now
Pull Request Test Coverage Report for Build 14065854965
Warning: This coverage report may be inaccurate.
This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
- For more information on this, see Tracking coverage changes with pull request builds.
- To avoid this issue with future PRs, see these Recommended CI Configurations.
- For a quick fix, rebase this PR at GitHub. Your next report should be accurate.
Details
- 16 of 16 (100.0%) changed or added relevant lines in 1 file are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage increased (+0.04%) to 81.434%
| Totals | |
|---|---|
| Change from base Build 14044917943: | 0.04% |
| Covered Lines: | 5101 |
| Relevant Lines: | 6264 |
💛 - Coveralls
https://github.com/payjoin/rust-payjoin/pull/596 replaces this hopefully, so we can close this pr I suppose. want to test ReactNative hopefully soon
Don't worry too much about ReactNative. It's interesting but very much not a bottleneck in the way of our forward progress the way other changes are.
On further discussion with Dan this approach is still under consideration.
The reason I originally proposed this was that it's the approach bdk takes; however, looking into it I don't believe they created the time input versions of functions for bindings specifically but for more high level bitcoin ecosystem reasons, see bdk/crates/wallet/src/wallet/mod.rs #apply_update_at the documentation on it states
/// `last_seen` value is used internally to determine precedence of conflicting unconfirmed
/// transactions (where the transaction with the lower `last_seen` value is omitted from the
/// canonical history).
So in the case for bdk I think it just so happens that they want to expose the ability to manually choose a timestamp in some cases, not that it is necessary because bindings envs (like wasm) don't allow use of rust std::time. For bdk wasm bindings it was straight forward to add js Date::now at the bindings layer,
but for payjoin it's not so clear that we want methods that take time input, the only use case rn is wasm bindings which could be solved with a feature that uses web_time instead of std::time. But this is a web specific solution and there are likely other targets for which additional solutions would be required, if so then it would be nice to handle time at the bindings layer for whatever target and then it would make sense to have a _at version of any rust-payjoin functions that require use of time::now
another thing to consider is the complexity of the feature flagging if we go with this solution, it adds quite a bit of config to the toml and code, it might be cleaner to keep all this in the bindings layer; however, the downside with that approach is that we really want to protect against any possibility of calling an unsupported function in a build for a given target, eg: wasm shouldn't even build if it depends on std::time. It would be nice to robustly protect against any such runtime errors in the config setup for targets/features etc
Steve: """ time is needed to so the bdk_chain crate knows how to order the blockdata, the apply_update_at function was added for no_std environments like wasm that don't have access to the standard system time functions. So for wasm I think you'll need to use the JS time function to get the time when applying updates. """ sounds like the _at functions are essentially for wasm
@DanGould ive come back around to thinking this is the best approach. Lowest maintenance, no extra deps, highest flexibility. I confirmed that this was the reasoning behind bdk approach to time in Wasm. I'll need to verify that no other updates are needed for the bindings to work (I kinda suspect there will be)
Yeah unclear to me if this is sufficient or if it'll throw an error because of NewReceiver::new's signature. If we have a Proof of Concept I'm convinced to move forward with this. Even better would be to solve it as in #596 but with complete feature gating
What do you mean "as in 596"? That's a totally different approach which I'm arguing is not the way to go. It's much more complex and web specific
My point is that this approach is ok now but is a step backwards in terms of ergonomics. My comment was based on the misinterpretation that new_with_time accepted a u64 unix time instead of a SystemTime, because that dependency is insufficient for wasm support as far as I understand. Assuming that is the case, eventually we'd want to get #596 to return to the prior ergonomics.
Is there a branch where this PR works with wasm as-is, or does the function parameter need to be a different suitable type?
The reason I opted to use systemTime here is that is what you get from JS-sys, how I had it set up was to expose only new in bindings but have that actually wrap new_with_time and get the time from JS-sys which is already a dependency for Wasm.
I think I'd like to rename it new_at bc that's the pattern in bdk. I'll also move time to be the first input since it feels weird to put it after optional args
@DanGould here is the commit that shows how the interface is used: https://github.com/LtbLightning/payjoin-ffi/pull/49/commits/f78480210297d09f364294cb3ffea2a6fdd3be3e Since js_sys::Date::now() returns a u64 I add it to the std::time::SystemTime::UNIX_EPOCH at the bindings level to get a SystemTime value. Would you rather send the u64 directly as the input? and add epoc in rust-payjoin? The awkward thing is that std::time::SystemTime is available in the wasm context even tho calling std::time::SystemTime::now() causes a panic (or error). So we can create a SystemTime value in WASM with no problem, we just have to get the 'now' part as a u64 from js.
I'm not exactly sure what you're referring to by 'ergonomics'. The main thing I see as potentially interesting is feature flagging std::time to not be available in wasm context, in which case the other approach using web_time crate makes more sense. however, it's significantly more complex and not worth IMO. plus that specific implementation is browser specific, I think the main use case for the wasm bindings is ReactNative and Node.
Overall I think this pr outlines the best general approach, its flexible and low maintenance, tho a bit brittle (you have to know not to call std::time::SystemTime::now). Also this is the approach BDK takes.
@DanGould I started working on a minimal first WASM pr to merge, however to update payjoin-ffi I also need to update rust-payjoin which means rebasing whichever of these time PRs we want to move forward with, so what I'm saying is that this decision is also holding up that PR (unsurprisingly)
