Separate out fee application into own typestate by arminsabouri · Pull Request #881 · payjoin/rust-payjoin

arminsabouri · 2025-07-15T15:58:44Z

This commit refactors fee application on the receiver side by moving
it out of the ProvisionalProposal typestate into a dedicated
WantsFeeRange typestate, as outlined in #870

It also removes the receiver_inputs field from
ProvisionalProposal, since it was solely used during fee computation
and is no longer needed in this state.

coveralls · 2025-07-15T19:34:32Z

Pull Request Test Coverage Report for Build 16326869862

Details

104 of 110 (94.55%) changed or added relevant lines in 6 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.03%) to 85.8%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-cli/src/app/v2/mod.rs	10	12	83.33%
payjoin/src/core/receive/v2/mod.rs	23	27	85.19%

Totals
Change from base Build 16298010662:	0.03%
Covered Lines:	7698
Relevant Lines:	8972

💛 - Coveralls

arminsabouri · 2025-07-15T19:42:43Z

FWIW I wanted to include an example of how we are using this in the Liana integration: payjoin/liana#8

spacebear21

concept ACK, but the FFI parameter names should keep the fee rate unit, and the new typestate/method need docstrings (these should also be applied to the FFI layer).

I second Dan's suggestion in #870 to rename ProvisionalProposal to WantsSigner as well, but this can be done in a follow-up after we evaluate whether it should allow for multiple individual signature operations + a commitment method. I don't think that should block this PR since separating fee application solves a separate known problem and makes for a cleaner state machine regardless.

spacebear21 · 2025-07-15T20:20:26Z

payjoin/src/core/receive/v1/mod.rs

        output_contribution_weight
    }

+    pub fn commit_fee_range(


The "commit" naming pattern refers to something that was contributed/applied to the existing typestate (e.g. contributed inputs/outputs) and "committing" it moves to the next typestate. Maybe apply_fee_range, with an explanatory docstring that it only applies a fee if the receiver contributions exceed the sender's additional fee, to match min_fee_rate, and not exceeding max_effective_fee_rate.

TBH the docstring from the inner apply_fee should probably just live on this method, since it's public whereas apply_fee is private.

spacebear21 · 2025-07-15T20:25:11Z

payjoin/src/core/receive/v1/mod.rs

+        let mut finalized_psbt =
+            wallet_process_psbt(&self.payjoin_psbt).map_err(ReplyableError::Implementation)?;


Was there a reason to move this to happen before the sender signatures get cleared? IIRC it had to happen in this order before to fix a bug in bitcoin-cli, though this specific bug has been fixed since. See #241

Not particularly. If it was moved that is my mistake.

spacebear21 · 2025-07-15T20:31:10Z

payjoin-ffi/src/receive/uni.rs

+        min_fee_rate: Option<u64>,
+        max_effective_fee_rate: Option<u64>,


Since the strong FeeRate type is lost in uniffi translation, we should specify the unit in the parameter names (e.g. min_fee_rate_sat_per_vb).

yes I had the same thought.

Fixed in the first commit

spacebear21 · 2025-07-15T20:33:27Z

payjoin-ffi/src/receive/uni.rs

    }

+    /// Psbt With fee contributions applied
+    pub fn psbt_with_fee_contributions(&self) -> Option<Arc<crate::Psbt>> {


What value does it provide to make this specific PSBT state accessible downstream? And is psbt_with_contributed_inputs still useful at all?

And is psbt_with_contributed_inputs still useful at all?

No I would recommend we either deprecate it or just remove it.

What value does it provide to make this specific PSBT state accessible downstream?

This is useful when the singer is not the entity that is progressing the state machine. In these wallet architectures the psbt that is "ready to be signed" will be made accessible when its requested. For a more concrete example please reference payjoin/liana#8. In this example, Lianad (backend service) will save the psbt with fee's applies in their psbt table. When the signer navigates to the their psbt page the backend will pull from this table and the user can sign.

psbt_with_contributed_inputs is now removed

It might carry more meaning to name this psbt_ready_for_signing?

arminsabouri · 2025-07-15T22:15:38Z

I second Dan's suggestion in #870 to rename ProvisionalProposal to WantsSigner as well, but this can be done in a follow-up after we evaluate whether it should allow for multiple individual signature operations + a commitment method. I don't think that should block this PR since separating fee application solves a separate known problem and makes for a cleaner state machine regardless.

I don't understand the rationale for this naming change or the new methods. rust-payjoin should less involved in signing operations and by extension application business logic. All rust-payjoin cares about from ProvisionalProposal to PayjoinProposal is that all the inputs belonging to the reciever are finalized. How it finalizes the inputs should be a application concern. Perhaps the naming should be WantsFinalizedInputs or WantsFinalizedProposal.

spacebear21

utACK with a couple of naming comments that imo would be best addressed in this PR.

spacebear21 · 2025-07-16T15:54:42Z

payjoin-ffi/src/receive/uni.rs

    }

+    /// Psbt With fee contributions applied
+    pub fn psbt_with_fee_contributions(&self) -> Option<Arc<crate::Psbt>> {


It might carry more meaning to name this psbt_ready_for_signing?

payjoin-cli/src/app/v2/mod.rs

payjoin/src/core/receive/multiparty/mod.rs

spacebear21 · 2025-07-16T16:11:26Z

payjoin/src/core/receive/v1/mod.rs

@@ -819,21 +843,19 @@ impl ProvisionalProposal {
    ///
    /// Errors if the final effective fee rate exceeds `max_effective_fee_rate`.
    pub fn finalize_proposal(


I just noticed this docstring needs to be updated too since all the fee stuff is now handled in WantsFeeRange.

This commit refactors fee application on the receiver side by moving it out of the `ProvisionalProposal` typestate into a dedicated `WantsFeeRange` typestate, as outlined in payjoin#870 It also removes the `receiver_inputs` field from `ProvisionalProposal`, since it was solely used during fee computation and is no longer needed in this state.

This method was originally exposed to provide signers access to the PSBT they are expected to sign. However, that PSBT excludes the receiver's fee contributions. It has therefore been replaced by `psbt_with_fee_contributions`.

spacebear21

re-ACK 3dcea20

arminsabouri changed the title ~~WIP - seperate out fee application into own typestate~~ WIP - separate out fee application into own typestate Jul 15, 2025

arminsabouri force-pushed the seperate-fee-application-typestate branch 2 times, most recently from 58255be to 9861a58 Compare July 15, 2025 19:32

arminsabouri changed the title ~~WIP - separate out fee application into own typestate~~ Separate out fee application into own typestate Jul 15, 2025

arminsabouri marked this pull request as ready for review July 15, 2025 19:36

arminsabouri requested a review from spacebear21 July 15, 2025 19:42

spacebear21 requested changes Jul 15, 2025

View reviewed changes

arminsabouri force-pushed the seperate-fee-application-typestate branch from 9861a58 to 7776fa1 Compare July 15, 2025 22:07

spacebear21 approved these changes Jul 16, 2025

View reviewed changes

spacebear21 mentioned this pull request Jul 16, 2025

Seperating fee application from signing #870

Closed

spacebear21 reviewed Jul 16, 2025

View reviewed changes

arminsabouri force-pushed the seperate-fee-application-typestate branch from 7776fa1 to 5eef949 Compare July 16, 2025 17:59

arminsabouri added 3 commits July 16, 2025 14:02

Expose ready to be signed psbt from session history

c5164fb

Remove psbt_with_contributed_inputs

3dcea20

This method was originally exposed to provide signers access to the PSBT they are expected to sign. However, that PSBT excludes the receiver's fee contributions. It has therefore been replaced by `psbt_with_fee_contributions`.

arminsabouri force-pushed the seperate-fee-application-typestate branch from 5eef949 to 3dcea20 Compare July 16, 2025 18:03

spacebear21 approved these changes Jul 18, 2025

View reviewed changes

spacebear21 merged commit 16c13d6 into payjoin:master Jul 18, 2025
15 checks passed

		let mut finalized_psbt =
		wallet_process_psbt(&self.payjoin_psbt).map_err(ReplyableError::Implementation)?;

		min_fee_rate: Option<u64>,
		max_effective_fee_rate: Option<u64>,

Conversation

arminsabouri commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 16326869862

Details

💛 - Coveralls

Uh oh!

arminsabouri commented Jul 15, 2025

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacebear21 Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arminsabouri Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arminsabouri commented Jul 15, 2025

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

arminsabouri commented Jul 15, 2025 •

edited

Loading

coveralls commented Jul 15, 2025 •

edited

Loading

spacebear21 Jul 15, 2025 •

edited

Loading

arminsabouri Jul 15, 2025 •

edited

Loading