Upgrade payjoin#2041
Conversation
|
Is payjoin 0.25 the new release number for what was 1.0.0 before? |
| path: "../rust-payjoin/payjoin-ffi/dart" | ||
| relative: true | ||
| source: path | ||
| version: "0.1.0" |
There was a problem hiding this comment.
rust-payjoin@993f3b8b240e29d46c5ecc1eb9d11958b3b2002a: sources
|
We've got 1.0 rc's but still have some TODOs on the milestone. The rcs aren't discoverable on docs.rs, so we decided to cut another minor release and continue to do so until we're way more certain 1.0 is right around the corner (like 1 week away). |
|
Hello guys, thanks for the contribution I've had to fork bdk-dart to target my own fork of uniffi that include the fix for reproducible builds, this fix is merged upstream. Anytime soon we should get a new tag/release from uniffi that include it and will be required for our native assets dependencies |
| final pjUri = initialized.pjUri().asString(); | ||
| // Derive the receiver ID from pjUri | ||
| final id = sha256.convert(utf8.encode(pjUri)).toString().substring(0, 16); | ||
|
|
There was a problem hiding this comment.
This seems like it may be both a regression and divergence from the state of the art in our reference implementation. There we're generating session IDs from random because it's possible that multiple attempts could be made with the same URL. At least I think that's what the rationale is here:
I think if the ID were to come from the persister implementation after the .save call that might match, but I do understand that it's also imperative messiness to expect that state after save rather than return it from .save(). I question why the model needs this to begin with.
There was a problem hiding this comment.
The model itself probably doesn't need it, seems like it could just be a DB concern to auto-generate primary keys for pj sessions. I think the same answer as #2041 (comment) applies here.
| receiver: persister.toJson(), | ||
| walletId: walletId, | ||
| pjUri: (await receiver.pjUri()).asString(), | ||
| pjUri: pjUri, |
There was a problem hiding this comment.
I wonder why ~receiver: persister and why pjUri is saved rather than the whole receiver. This doesn't seem like a clean change, but it may just be an intermediate step still (if so, a FIXME comment in the in-between code that gets removed in the later commit in the same PR would make this easier to review).
There was a problem hiding this comment.
pjUri is a legacy field I didn't want to touch here yet so yeah I consider this an intermediate step until the Receiver/Sender Models are refactored for proper session event log support.
| pj.WithReplyKey? withReplyKey; | ||
| try { | ||
| withReplyKey = sendBuilder | ||
| .buildRecommended(minFeeRate: minFeeRateSatPerKwu) |
There was a problem hiding this comment.
nit: if this minFeeRate parameter is not strongly typed internalizing units then the unit belongs in the name of the parameter. I see this is not a regression but it is something for us to note before merge.
There was a problem hiding this comment.
also adding here as a reminder: there are a few places in the FFI API with awkward naming, particularly the InputPair-related structs: PlainTxIn, PlainOutPoint, PlainPsbtInput, PlainTxOut. These are primitive representations of bitcoin types which are then validated in payjoin-ffi, but those names are confusing from an outsider perspective. I think we could just drop the Plain prefix and make it clear they expect primitive types in the respective fields.
| uri: uri.asString(), | ||
| uri: parsedUri.asString(), | ||
| isTestnet: isTestnet, | ||
| sender: senderJson, | ||
| sender: persister.toJson(), |
There was a problem hiding this comment.
same question as receive side. Why is sender: persister and if we're passing the real sender why even pass the uri as a separate field?
| chosen = inner.tryPreservingPrivacy(candidateInputs: candidates); | ||
| } catch (e) { | ||
| throw StateError('No inputs available to contribute to payjoin'); | ||
| } |
There was a problem hiding this comment.
This is slightly different behavior than the branch you're building off of. BBMobile Asked us to use candidateInputs.first on this error condition rather than throw an error for the time being.
separating behavior changes from the upgrade
There was a problem hiding this comment.
try_preserving_privacy internally falls back to the first candidate input for convenience if no UIH-defeating input is found, so the behavior is the same regardless. This may be a footgun for implementers though, not sure that we have the right trade-off here.
| payjoin: | ||
| path: ../rust-payjoin/payjoin-ffi/dart |
There was a problem hiding this comment.
| payjoin: | |
| path: ../rust-payjoin/payjoin-ffi/dart | |
| payjoin: | |
| git: | |
| url: https://github.com/payjoin/rust-payjoin | |
| ref: 993f3b8b240e29d46c5ecc1eb9d11958b3b2002a | |
| path: payjoin-ffi/dart |
| break; | ||
| return (ohttpKeys, ohttpRelayUrl); | ||
| } catch (e) { | ||
| continue; |
There was a problem hiding this comment.
prevent swallowing the errors? log('fetchOhttpKeys via $ohttpRelayUrl failed: $e') I don't think this is the only place we're losing error info, but it's not a regression so it could be info overload.
| log('[Senders Isolate] Received response from ${req.url.asString()}'); | ||
| class OhttpRelaysUnavailableException extends BullException { | ||
| OhttpRelaysUnavailableException(super.message); | ||
| } |
|
What flutter version are you on? I'm on but getting what seem to be flutter version-related failures when I build. I think bbmobile pins 3.38.5 trying the android emulator locally and hitting exhaustive-switch errors in recoverbull/recipients/key_server_status_widget (amongst many other issues) against Flutter 3.38.5. |
|
@DanGould You may experience a global unhandled error related to Boltz testnet deprecated that will be fix |
|
First off, @ethicnology I was trying to build on 2025 Second these passed afaict: 02:07 +1 -2: Payjoin Integration Tests with one receive and one send should fail if the receiver does not have enough These are the problems I faced running the test: Android blocker No. 1: ElectrumClient::new fails with Failed to install CryptoProvider — rustls/TLS init issue in bdk_dart/bdk-ffi v2.3.1, sync never succeeds. Log2026-04-24T14:30:44.991269 SEVERE sync wallet error with electrum server: ssl://blockstream.info:993 CouldNotCreateConnectionElectrumException(unexpected error: Faile
d to install CryptoProvider) #0 checkCallStatus (package:bdk_dart/bdk.dart:24814:5)
#1 rustCall (package:bdk_dart/bdk.dart:24837:5)
#2 new ElectrumClient (package:bdk_dart/bdk.dart:20680:14)
#3 _performFullScan (package:bb_mobile/core/wallet/data/datasources/bdk_wallet_datasource.dart:717:26)
<asynchronous suspension>
#4 _RemoteRunner._run (dart:isolate:1118:18)
<asynchronous suspension>Android blocker No. 2: first happy-path test throws SeedNotFoundException('Seed not found for fingerprint: 858ab5df') from send_with_payjoin_usecase.dart:45, after 4 null reads over ~4.5s from secure storage. Same fingerprint reads fine in subsequent tests. Mechanism unclear — may be a persistence gap in walletRepository.createWallet for the second wallet, or a storage timing issue/race? Log2026-04-24T14:31:00.931014 FINE Seed read returned null for fingerprint 858ab5df on attempt 1, retrying in 300ms
2026-04-24T14:31:01.248030 FINE Seed read returned null for fingerprint 858ab5df on attempt 2, retrying in 600ms
2026-04-24T14:31:01.852582 FINE Seed read returned null for fingerprint 858ab5df on attempt 3, retrying in 1200ms
2026-04-24T14:31:03.067069 FINE Seed read returned null for fingerprint 858ab5df on attempt 4, retrying in 2400ms
01:54 +0 -2: Payjoin Integration Tests with one receive and one send should work with one receiver and one sender [E]
Seed not found for fingerprint: 858ab5df
package:bb_mobile/core/payjoin/domain/usecases/send_with_payjoin_usecase.dart 45:7 SendWithPayjoinUsecase.execute
===== asynchronous gap ===========================
package:stream_channel _GuaranteeSink.add
var/folders/f8/bx_b4w_90hl17m6ynl85nc0r0000gn/T/flutter_tools.5pAZVh/flutter_test_listener.45KaLB/listener.dart 56:22 main.<fn> |
|
Can you attempt a Also, on develop we have |
There was a problem hiding this comment.
My test is hanging at the process proposal step but I cannot really find a good way to expose the error that is happening as all we get is did not complete
2026-04-29T09:37:18.846543 INFO Processing payjoin request: 4a25f4cb99a44221
01:19 +1: Payjoin Integration Tests with one receive and one send should work with one receiver and
one sender
Payjoin event for 4a25f4cb99a44221: PayjoinStatus.requested
01:20 +1: Payjoin Integration Tests with one receive and one send should work with one receiver and
one sender - did not complete [E]
To get to this point I also had to completely remove the proxy behavior in the http.dart on the rust-payjoin side and just get the keys from the directory directly. I missed that is the point of payjoin/rust-payjoin#1500
I manually got rid of freezed files which did it but will do this in the future
arm64 Apple Silicon baby got tests running based on your spec @spacebear21 but need human-in-the-loop to get into this PR DanGould@72b0b65 |
6e4f1cf to
f1e786a
Compare
cherry-picked into this PR and confirmed all payjoin tests are passing locally |
DanGould
left a comment
There was a problem hiding this comment.
As far as I can tell the review back-and-forth between @spacebear21 and I is satisfied and we need feedback from Bull Bitcoin @ethicnology. While this passes tests and would work, we probably want to follow this up with a polish to the sender/receiver models, as noted by @spacebear21, and are ultimately blocked from hitting the merge button on this by an upstream PR payjoin/rust-payjoin#1500.
f1e786a to
0cb6c03
Compare
Switch from payjoin_flutter to dart payjoin bindings, which are actively maintained and support the latest rust-payjoin versions.
These pre-load the wallet and return a synchronous callback compatible with the synchronous payjoin interface, for isMine and signPsbtSync.
These session persisters hold payjoin events in memory as a transitive step, so that DB migrations and complete event persistence may be implemented in a follow-up step.
Implements a chaining pattern with processReceiveSession to process and advance a session from any state to its terminal state. BBM needs the proposal PSBT to save to its model, so it needs to be extracted before transitioning to the Monitor typestate to be returned alongside the session.
This should be droppable once isolates architecture is replaced
Move receiver/sender polling onto the main isolate, keyed by session idin two Timer.periodic maps. The old isolate indirection existed because frb async FFI could block the UI isolate. With sync uniffi, `Timer.periodic` on the main isolate works and removes ~150 lines of accidental complexity. Co-Authored-By: Dan Gould <d@ngould.dev> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
0cb6c03 to
8f924ad
Compare
|
Updated the payjoin dependency to use the published package at https://pub.dev/packages/payjoin instead of a local path |
|
|
||
| // Build the psbt with the sender wallet | ||
| const amountSat = 10000; | ||
| const networkFeesSatPerVb = 1000.0; |
There was a problem hiding this comment.
We really should reconsider this as the feerate. whether here or as a followup in #1978
Working branch for updating to payjoin 0.25 with native Dart bindings