WIP - Payjoin POC - DO NOT MERGE #1806
Conversation
benalleng
force-pushed
the
payjoin-v4
branch
2 times, most recently
from
e96d087 to
92236a2
Compare
This comment was marked as resolved.
This comment was marked as resolved.
benalleng
force-pushed
the
payjoin-v4
branch
2 times, most recently
from
34857cd to
6c00f6c
Compare
|
note: not yet taken time to look at this, on my todo for this week |
There was a problem hiding this comment.
I take a first look at this PR, not yet dig the payjoin logic but here few preliminary comments.
I've also opened this issue for general talk about payjoin integration
liana-gui/src/app/message.rs
Outdated
| LoadWallet(Wallet), | ||
| Info(Result<GetInfoResult, Error>), | ||
| ReceiveAddress(Result<(Address, ChildNumber), Error>), | ||
| ReceiveAddress(Result<(Address, ChildNumber, Option<Url>), Error>), |
There was a problem hiding this comment.
I'd be in favor of a separate variant ReceivePayjoin here instead merging the 2 purposes
| &self | ||
| .addresses | ||
| .bip21s | ||
| .get(self.address(i).expect("Address should be in bip21")), |
There was a problem hiding this comment.
we should not panic here, just stop and log w/ tracing::error
| label: form::Value<String>, | ||
| address: form::Value<String>, | ||
| amount: form::Value<String>, | ||
| bip21: form::Value<String>, |
There was a problem hiding this comment.
I dont think we should have a separate field for BIP21, we should be able to parse both BIP21 & address in the address field
| SelectHardwareWallet(usize), | ||
| CreateRbf(CreateRbfMessage), | ||
| ShowQrCode(usize), | ||
| ShowBip21QrCode(usize), |
There was a problem hiding this comment.
I dont think we should have a separate message for this
| self.derivation_index(i), | ||
| ) { | ||
| if let Some(modal) = ShowBip21QrCodeModal::new(bip21, *index) { | ||
| self.modal = Modal::ShowBip21QrCode(modal); |
There was a problem hiding this comment.
Do we really need a different modal, can't we reuse the initial QR modal?
lianad/src/commands/mod.rs
Outdated
| None, | ||
| ) | ||
| .save(&persister) | ||
| .unwrap(); |
There was a problem hiding this comment.
we can error here instead
| } | ||
|
|
||
| /// Initiate a payjoin sender | ||
| // TODO bip21 should be a uri not a string |
There was a problem hiding this comment.
yes validation should be done early
| let uri = uri | ||
| .check_pj_supported() | ||
| .map_err(|_| "URI does not support Payjoin".to_string()) | ||
| .unwrap(); |
There was a problem hiding this comment.
we should error instead
| } | ||
|
|
||
| /// Get Payjoin URI (BIP21) and its sender/receiver status by txid | ||
| pub fn get_payjoin_info(&self, txid: &bitcoin::Txid) -> Result<PayjoinStatus, CommandError> { |
There was a problem hiding this comment.
In this fn info logs should be debug
| ) | ||
| })?; | ||
|
|
||
| for index in 0..spend_psbt.inputs.len() { |
There was a problem hiding this comment.
info => debug
warn => error
This commit acts as the initial approach set out by Artur to add payjoin tnio liana. Any changes from this PR will be split up into their individual issues / PRs for more exact review.
When the receive session for loop failed to proceed past a certain callback method it stopped the entire for loop but now it simply skips over those sessions allowing the other sessions to proceed giving those older sessions an opportunity to be fixed or deleted.
benalleng
force-pushed
the
payjoin-v4
branch
2 times, most recently
from
1984f04 to
7f6b618
Compare
update msrv to 1.85.0
3 participants
THIS PR IS NOT INTENDED TO BE MERGED
This commit acts as the initial approach set out by Artur, Armin, and myself to add payjoin support into liana.
Any changes from this PR will be split up into their individual issues / PRs for more precise review.
Some initial starting points may be to add bip21 support as an example which is a small lift and is ultimately not payjoin specific