Use payjoin crate
#8
Conversation
evanlinjin
force-pushed
the
rust-payjoin
branch
from
913214b to
8be217a
Compare
evanlinjin
force-pushed
the
rust-payjoin
branch
from
8be217a to
69a8b20
Compare
evanlinjin
force-pushed
the
rust-payjoin
branch
from
e14a131 to
45037d0
Compare
| let reader = &*bytes; | ||
| let proposal = UncheckedProposal::from_request(reader, query, headers).unwrap(); | ||
| // extract original PSBT from HTTP request | ||
| // TODO: `UncheckedProposal` is not really the proposal, but the original PSBT? Is the naming wrong? |
There was a problem hiding this comment.
UncheckedProposal is the OriginalPSBT + the sender's paramaters. It is a PayJoin proposal
There was a problem hiding this comment.
I mean the UncheckedProposal struct is badly named, because the proposed PSBT is what is returned to the receiver.
| let proposal = UncheckedProposal::from_request(reader, query, headers).unwrap(); | ||
| // extract original PSBT from HTTP request | ||
| // TODO: `UncheckedProposal` is not really the proposal, but the original PSBT? Is the naming wrong? | ||
| // TODO: Unneeded and/or undocumented traits in bip-78 library? (`Proposal`, `Headers`) |
There was a problem hiding this comment.
Proposal is probably unecessary, but I imagine as a way to extract psbt() -> Psbt between check states if necessary. That might only be necessary at the fully checked / unlockedProposal stage. We can play with the interface to arrive at v1.0
There was a problem hiding this comment.
This is also an issue for the bip78.
| // extract original PSBT from HTTP request | ||
| // TODO: `UncheckedProposal` is not really the proposal, but the original PSBT? Is the naming wrong? | ||
| // TODO: Unneeded and/or undocumented traits in bip-78 library? (`Proposal`, `Headers`) | ||
| // TODO: We should really save the original PSBT just in case the sender does not sign |
There was a problem hiding this comment.
Pretty much. This is most important in the case when the receiver is a payment processor that generates bip21 URIs on request, called a non-interactive receiver. If you're scanning someone next to you's phone it's not so important to broadcast the UncheckedProposal.extract_tx() in the failure case or even make sure it can be broadcast because humans will be able to interact to solve the fail case by trying again.
There was a problem hiding this comment.
As previously mentioned, if UncheckedProposal is renamed to OriginalPsbtRequest or something alike, it would be less confusing.
| let fees = calculate_fees( | ||
| scheduled_payjoin.channels.len() as u64, | ||
| scheduled_payjoin.fee_rate, | ||
| scheduled_payjoin.wallet_amount != bitcoin::Amount::ZERO, | ||
| ); | ||
|
|
||
| // TODO: not great |
There was a problem hiding this comment.
make TODOs actionable. Explain qualitative disagreements.
There was a problem hiding this comment.
I mean we shouldn't have anything that panics when handling an http request
| psbt.unsigned_tx | ||
| .input | ||
| .iter_mut() | ||
| .for_each(|txin| txin.script_sig = bitcoin::Script::new()); |
There was a problem hiding this comment.
Yes and this should be automated by the payjoin crate instead of written here. Best downstream code doesn't need to grok this piece
| // TODO: make sure it is impossible to create a BIP21pj linked to `ScheduledPayJoin` | ||
| // with a "total channel amount" larger than requested + fee | ||
| let total_channel_amount = scheduled_payjoin |
There was a problem hiding this comment.
What's the problem with total_channel_amount > Bip78Uri.amount()`? The edge case I imagine is BTCPayServer requests an invoice for a strict amount and the excess is either 1. added to on chain single sig or 2. increase a channel amount. It does not seem like requesting more is dire.
| // TODO: Instead of interfacing with `tonic_lnd` directly, consider having a trait | ||
| // so this can be extended in the future |
There was a problem hiding this comment.
Agree. As soon as we duplicate code, abstract into a trait to support LDK & CLN.
| // TODO: Creating `OpenChannelRequest` should be one thing, and async calls another | ||
| // TODO: Instead of interfacing with `tonic_lnd` directly, consider having a trait | ||
| // so this can be extended in the future | ||
| // TODO: What if channel open fails or takes too long? |
There was a problem hiding this comment.
We get a corrupt channel state 😬 This is the worst failure case in this code. Most important error to address.
| use std::collections::HashMap; | ||
| use std::sync::{Arc, Mutex}; | ||
|
|
||
| use bitcoin::{Address, Script, TxOut}; | ||
|
|
||
| use crate::ScheduledPayJoin; | ||
|
|
||
| #[derive(Default)] | ||
| pub(crate) struct PayJoinScheduler { | ||
| // maps scheduled pjs via spk | ||
| by_spk: HashMap<Script, ScheduledPayJoin>, | ||
| } | ||
|
|
||
| impl PayJoinScheduler { | ||
| fn insert(&mut self, addr: &Address, pj: &ScheduledPayJoin) -> Result<(), ()> { | ||
| let current_pj = &*self.by_spk.entry(addr.script_pubkey()).or_insert(pj.clone()); | ||
| if current_pj == pj { | ||
| Err(()) // not inserted | ||
| } else { | ||
| Ok(()) // inserted | ||
| } | ||
| } | ||
|
|
||
| /// pop scheduled payjoin alongside txout | ||
| fn find_mut<'a, I>(&mut self, mut txouts: I) -> Option<(&'a mut TxOut, ScheduledPayJoin)> | ||
| where | ||
| I: Iterator<Item = &'a mut TxOut>, | ||
| { | ||
| txouts.find_map(|txo| self.by_spk.remove(&txo.script_pubkey).map(|pj| (txo, pj))) | ||
| } |
There was a problem hiding this comment.
This file is superfluous and should be outside of our history
Replaces and closes #1
This is a continuation of @DanGould's work.
payjoincrate for logic and various checks.TODOcomments everywhere.