Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send payjoin #647

Merged
merged 1 commit into from
Nov 29, 2023
Merged

Send payjoin #647

merged 1 commit into from
Nov 29, 2023

Conversation

DanGould
Copy link
Contributor

@DanGould DanGould commented Jul 7, 2023

Here's an outline of what a payjoin implementation might look like. There are a few dangling pieces but the bulk of the happy path is here.

A mutinynet payjoin receiver is live and funded when you're ready to test this out by pulling a new payjoin bip21 from https://mutiny.payjoin.org:4433/bip21

Note, this version of payjoin depends on rust-bitcoin v0.30.0 to use the new version of bip21 that disables default features for mutiny. So it duplicates that dependency until Mutiny upgrades.

tag #194, but I don't think it closes it. Send is 1/2 of the equation.

@TonyGiorgio
Copy link
Contributor

Awesome! Really appreciate that.

I think from the frontend point of view, it just uses https://github.com/MutinyWallet/bitcoin-waila for parsing the bitcoin URI and giving the result of that mutiny-node.

So I think just exposing this like it is here

pub async fn send_to_address(
&self,
send_to: Address,
amount: u64,
labels: Vec<String>,
fee_rate: Option<f32>,
) -> Result<Txid, MutinyError> {
if !send_to.is_valid_for_network(self.network) {
return Err(MutinyError::IncorrectNetwork(send_to.network));
}
self.wallet.send(send_to, amount, labels, fee_rate).await
}
&
pub async fn send_to_address(
&self,
destination_address: String,
amount: u64,
labels: JsValue, /* Vec<String> */
fee_rate: Option<f32>,
) -> Result<String, MutinyJsError> {
let send_to = Address::from_str(&destination_address)?;
let labels: Vec<String> = labels
.into_serde()
.map_err(|_| MutinyJsError::InvalidArgumentsError)?;
Ok(self
.inner
.node_manager
.send_to_address(send_to, amount, labels, fee_rate)
.await?
.to_string())
}

@DanGould
Copy link
Contributor Author

DanGould commented Jul 7, 2023

Would the payjoin Uri check be a better fit in mutiny-wasm, then?

        let uri = payjoin::Uri::try_from(uri)
            .expect("Invalid payjoin URI")
            .require_network(self.network)
            .expect("Payjoin network mismatch")
            .check_pj_supported()
            .expect("Payjoin not supported");

@TonyGiorgio
Copy link
Contributor

Would the payjoin Uri check be a better fit in mutiny-wasm, then?

        let uri = payjoin::Uri::try_from(uri)
            .expect("Invalid payjoin URI")
            .require_network(self.network)
            .expect("Payjoin network mismatch")
            .check_pj_supported()
            .expect("Payjoin not supported");

Oh yeah good point, I think that would be good. That way it can just be passed over to the mutiny-core 's pay_payjoin method.

@benthecarman
Copy link
Collaborator

Would the payjoin Uri check be a better fit in mutiny-wasm, then?

        let uri = payjoin::Uri::try_from(uri)
            .expect("Invalid payjoin URI")
            .require_network(self.network)
            .expect("Payjoin network mismatch")
            .check_pj_supported()
            .expect("Payjoin not supported");

maybe this should be added to waila

@DanGould
Copy link
Contributor Author

DanGould commented Jul 7, 2023

it would not be too hard to export the payjoin uri bits as a standalone feature if you wanted to depend on it in waila instead of duplicating

@DanGould
Copy link
Contributor Author

This has been refactored to handle errors, sign in a BDK environment, produce spec-recommended fee contribution, and separate onchain behavior from nodemanager behavior. I'll have to figure out how to hook it up to mutiny-web to proceed with testing.

@DanGould
Copy link
Contributor Author

DanGould commented Oct 4, 2023

I did have to alter payjoin-cli to filter mixed input contribution with the following

let candidate_inputs: HashMap<Amount, OutPoint> = available_inputs
    .iter()
    .filter(|i| i.address.clone().is_some_and(|a| a.assume_checked().address_type() == Some(bitcoin::AddressType::P2tr)))
    .map(|i| (i.amount, OutPoint { txid: i.txid, vout: i.vout }))
    .collect();

But this was an issue with payjoin-cli (payjoin/rust-payjoin#70), not this draft.

@DanGould DanGould marked this pull request as ready for review October 19, 2023 15:00
@DanGould
Copy link
Contributor Author

Other than correctness, the design area I'm most interested in review is the surface between web and node. I wonder if web should completely parse the bip21 and pass validated components, or if node should validate all of the inputs, or both. Right now web just passes a raw destination which node parses.

@benalleng benalleng force-pushed the send-payjoin branch 4 times, most recently from 1a30143 to 4908b6a Compare November 22, 2023 19:15
@DanGould
Copy link
Contributor Author

@benalleng and I fixed the Failed to send issue. We're having expected solid reliability now 😎 This should be ready for a review.

Copy link
Collaborator

@benthecarman benthecarman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good to me, might want to hold off on merging for now. @TonyGiorgio is doing a pretty large refactor in #867 that will effect this.

&self,
payjoin_uri: String,
amount: u64, /* override the uri amount if desired */
labels: &JsValue, /* Vec<String> */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be labels: Vec<String> now

@TonyGiorgio
Copy link
Contributor

Overall this looks good to me, might want to hold off on merging for now. @TonyGiorgio is doing a pretty large refactor in #867 that will effect this.

I can work around it, there's not much I think will conflict.

Copy link
Contributor

@TonyGiorgio TonyGiorgio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks solid! Thank you both for testing this thoroughly and getting it in here. Exciting!

@TonyGiorgio TonyGiorgio merged commit 74a090d into MutinyWallet:master Nov 29, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants