Skip to content

Remove redundant fields from WantsInputs event #1106

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.

Sign up for GitHub

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

Merged
arminsabouri merged 1 commit into from
Sep 25, 2025
Merged

Remove redundant fields from WantsInputs event #1106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion payjoin/src/core/receive/common/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use crate::receive::{InternalPayloadError, OriginalPayload, PsbtContext};
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
pub struct WantsOutputs {
original_psbt: Psbt,
payjoin_psbt: Psbt,
pub(super) payjoin_psbt: Psbt,
params: Params,
change_vout: usize,
pub(crate) owned_vouts: Vec<usize>,
Expand Down
13 changes: 11 additions & 2 deletions payjoin/src/core/receive/v2/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -821,12 +821,21 @@ impl Receiver<WantsOutputs> {
pub fn commit_outputs(self) -> NextStateTransition<SessionEvent, Receiver<WantsInputs>> {
let inner = self.state.inner.clone().commit_outputs();
NextStateTransition::success(
SessionEvent::WantsInputs(inner.clone()),
SessionEvent::WantsInputs(self.state.inner.payjoin_psbt.unsigned_tx.output),
Receiver { state: WantsInputs { inner }, session_context: self.session_context },
)
}

pub(crate) fn apply_wants_inputs(self, inner: common::WantsInputs) -> ReceiveSession {
pub(crate) fn apply_wants_inputs(self, outputs: Vec<TxOut>) -> ReceiveSession {
let mut payjoin_proposal = self.inner.payjoin_psbt.clone();
let outputs_len = outputs.len();
// Add the outputs that may have been replaced
payjoin_proposal.unsigned_tx.output = outputs;
payjoin_proposal.outputs = vec![Default::default(); outputs_len];

let mut inner = self.state.inner.commit_outputs();
inner.payjoin_psbt = payjoin_proposal;
Comment on lines +830 to +837
Copy link
Contributor

@DanGould DanGould Sep 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

What happens with legacy pre-segwit outputs? I suppose there is never a case where you need bitcoin::psbt::Output because, well, they're outputs being spent to and all you need is the TxOut.script_pubkey to spend to them? perhaps a comment explaining why the Default payjoin_proposal.outputs are appropriate would clarify.

This is making me think the SessionEvent variants might need renames. This WantsInputs event does produce a WantsInputs type but it's really an event which does ~CommitOutputs, since it encapsulates those outputs. I think that tripped me up on first read.

Copy link
Collaborator

@spacebear21 spacebear21 Sep 25, 2025

Choose a reason for hiding this comment

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

This is making me think the SessionEvent variants might need renames.

I agree, event variants should describe "what event happened" instead of "what this event produces". We already do this for the Created and Closed variants. Could be something like:

{
//...
    IdentifiedReceiverOutputs(Vec<usize>),
    CommittedOutputs(Vec<bitcoin::TxOut>),
    CommittedInputs(Vec<InputPair>),
//...
}

and so on for the other variants.

Copy link
Contributor

@DanGould DanGould Sep 25, 2025

Choose a reason for hiding this comment

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

I prefer these get named in imperative mood over past tense but otherwise agree

Copy link
Collaborator Author

@arminsabouri arminsabouri Sep 25, 2025

Choose a reason for hiding this comment

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

I suppose there is never a case where you need bitcoin::psbt::Output because, well, they're outputs being spent to and all you need is the TxOut.script_pubkey to spend to them?

Correct. I actually copied this code from the common abstraction

        // Update the payjoin PSBT outputs
        payjoin_psbt.outputs = vec![Default::default(); outputs.len()];
        payjoin_psbt.unsigned_tx.output = outputs;

rust-payjoin/payjoin/src/core/receive/common/mod.rs

Line 135 in f7589fb

// Update the payjoin PSBT outputs

The PSBT outputs are being set to default regardless if the receiver replaces their outputs.
That may be an issue for some implementations that expect BIP32 paths to be present in order to validate change address. But in those cases the implementation could mutate the PSBT pre-signing.


let new_state =
Receiver { state: WantsInputs { inner }, session_context: self.session_context };
ReceiveSession::WantsInputs(new_state)
Expand Down
14 changes: 9 additions & 5 deletions payjoin/src/core/receive/v2/session.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use crate::error::{InternalReplayError, ReplayError};
use crate::output_substitution::OutputSubstitution;
use crate::persist::SessionPersister;
use crate::receive::v2::{extract_err_req, SessionError};
use crate::receive::{common, InputPair, JsonReply, OriginalPayload, PsbtContext};
use crate::receive::{InputPair, JsonReply, OriginalPayload, PsbtContext};
use crate::{ImplementationError, IntoUrl, PjUri, Request};

/// Replay a receiver event log to get the receiver in its current state [ReceiveSession]
Expand Down Expand Up @@ -197,7 +197,7 @@ pub enum SessionEvent {
MaybeInputsSeen(),
OutputsUnknown(),
WantsOutputs(Vec<usize>),
WantsInputs(common::WantsInputs),
WantsInputs(Vec<bitcoin::TxOut>),
WantsFeeRange(Vec<InputPair>),
ProvisionalProposal(PsbtContext),
PayjoinProposal(bitcoin::Psbt),
Expand Down Expand Up @@ -297,7 +297,7 @@ mod tests {
SessionEvent::MaybeInputsSeen(),
SessionEvent::OutputsUnknown(),
SessionEvent::WantsOutputs(wants_outputs.state.inner.owned_vouts.clone()),
SessionEvent::WantsInputs(wants_inputs.state.inner.clone()),
SessionEvent::WantsInputs(wants_outputs.state.inner.payjoin_psbt.unsigned_tx.output),
SessionEvent::WantsFeeRange(wants_fee_range.state.inner.receiver_inputs.clone()),
SessionEvent::ProvisionalProposal(provisional_proposal.state.psbt_context.clone()),
SessionEvent::PayjoinProposal(payjoin_proposal.psbt().clone()),
Expand Down Expand Up @@ -508,7 +508,9 @@ mod tests {
events.push(SessionEvent::MaybeInputsSeen());
events.push(SessionEvent::OutputsUnknown());
events.push(SessionEvent::WantsOutputs(wants_outputs.state.inner.owned_vouts.clone()));
events.push(SessionEvent::WantsInputs(wants_inputs.state.inner.clone()));
events.push(SessionEvent::WantsInputs(
wants_outputs.state.inner.payjoin_psbt.unsigned_tx.output,
));
events
.push(SessionEvent::WantsFeeRange(wants_fee_range.state.inner.receiver_inputs.clone()));
events.push(SessionEvent::ProvisionalProposal(
Expand Down Expand Up @@ -586,7 +588,9 @@ mod tests {
events.push(SessionEvent::MaybeInputsSeen());
events.push(SessionEvent::OutputsUnknown());
events.push(SessionEvent::WantsOutputs(wants_outputs.state.inner.owned_vouts.clone()));
events.push(SessionEvent::WantsInputs(wants_inputs.state.inner.clone()));
events.push(SessionEvent::WantsInputs(
wants_outputs.state.inner.payjoin_psbt.unsigned_tx.output,
));
events
.push(SessionEvent::WantsFeeRange(wants_fee_range.state.inner.receiver_inputs.clone()));
events.push(SessionEvent::ProvisionalProposal(
Expand Down
Loading