Remove redundant fields from WantsInputs event

[arminsabouri]

The primary information obtained from WantsOutputs is the outputs being commited. The session event should reflect new primary information only.

Related ticket: #1103

Pull Request Checklist

Please confirm the following before requesting review:

• I have disclosed my use of
  AI
  in the body of this PR.
• I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

[DanGould]

I see why you preferred the field to this in #1102 because the feature flag can be avoided. Though I'm not sure why you're using pub(crate) when pub(super) seems sufficient.

[arminsabouri]

Good point. Pub(super) would work fine here.
Last push removes the feature flagged getter infavor if a pub(super) field.

[coveralls]

Pull Request Test Coverage Report for Build 17981042076

Details

• 15 of 15 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.02%) to 84.54%

---

Totals
Change from base Build 17960911092:	0.02%
Covered Lines:	8591
Relevant Lines:	10162

---

💛 - Coveralls

[DanGould]

ACK 35ed7d7

[DanGould]

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.

[spacebear21]

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.

[DanGould]

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

[arminsabouri]

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.

[spacebear21]

utACK 35ed7d7