[WIP] Payjoin receiver#2011
Conversation
benalleng
force-pushed
the
payjoin-receiver
branch
4 times, most recently
from
6c93104 to
943eb03
Compare
benalleng
force-pushed
the
payjoin-receiver
branch
from
f24e841 to
00c9c51
Compare
benalleng
force-pushed
the
payjoin-receiver
branch
4 times, most recently
from
b6b17ff to
f53d60a
Compare
benalleng
force-pushed
the
payjoin-receiver
branch
5 times, most recently
from
d0f60d0 to
da7ff64
Compare
xstoicunicornx
left a comment
xstoicunicornx
left a comment
There was a problem hiding this comment.
I know this is still a WIP and my comments may be a bit premature but just wanted to brain dump some of the notes I took as part of the BOSS Payjoin Showcase session.
Some additional general comments:
- should amount be added to URI?
- should adjusting expiration will be added to settings?
- when I ran this PR Liana panicked with this error when I tried to send a payjoin to the receiver:
2026-04-09T16:19:40.194941Z ERROR liana_gui:82: panic occurred at line 508 of file lianad/src/database/sqlite/mod.rs: Some("database must be available: SqliteFailure(Error { code: Unknown, extended_code: 1 }, Some(\"table payjoin_outpoints has no column named added_at\"))") 0: backtrace::backtrace::libunwind::trace - in the GUI when payjoin is listed under previously generated addresses it shows the address rather than URI (I'm guessing this is related to this comment)
| /* Payjoin OHttpKeys */ | ||
| CREATE TABLE payjoin_ohttp_keys ( | ||
| id INTEGER PRIMARY KEY NOT NULL, | ||
| relay_url TEXT UNIQUE NOT NULL, |
There was a problem hiding this comment.
Should this be the payjoin directory url instead? Aren't OHttpKeys specific to a payjoin directory, not relay?
If sopayjoin_save_ohttp_keys and payjoin_get_ohttp_keys also would need to be updated.
There was a problem hiding this comment.
Bumping this, aren't the ohttp keys specific to the payjoin directory?
| } | ||
| } | ||
|
|
||
| pub(crate) fn fetch_ohttp_keys( |
There was a problem hiding this comment.
Is it possible to reuse PDK's implementation of fetch_ohttp_keys?
There was a problem hiding this comment.
Yes, but the current PDK impl is async so we would need to add some blocking capability and without it the async stuff would cascade. So i think its best to keep it manually re-implemented. though It might be nice to add sync versions for situations like this
| db_conn: &mut Box<dyn DatabaseConnection>, | ||
| secp: &secp256k1::Secp256k1<secp256k1::VerifyOnly>, | ||
| ) -> Result<(), Box<dyn Error>> { | ||
| let proposal = proposal.apply_fee_range(None, None).save(persister)?; |
There was a problem hiding this comment.
No fee range actually being applied.
|
Thanks for the feedback, Regarding the UI notes.
Since it is not a mandatory addition in the payjoin spec I thought that for a desktop on-chain only wallet the UX of adding an amount on the receive side is sort of unfamiliar, especially given the current liana receiver UI
I think again perhaps this is worth some user research but with the ability to do a fallback tx at any point I am not sure that extending the expiration as an option is great to put in the user's direct control
This was something we landed on due wanting to discourage reuse of payjoin sessions. I could share this bip21 with as many people as I want similar to address reuse but instead of just bad privacy you would get a bunch of payjoin failures The method again is a holdout as the last time I gave this some serious focus I transitioned from storing the bip21 to the session ID directly and I previously wanted to have access to the bip21 in the db where I just generate it in the gui now and don't store it directly |
This is definitely an artifact of me playing in the database between naming the timestamp |
Familiarity might not be an issue IMO, since Liana users presumably also use other (esp mobile) wallets that have this. IMO it's more about the use case or user type: how often do they want to receive an specific exact amount? That said, this seems like a reasonable starting point.
My hunch is, Liana users are likely to be sophisticated enough to use such a feature properly. Perhaps there could be appropriate messaging/guidance about using the feature. |
benalleng
force-pushed
the
payjoin-receiver
branch
2 times, most recently
from
aac7f6e to
06623c7
Compare
| let txid = psbt.unsigned_tx.compute_txid(); | ||
| if let Some(psbt) = db_conn.spend_tx(&txid) { | ||
| let mut is_signed = false; | ||
| for psbtin in &psbt.inputs { | ||
| if !psbtin.partial_sigs.is_empty() { | ||
| log::debug!("[Payjoin] PSBT is signed!"); | ||
| is_signed = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if is_signed { | ||
| let proposal = proposal | ||
| .finalize_proposal(|_| { | ||
| let mut psbt = psbt.clone(); | ||
| finalize_psbt(&mut psbt, secp); | ||
| Ok(psbt) | ||
| }) | ||
| .save(persister)?; | ||
|
|
||
| send_payjoin_proposal(proposal, persister)?; | ||
| } |
There was a problem hiding this comment.
I couldn't quite figure out what the intention was here with is_signed, however during my testing is_signed never gets set to true and the proposal never gets finalized. Can you explain what validation is being done here?
Also, when I replaced |_| with |psbt| in the finalize_psbt closure it was still possible to clone as mut, whats the rationale for calling psbt_to_sign outside of this closure?
Something else that may be related is that currently psbt_to_sign doesn't return a psbt with sender sigs removed, this is something I am fixing in payjoin/rust-payjoin#1446. Let me know if it would good for me to create a separate smaller PR for this just to get this fixed sooner.
I did also try hardcoding is_signed to true so the receiver does send out a psbt proposal, but its then erroring on the sender side during validation. So the finalize_psbt isn't quite working right, but that may be because the psbt thats being finalized has the leftover sender sigs still.
Apologies this was due to my incomplete understanding of the PSBT signing workflow
|
Just started looking at this again. Good job with the broadcast fallback tx button, I like it. Couple quick notes about it:
|
|
I think I will want to try and use the latest commit for rust-payjoiin that includes payjoin/rust-payjoin#1470 so that I can just have |
benalleng
force-pushed
the
payjoin-receiver
branch
from
a3a207d to
c4bcb80
Compare
|
sure we'll cut another release that includes the new function asap |
There was a problem hiding this comment.
Looking much better :)
General feedback:
TheBroadcast fallbackbutton should be available prior to signing PSBTStill getting error when broadcasting fallback transaction (maybe related to this comment?)- Should user be allowed to specify multiple relays?
- Transactions display for payjoin receive tx should be more aligned with existing Liana receive txs (example screenshots below):
- it is classified as outgoing transaction
- only our coins should show as spent (right now includes senders coins)
- our received payment output should be classified as payment output (right now it shows as change)
- senders change output should show as external output
- If we only show the bip21 initially when clicking
Receive Payjoindoes it need to be stored as part of gui app receive state? - Somewhat related and contradictory to last point, should bip21 be displayed until expiry?
- Lots of
unimplemented!s in liana-gui/src/app/state/receive.rs - Should we pro-actively validate that key's can be fetched from payjoin directory when it is updated in settings? Not sure how feasible that is with this UI framework
Questions:
- I couldn't follow the purpose of changing the
descs: &[descriptors::SinglePathLianaDesc]parameters todesc: &descriptors::LianaDescriptorin the poller, can you help me follow why this was needed?
Edit: Broadcast fallback is working appropriately - I neglected to build on latest commit when testing sorry.
Regular receive tx:
Payjoin receive tx:
| .address(self.config.bitcoin_config.network); | ||
|
|
||
| let persister = ReceiverPersister::new(Arc::new(self.db.clone()), new_index.into(), ""); | ||
| let session = ReceiverBuilder::new(address.clone(), payjoin_dir_url, ohttp_keys) |
There was a problem hiding this comment.
Is there possibility that ohttp_keys on payjoin directory have been rotated more recently than the 7 days for which the ohttp keys have been stored here? If so there probably needs to be handling for this.
Some options:
- Maybe don't store ohttp keys at all? Fetching shouldn't be happening that frequently and would simplify code
- Try refetching keys if
RecieverBuilder::new()returns error
Also mapping the error to a CommandError::IntoUrlError by default feels weird since the pj directory url ideally should have already been validated upstream.
| } | ||
| } | ||
|
|
||
| impl std::error::Error for PersisterError {} |
| UnexpectedStatusCode(reqwest::StatusCode), | ||
| } | ||
|
|
||
| impl std::error::Error for FetchOhttpKeysError {} |
| .next() | ||
| .map(|e| e.to_string()) | ||
| .unwrap_or_default(), | ||
| ) | ||
| })?; | ||
| for index in 0..spend_psbt.inputs.len() { | ||
| match spend_psbt.finalize_inp_mut(&self.secp, index) { | ||
| Ok(_) => log::debug!("Finalizing input at: {}", index), | ||
| Err(e) => { | ||
| // If the input is already finalized (e.g. a payjoin sender input that | ||
| // arrived with final_script_witness already set), ignore the error. | ||
| // Otherwise, the transaction can't be broadcast — return an error. | ||
| let input = &spend_psbt.inputs[index]; | ||
| if input.final_script_witness.is_none() && input.final_script_sig.is_none() { | ||
| return Err(CommandError::SpendFinalization(e.to_string())); | ||
| } | ||
| log::debug!( | ||
| "Input at index {} already finalized, skipping: {}", | ||
| index, | ||
| e | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Why are we touching broadcast_spend? I don't think there is a payjoin flow that requires this?
Broadcasting the fallback transaction shouldn't require finalizing the inputs, and it would probably be cleaner to implement separate broadcast_payjoin_fallback if necessary.
There was a problem hiding this comment.
Good point, I was hoping that I could tie into some existing logic. but you're probably right these are best kept separate.
| @@ -19,9 +17,10 @@ use liana::{ | |||
| }, | |||
| spend::{SpendCreationError, DUST_OUTPUT_SATS, MAX_FEERATE}, | |||
| }; | |||
| use lianad::commands::ListCoinsEntry; | |||
| use lianad::{commands::ListCoinsEntry, payjoin::types::PayjoinStatus}; | |||
|
|
|||
| use liana_ui::{component::form, widget::Element}; | |||
| use payjoin::Uri; | |||
|
|
|||
| use crate::{ | |||
| app::{ | |||
| @@ -971,7 +970,6 @@ impl Recipient { | |||
| } | |||
| } | |||
| view::CreateSpendMessage::RecipientFiatAmountEdited(_, fiat_amt_str, converter) => { | |||
| self.amount.warning = None; // Clear any warning on the BTC amount as it is no longer the last edited field. | |||
| self.fiat_converter = Some(converter); | |||
| if fiat_amt_str.is_empty() { | |||
| self.fiat_amount = Some(form::Value::default()); | |||
| @@ -1019,25 +1017,7 @@ impl Recipient { | |||
| view::CreateSpendMessage::RecipientEdited(_, "amount", amount) => { | |||
| self.fiat_amount = None; // Clear any fiat amount to indicate BTC amount is now primary. | |||
| self.fiat_converter = None; | |||
| self.amount.warning = None; | |||
|
|
|||
| // If a float has been passed with more than 8 decimal places, and truncating it | |||
| // to 8 decimal places would result in a valid BTC amount, we truncate it and show a warning. | |||
| // This can only happen if the user pastes an amount, as the input only allows up to 8 decimal places. | |||
| // Otherwise, keep the full string value. | |||
| self.amount.value = f64::from_str(&amount) | |||
| .ok() | |||
| .and_then(|_| amount.split_once('.')) | |||
| .filter(|(_, fraction)| fraction.len() > 8) | |||
| .map(|(integer, fraction)| format!("{}.{}", integer, &fraction[..8])) | |||
| .filter(|truncated| { | |||
| Amount::from_str_in(truncated, Denomination::Bitcoin).is_ok() | |||
| }) | |||
| .inspect(|_| { | |||
| self.amount.warning = Some("Amount has been truncated to 8 decimal places"); | |||
| }) | |||
| .unwrap_or(amount); | |||
|
|
|||
| self.amount.value = amount; | |||
| if !self.amount.value.is_empty() { | |||
| self.amount.valid = self.amount().is_ok(); | |||
| } else { | |||
| @@ -1058,7 +1038,7 @@ impl Recipient { | |||
| i: usize, | |||
| is_max_selected: bool, | |||
| fiat_converter: Option<&view::FiatAmountConverter>, | |||
| ) -> Element<'_, view::CreateSpendMessage> { | |||
| ) -> Element<view::CreateSpendMessage> { | |||
| let mut fiat_form_value = self.fiat_amount.as_ref(); | |||
|
|
|||
| // If we have a fiat converter, check if it has changed since the last time we set | |||
| @@ -1112,13 +1092,31 @@ impl SaveSpend { | |||
| impl Step for SaveSpend { | |||
| fn load(&mut self, _coins: &[Coin], _tip_height: i32, draft: &TransactionDraft) { | |||
| let (psbt, warnings) = draft.generated.clone().unwrap(); | |||
|
|
|||
| let recipient = draft.recipients.first().expect("one recipient"); | |||
| let bip21 = format!( | |||
| "bitcoin:{}?amount={}", | |||
| recipient.address.value, recipient.amount.value | |||
| ); | |||
|
|
|||
| let payjoin_status = if let Ok(uri) = Uri::try_from(bip21.as_str()) { | |||
| if uri.assume_checked().extras.pj_is_supported() { | |||
| Some(PayjoinStatus::Pending) | |||
| } else { | |||
| None | |||
| } | |||
| } else { | |||
| None | |||
| }; | |||
|
|
|||
| let mut tx = SpendTx::new( | |||
| None, | |||
| psbt, | |||
| draft.inputs.clone(), | |||
| &self.wallet.main_descriptor, | |||
| &self.curve, | |||
| draft.network, | |||
| payjoin_status, | |||
There was a problem hiding this comment.
Does this need to part of receive payjoin pr?
benalleng
force-pushed
the
payjoin-receiver
branch
from
a49282a to
d22186a
Compare
benalleng
force-pushed
the
payjoin-receiver
branch
from
c06ac3c to
b3c70ee
Compare
5 participants
This adds support for payjoin receive in liana.
This is currently WIP and has the following TODOs