add ProvisionalProposal struct #90

jbesraa · 2023-07-25T12:57:16Z

resolves #73

jbesraa · 2023-07-25T13:13:16Z

@DanGould
so I did just the first step which is creating a ProvisionalProposal without doing any changes to functionality but just signature changes.
I am planning to combine apply_fees and prepare_psbt as the next step, but when I looked in the CLI implementation I see that after apply_fee is executed, the psbt is finalized with the bitcoind.wallet but we dont have access to the wallet in payjoin receive module. should I add that as an input to "finalize_proposal" ?

actually maybe it would be good to think about the signature of finalize_proposal.
what do u think about this?

pub fn finalize_proposal(&mut self, w: impl wallet trait, min_feerate_sat_per_vb: Option<u64>)

DanGould · 2023-07-25T13:17:26Z

🏋️‍♂️🤙

It might make more sense to pass a closure as a parameter than a trait we'd have to define and implement downstream. This is how check methods are done earlier in the module. Take a look at those and let me know which parts you need more clarity on

payjoin/Cargo.toml

payjoin/src/receive/mod.rs

DanGould

This is starting to look beautiful. The attention to detail and effort is paying off

payjoin/src/receive/mod.rs

payjoin-cli/src/app.rs

DanGould · 2023-08-02T02:16:58Z

payjoin-cli/src/app.rs

-                Ok(false)
-            }
-        })?;
+        let mut provisional_payjoin: ProvisionalProposal =


is this explicit type required? Seems redundant since you're using such care with naming.

a nit would be to make check4's result proposal or provisional_payjoin instead of becoming a payjoin and turning into a provisional_payjoin

not really, removed.

created a new issue for check4 change here #96, will handle that in a new pr as i dont wanna add more changes to this pr

I'm a bit confused why you chose to make this a new issue, it seems this PR introduces the issue, no? The second part of the comment was just about naming.

jbesraa · 2023-08-06T10:07:14Z

@DanGould thanks so much for the input. I made the required changes, feel free to review

DanGould

Overall this is looking like a much less leaky way to handle typestate. I'm impressed.

I left a few more questions about the interface design

DanGould · 2023-08-06T22:59:22Z

payjoin/src/receive/mod.rs

+        wallet_process_psbt: impl Fn(&Psbt) -> Result<std::string::String, Error>,
+        min_feerate_sat_per_vb: Option<u64>,


Might we want to return Result<Psbt, Error> to validate the result and min_feerate: Option<FeeRate> for parameters?

we currently validate the result with Psbt::from_str, if we change the signature we basically leave the validation(or the conversion part from str to psbt) to the user.
when u make a call to finalize_psbt it will look something similar to this:

.finalize_proposal( |psbt: &Psbt| { Ok(receiver .wallet_process_psbt( &bitcoin::base64::encode(psbt.serialize()), None, None, Some(false), ) .map(|res: WalletProcessPsbtResult| { let psbt = Psbt::from_str(&res.psbt).unwrap(); //convert from str to psbt return psbt; }) .unwrap()) }, Some(bitcoin::FeeRate::MIN), )

and finalize_proposal would look like this

pub fn finalize_proposal( mut self, wallet_process_psbt: impl Fn(&Psbt) -> Result<Psbt, Error>, min_feerate_sat_per_vb: Option<FeeRate>, ) -> Result<PayjoinProposal, RequestError> { let psbt = self.apply_fee(min_feerate_sat_per_vb)?; let psbt = wallet_process_psbt(psbt).map_err(|_| { log::error!("wallet_process_psbt error"); InternalRequestError::PsbtFinalizingError })?; let payjoin_proposal = self.prepare_psbt(psbt).map_err(RequestError::from)?; Ok(payjoin_proposal) }

I like it. its nicer and makes a bit more sense because the function implementation is the user responsibility so it makes sense that they send the EXACT expected type. we can also do some checks after we get the psbt and do a set of validations from pj perspective (maybe differentiating between psbt version(s) or so..)

DanGould · 2023-08-06T23:01:35Z

payjoin/src/receive/error.rs

    Base64(bitcoin::base64::DecodeError),
+    PsbtFinalizingError(&'static str),


This always returns the same kind of string, so it doesn't need to take a value at all

InternalRequestError::PsbtFinalize would be fine

DanGould · 2023-08-06T23:04:44Z

payjoin/src/receive/mod.rs

+        let payjoin_proposal_psbt = match wallet_process_psbt(payjoin_proposal_psbt) {
+            Ok(p) => p,
+            Err(_) => {
+                return Err(RequestError::from(InternalRequestError::PsbtFinalizingError(
+                    "wallet_process_psbt failed",
+                )));
+            }
+        };


Suggested change

let payjoin_proposal_psbt = match wallet_process_psbt(payjoin_proposal_psbt) {

Ok(p) => p,

Err(_) => {

return Err(RequestError::from(InternalRequestError::PsbtFinalizingError(

"wallet_process_psbt failed",

)));

}

};

let psbt = wallet_process_psbt(payjoin_proposal_psbt)

.map_err(|_| InternalRequestError::PsbtFinalizingError.into())?;

I think it's fine to call it plain psbt in this context, we know it's a payjoin_proposal method function.

DanGould · 2023-08-06T23:06:00Z

payjoin/tests/integration.rs

+                        .map(|res| res.psbt)
+                        .map_err(|e| Error::Server(e.into()))
+                },
+                Some(1),


magic numbers usually mean something has gone wrong somewhere. I think we should accept a FeeRate here instead of u64

DanGould · 2023-08-06T23:06:33Z

payjoin/tests/integration.rs

+                Some(1),
+            )
+            .unwrap();
+        let psbt = payjoin_proposal.get_payjoin_psbt();


Is there a good reason to return a PayjoinProposal to hang onto instead of just the psbt straight away? there might be.

yea we might need to return other stuff in the future so In that case it wouldnt be a breaking change

That makes sense. It might make more sense to name this psbt() since we know it's in the PayjoinProposal and get_ is associated with returning from an index in rust by convention https://rust-lang.github.io/api-guidelines/naming.html#getter-names-follow-rust-convention-c-getter

DanGould · 2023-08-06T23:07:04Z

payjoin/src/receive/mod.rs

@@ -550,8 +551,8 @@ impl OutputsUnknown {
 }

 /// A mutable checked proposal that the receiver may contribute inputs to to make a payjoin.
+


DanGould · 2023-08-06T23:07:11Z

payjoin/Cargo.toml

@@ -28,4 +28,4 @@ env_logger = "0.9.0"
 bitcoind = { version = "0.31.1", features = ["0_21_2"] }

 [package.metadata.docs.rs]
-features = ["send", "receive"]


stray change here

so weird, cant find this change locally

jbesraa · 2023-08-09T15:23:10Z

hey @DanGould thanks for the review.
I made some small changes and wanna allocate a bigger time window for this to get to a better shape. I am trying to run the integration tests locally without success, any tips about that?
I tried with cargo test --verbose --all-features --lib and cargo test tests/integration.rs --verbose --all-features --lib without success.

DanGould · 2023-08-11T03:54:35Z

RUST_LOG=debug cargo test --package payjoin --test integration --features send --features receive -- integration::integration_test --exact --nocapture works for me, try that

jbesraa · 2023-08-13T14:15:27Z

payjoin-cli/src/app.rs

+                        None,
+                        Some(false),
+                    )
+                    .map(|res| Psbt::from_str(&res.psbt).unwrap())


@DanGould been trying to tackle this unwrap without success. how would you approach it?

This article gives a few different options for returning errors from iterators: https://doc.rust-lang.org/rust-by-example/error/iter_result.html

I think the main issue is that it it doesn't make sense for finalize_proposal to return a RequestError, because the request has already been checked by this point. The error is more of a receiver::Error::Server and could be returned inside that Box.

ah changing that to Error made things so easier! thanks

jbesraa · 2023-08-13T14:23:22Z

hey @DanGould I made few changes and resolved #96
we need to add a bit of docs to reflect this change, any other place than the code that I need to take into account?

DanGould · 2023-08-13T16:24:41Z

payjoin/src/receive/mod.rs

+        min_feerate_sat_per_vb: Option<FeeRate>,
+    ) -> Result<PayjoinProposal, RequestError> {
+        let psbt = self.apply_fee(min_feerate_sat_per_vb)?;
+        let psbt = wallet_process_psbt(psbt).map_err(|_| {


better to handle the specific error instead of discarding it by returning Error from finalize_proposal instead of RequestError. Then we don't have to convert after prepare_psbt either

DanGould · 2023-08-13T16:30:07Z

payjoin-cli/src/app.rs

-        let payjoin = proposal.check_no_inputs_seen_before(|input| {
-            Ok(!self.insert_input_seen_before(*input).map_err(|e| Error::Server(e.into()))?)
-        })?;
+        let mut provisional_payjoin = proposal.check_no_inputs_seen_before(
+            |input| {
+                Ok(!self.insert_input_seen_before(*input).map_err(|e| Error::Server(e.into()))?)
+            },
+            |output_script| {
+                if let Ok(address) = bitcoin::Address::from_script(output_script, network) {
+                    self.bitcoind
+                        .get_address_info(&address)
+                        .map(|info| info.is_mine.unwrap_or(false))
+                        .map_err(|e| Error::Server(e.into()))
+                } else {
+                    Ok(false)
+                }
+            },
+        )?;


identify_receiver_outputs does not seem directly related to the check, so I think we lose some semantics by merging them into one step. Either way it's unrelated to the ProvisionalProposal struct and would be better suited to its own PR

yea I see what u mean
maybe we can change the parent function name

I don't think it should be renamed rather not combined. It's unrelated to this PR.

DanGould · 2023-08-13T16:32:44Z

payjoin/src/receive/mod.rs

@@ -491,7 +491,8 @@ impl MaybeInputsSeen {
    pub fn check_no_inputs_seen_before(
        self,
        is_known: impl Fn(&OutPoint) -> Result<bool, Error>,
-    ) -> Result<OutputsUnknown, Error> {
+        is_receiver_output: impl Fn(&Script) -> Result<bool, Error>,
+    ) -> Result<ProvisionalProposal, Error> {


I can see why these might be merged, but perhaps the checks should return RequestError (since if they fail, the request is bad) while the closure returns Error, since that's a problem with the server implementation. Either way this is separate from the ProvisionalProposal change

DanGould · 2023-08-18T15:12:56Z

I'm not sure my comments re: return type errors and the merging of 2 functions into one have been addressed. Addressing them both will help this API deliver on its purpose.

I think #[derive(Default)] on an enum breaks our MSRV requirement in CI, too.

And for the most detail oriented changes, the commit messages would benefit from these semantics so we have a neat history to read later down the line https://cbea.ms/git-commit/

CI is failing because flate2 released new version 1.0.27 that is not passing rustc 1.57. Reference: rust-lang/flate2-rs#370

Motivaiton is to create a struct that is used until we finalize the proposal and return PayjoinProposal so we dont leak data out of PayjoinProposal. Changed apply_fee and prepare_psbt to private functions and they are now invoked from finalize_proposal

DanGould · 2023-08-23T17:09:42Z

I've re-ordered the commits so that each one passes tests and changed the apply_fee param to min_fee_rate since it's using the FeeRate type and doesn't need to specify sats_per_byte.

@jbesraa you've made this receiver interface 10x cleaner at least ⭐️

jbesraa changed the title ~~add ProvisionalProposal struct~~ [Draft] add ProvisionalProposal struct Jul 25, 2023

jbesraa marked this pull request as draft July 25, 2023 12:58

DanGould reviewed Jul 26, 2023

View reviewed changes

payjoin/Cargo.toml Outdated Show resolved Hide resolved

DanGould reviewed Jul 26, 2023

View reviewed changes

payjoin/src/receive/mod.rs Outdated Show resolved Hide resolved

jbesraa force-pushed the Feature/Add-ProvisionalPayjoinProposal-Struct branch 2 times, most recently from fb02857 to 579bbda Compare July 31, 2023 19:57

jbesraa changed the title ~~[Draft] add ProvisionalProposal struct~~ add ProvisionalProposal struct Jul 31, 2023

jbesraa marked this pull request as ready for review July 31, 2023 20:22

DanGould reviewed Aug 2, 2023

View reviewed changes

jbesraa force-pushed the Feature/Add-ProvisionalPayjoinProposal-Struct branch 2 times, most recently from b00f73d to 956025e Compare August 6, 2023 07:31

jbesraa mentioned this pull request Aug 6, 2023

change check_no_inputs_seen_before signature to return proposal or provisional_payjoin #96

Closed

DanGould reviewed Aug 6, 2023

View reviewed changes

jbesraa force-pushed the Feature/Add-ProvisionalPayjoinProposal-Struct branch 2 times, most recently from 25eac79 to e26ad17 Compare August 9, 2023 15:13

jbesraa commented Aug 13, 2023

View reviewed changes

jbesraa force-pushed the Feature/Add-ProvisionalPayjoinProposal-Struct branch from d025713 to ac4f10a Compare August 13, 2023 14:19

DanGould reviewed Aug 13, 2023

View reviewed changes

jbesraa force-pushed the Feature/Add-ProvisionalPayjoinProposal-Struct branch 2 times, most recently from aaf5b99 to 84e8ac9 Compare August 18, 2023 05:07

jbesraa force-pushed the Feature/Add-ProvisionalPayjoinProposal-Struct branch from 84e8ac9 to 88d37e7 Compare August 20, 2023 15:11

jbesraa force-pushed the Feature/Add-ProvisionalPayjoinProposal-Struct branch 2 times, most recently from 64602c3 to 209c153 Compare August 20, 2023 15:29

jbesraa added 2 commits August 23, 2023 12:58

Pin flate2-rs to 1.0.26

991695f

CI is failing because flate2 released new version 1.0.27 that is not passing rustc 1.57. Reference: rust-lang/flate2-rs#370

Create ProvisionalPayjoin Struct

be353ab

Motivaiton is to create a struct that is used until we finalize the proposal and return PayjoinProposal so we dont leak data out of PayjoinProposal. Changed apply_fee and prepare_psbt to private functions and they are now invoked from finalize_proposal

DanGould force-pushed the Feature/Add-ProvisionalPayjoinProposal-Struct branch from 17da43c to be353ab Compare August 23, 2023 17:00

DanGould merged commit 0c6a2e5 into payjoin:master Aug 23, 2023
7 checks passed

DanGould mentioned this pull request Sep 30, 2023

Release payjoin-0.10.0 #107

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ProvisionalProposal struct #90

add ProvisionalProposal struct #90

jbesraa commented Jul 25, 2023 •

edited

Loading

jbesraa commented Jul 25, 2023

DanGould commented Jul 25, 2023

DanGould left a comment

DanGould Aug 2, 2023

jbesraa Aug 6, 2023

DanGould Aug 6, 2023 •

edited

Loading

jbesraa commented Aug 6, 2023 •

edited

Loading

DanGould left a comment

DanGould Aug 6, 2023

jbesraa Aug 11, 2023

DanGould Aug 6, 2023

jbesraa Aug 11, 2023

DanGould Aug 6, 2023

DanGould Aug 6, 2023

DanGould Aug 6, 2023

jbesraa Aug 13, 2023

DanGould Aug 13, 2023

DanGould Aug 6, 2023

DanGould Aug 6, 2023

jbesraa Aug 13, 2023

jbesraa commented Aug 9, 2023

DanGould commented Aug 11, 2023 •

edited

Loading

jbesraa Aug 13, 2023

DanGould Aug 13, 2023

jbesraa Aug 18, 2023

jbesraa commented Aug 13, 2023

DanGould Aug 13, 2023

DanGould Aug 13, 2023

jbesraa Aug 18, 2023

DanGould Aug 18, 2023

DanGould Aug 13, 2023

DanGould commented Aug 18, 2023

DanGould commented Aug 23, 2023

		wallet_process_psbt: impl Fn(&Psbt) -> Result<std::string::String, Error>,
		min_feerate_sat_per_vb: Option<u64>,

		Base64(bitcoin::base64::DecodeError),
		PsbtFinalizingError(&'static str),

		@@ -550,8 +551,8 @@ impl OutputsUnknown {
		}

		/// A mutable checked proposal that the receiver may contribute inputs to to make a payjoin.

add ProvisionalProposal struct #90

add ProvisionalProposal struct #90

Conversation

jbesraa commented Jul 25, 2023 • edited Loading

jbesraa commented Jul 25, 2023

DanGould commented Jul 25, 2023

DanGould left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanGould Aug 6, 2023 • edited Loading

Choose a reason for hiding this comment

jbesraa commented Aug 6, 2023 • edited Loading

DanGould left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa commented Aug 9, 2023

DanGould commented Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbesraa commented Aug 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanGould commented Aug 18, 2023

DanGould commented Aug 23, 2023

jbesraa commented Jul 25, 2023 •

edited

Loading

DanGould Aug 6, 2023 •

edited

Loading

jbesraa commented Aug 6, 2023 •

edited

Loading

DanGould commented Aug 11, 2023 •

edited

Loading