Pre-1.0 Doorkeeper for SessionOutcome in

[DanGould]

150 lines of release hygiene that seals the persisted SessionOutcome format before 1.0 freezes it

• drop a never-read witness payload
• make the enum is non_exhaustive

so that after 1.0's db compatibility commitment we can add terminal outcomes (like the settlement classifier's in #1692) additively in 1.x instead of via a semver break plus a db migration. no behavior changes, and the cost is one more rc.

Pre-change:

• The witness payload is dead data (no accessor, no ffi method, no history getter reads it)
• SessionOutcome is currently exhaustive, unlike SessionEvent, so after 1.0 you can't add a terminal variant or retrofit #[non_exhaustive] without a breaking change

Co-authored by Claude Code

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.

[coveralls]

Coverage Report for CI Build 29807705209

Coverage decreased (-0.01%) to 86.284%

Details

• Coverage decreased (-0.01%) from the base build.
• Patch coverage: 3 uncovered changes across 1 file (19 of 22 lines covered, 86.36%).
• No coverage regressions found.

Uncovered Changes

File	Changed	Covered	%
payjoin-cli/src/app/v2/mod.rs	3	0	0.0%
Total (3 files)	22	19	86.36%

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	16010
Covered Lines:	13814
Line Coverage:	86.28%
Coverage Strength:	344.55 hits per line

---

💛 - Coveralls

[spacebear21]

This seems like a nice path that leaves the door open for non-breaking 1.x Monitor fixes. However, there are 4 distinct components to this PR and I think each deserves standalone attention:

1. ACK Reserve the SessionOutcome::Othervariant - this is the actually useful part, but the justification in the docstring and commit message argues for the wrong thing. "binaries pinned to this persisted format can still parse event logs written by a later minor release that does emit it" is a fine side-effect but forwards-compatibility is not the thing we actually care about (even the docs in persist.rs say "Forward compatibility in general is not appropriate"). Backwards compatibility is the goal. The reason to reserve Other isn't data compat at all, it's API semver: reserving the name now makes the classifier additive by construction. That argument doesn't appear in the PR.
2. cACK Change SessionOutcome::Success to a unit variant. This one requires historical context: the original monitor (0c67af7) recorded an empty witness on the non-segwit path, the path where it actually would have mattered to store those as the only way to reconstruct the final payjoin transaction (see comment). #1218 then deleted that path entirely, so the stored event payload is indeed dead code now. However, #1692 would restore non-segwit detection, which would make this event payload undead once again. I think an argument can be made for only storing a Success(Txid) instead of the full script_sig/witnesses. This would also make this PR self-consistent: as written we record the txid of a transaction we don't recognize (Other(Txid)) and nothing for the payjoin we do.
3. NACK Make SessionOutcome #[non_exhaustive]. This reverses 8c92050, which explicitly named SessionOutcome in the keep-exhaustive list, without engaging with that rationale, so I'm not sure whether this is intentional or an accidental omission/hallucination. The SessionOutcome::Other variant already grants exactly one named extension. I think any additional unforeseen outcomes should break at compile time instead of leading to unexpected behavior (e.g. as written any future unrecognized outcome would render as "Session closed" in payjoin-cli, with no compile error).
4. NACK Add success() and aborted() constructors on SessionOutcome. I don't understand the rationale for this. This seems to simply accomodate a payjoin-cli implementation detail for history display? I'm not even sure that implementation is correct, currently it looks like the sender just hardcodes aborted() it to display "Session aborted" if a replay error occurred.

[DanGould]

1. ACK Reserve the SessionOutcome::Other variant
2. NACK Make SessionOutcome #[non_exhaustive]

Adopted. docstring & commit message now refer to semver compat & exhaustive rationale so that we don't regress.

2. cACK Change SessionOutcome::Success to a unit variant.

Adopted as suggested including the #1218 reference AND the rationale for only Txid in the variant in the commit history. It keeps it simpler to let an underlying wallet handle saving the witnesses while our lib only stores the Txid as a link to that wallet state.

4. NACK Add success() and aborted() constructors on SessionOutcome

Reverted as suggested, please ignore. payjoin-cli does seem limited here in that it presents Closed(Aborted) as a placeholder rather than saying the session state is unknown. Seems like the RowStatus should be an Option or an Enum to make known when session state is really unknown in the display.

---

Just one commit now.

[spacebear21]

ACK c91f1d1 and confirmed this change allows #1692 to be rolled out additively with no breaking changes.

I made some changes:

1. Renamed the Other(Txid) variant to Unrecognized(Txid) - IMO "Other" is too vague and connotes some kind of miscallaneous outcome when it's in fact a specific identified spend. Both doc strings already used the "unrecognized" terminology.
2. Deleted superfluous comments and unit test.
3. Simplified the cli display to not print the txid in history status line. I think adding txid as a standalone column would make sense in a follow-up for success/fallback/unrecognized tx outcomes.

[DanGould]

rebased after #1753 to fix ci

[spacebear21]

reACK

[caarloshenriq]

tACK 4548e25

Unit tests in payjoin::receive::v2::session and payjoin::receive::v2 covering the new Success(Txid) and Unrecognized(Txid) variants pass, as do the two e2e integration tests in payjoin/tests/integration.rs that assert the settlement txid.

[benalleng]

Is this in theory what would occur if a regular transaction was sent to the same address as the one listed in the bip21 outside of the payjoin flow?

[spacebear21]

Not quite, it occurs if one of the sender's inputs is spent in any transaction that is not the payjoin or fallback tx, thus invalidating the payjoin session (receiver can't get paid with an already spent txo)

[benalleng]

ACK 4548e25