Skip to content

Introduce constructors for legacy input pairs#753

Merged
spacebear21 merged 1 commit intopayjoin:masterfrom
shinghim:583-input-pair
Jun 9, 2025
Merged

Introduce constructors for legacy input pairs#753
spacebear21 merged 1 commit intopayjoin:masterfrom
shinghim:583-input-pair

Conversation

@shinghim
Copy link
Contributor

@shinghim shinghim commented Jun 8, 2025

Checking off some remaining tasks in #583 - this PR introduces a constructor for each legacy type

@coveralls
Copy link
Collaborator

coveralls commented Jun 8, 2025

Pull Request Test Coverage Report for Build 15526167253

Details

  • 180 of 186 (96.77%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 85.463%

Changes Missing Coverage Covered Lines Changed/Added Lines %
payjoin/src/psbt/mod.rs 0 2 0.0%
payjoin/src/receive/mod.rs 180 184 97.83%
Totals Coverage Status
Change from base Build 15500779033: 0.3%
Covered Lines: 6990
Relevant Lines: 8179

💛 - Coveralls

@shinghim
Copy link
Contributor Author

shinghim commented Jun 8, 2025

From this comment from the payjoin-cli wallet code:

// NOTE: non_witness_utxo is not necessary because bitcoin-cli always supplies
// witness_utxo, even for non-witness inputs

The existing code is populating the witness_utxo field for a legacy transaction, which deviates from the BIP-174 spec of non-witness utxo:

The transaction in network serialization format the current input spends from. This should be present for inputs that spend non-segwit outputs and can be present for inputs that spend segwit outputs.

The constructors I created here had the spec in mind and populate the non_witness_utxo of a PSBT, but should these instead be populating the witness_utxo field?

@shinghim shinghim marked this pull request as ready for review June 8, 2025 21:25
Copy link
Collaborator

@spacebear21 spacebear21 left a comment

Choose a reason for hiding this comment

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

The constructors I created here had the spec in mind and populate the non_witness_utxo of a PSBT, but should these instead be populating the witness_utxo field?

This sounds like the correct approach. IIRC the reason we just provided witness_utxo for non-witness inputs is just that it was there for us to use in core RPC responses, which removed the need to make an extra RPC call to get the previous Transaction, but that was a shortcut.

Copy link
Collaborator

@spacebear21 spacebear21 left a comment

Choose a reason for hiding this comment

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

ACK aa4e9d3

I left two minor comments that can be addressed in follow-ups.

let redeem_script = if let Some(ref script_sig) = self.psbtin.final_script_sig {
script_sig.redeem_script()
// try the PSBT redeem_script field for unsigned inputs.
// try the PSBT redeem_script field for unsigned inputs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like this comment belongs in the else block?

Self::InvalidP2wpkhScriptPubkey => write!(f, "provided script pubkey is invalid for P2WPKH"),
Self::InvalidP2wshScriptPubkey => write!(f, "provided script pubkey is invalid for P2WSH"),
Self::InvalidP2trScriptPubkey => write!(f, "provided script pubkey is invalid for P2TR"),
Self::InvalidScriptPubKey(e) => write!(f, "provided script was not a valid type of {e}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:

Suggested change
Self::InvalidScriptPubKey(e) => write!(f, "provided script was not a valid type of {e}")
Self::InvalidScriptPubKey(type) => write!(f, "provided script was not a valid for {type}")

@spacebear21 spacebear21 merged commit 4f04d45 into payjoin:master Jun 9, 2025
7 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.

3 participants