Cli Refactor#1737
Merged
Merged
Conversation
Expiry was handled in six places with three different message shapes: bespoke finalize_expired_sender/receiver helpers, inline branches in resume_payjoins, cancel_*_session, handle_error, and long_poll_fallback. Delegate every expiry site to cancel_sender_session/cancel_receiver_session, which already know how to unwind every session typestate and both replay outcomes. Replay re-detects expiry from the event log, so the in-memory typestate can simply be dropped at the RelayPost::Expired sites. The cancel functions' expired-replay branches now share one finalize_expired_session helper that reports the fallback transaction when the event log has one and closes the session either way. Previously an expired session with no fallback (e.g. a receiver that never got an original payload) returned an error without closing, which would have made resume retry it forever. Also fix two copy-paste bugs in cancel_receiver_session: it closed with role "sender" and reported replay failures as sender sessions. close_failed_session now takes the Role enum so the mismatch cannot recur.
Session-scoped messages were inconsistent: some embedded the session
id mid-sentence, some named the role, and many ("Posted Original
PSBT...", "No response yet.") had neither, making interleaved output
from concurrently resumed sessions impossible to attribute.
Route all session-scoped printing through a print_session helper that
prefixes every line with "[<Role> <session id>]", padding the role to
the widest form so ids and messages align in columns. A SessionPrint
extension trait implemented for the sender and receiver persisters
lets call sites write persister.print(msg): the persister already
carries the session id, and its type determines the role, so a
message can never claim the wrong one. The resume completion loop and
finalize_expired_session print via the free helper since they do not
hold a typed persister at that point.
Raw payloads (BIP21 URI, fallback transaction hex) stay on their own
unprefixed lines for clean copy-paste. The "Payjoin sent" print moves
from the shared process_pj_response to its call sites so v2 can label
it while v1 output is unchanged. Receiver interrupt messaging now
gives the same resume/cancel instructions as the sender's.
Update e2e assertions that parsed the old formats: session ids are
read from the bracket prefix, and txid extraction uses the last token
rather than a fixed index.
Replace the loops in the v2 sender and receiver flows with a driver: each step function performs one state transition and returns the next SendSession or ReceiveSession state, and process_sender_session/process_receiver_session loop over dispatch until the session terminates. Wrap process_receiver_session in a select against the interrupt channel in receive_payjoin, mirroring how send_payjoin guards the sender driver. This is safe at any phase: transitions persist to the event log with no await between transition and save, so an interrupted session resumes from its last saved state via the resume command. The interrupt message now includes the session id, and read_from_directory becomes a plain single-step function like the other state handlers.
Collaborator
Coverage Report for CI Build 29369157815Coverage increased (+0.4%) to 86.187%Details
Uncovered Changes
Coverage Regressions3 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
benalleng
approved these changes
Jul 15, 2026
benalleng
reviewed
Jul 15, 2026
| } | ||
| } | ||
|
|
||
| #[allow(clippy::incompatible_msrv)] |
Collaborator
There was a problem hiding this comment.
nit: I don't think this is actually necessary
Collaborator
There was a problem hiding this comment.
same with send_payjoin but we can remove these another time
This was referenced Jul 15, 2026
Closed
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This refactor covers 3 items, each which is addressed with a corresponding commit:
Expiry handling was updated to encapsulate the logic in a single, robust location which will eliminate potential variance at each call site.
Unifying the stdout messages is a nice enhancement that improves the readability of the cli and eliminates variance in the message format. This item is a follow up from #1700 (review).
Lastly, replaced the loops in the v2 sender and receiver flows with a driver: each step function performs one state transition and returns the next SendSession or ReceiveSession state, and process_sender_session/process_receiver_session loop over dispatch until the session terminates. This is now feasible because the transient errors return the next state to be processed. This item is a suggested follow up from #1724 (comment).
Supersedes #1734.
Claude Fable was used to assist in creating these code changes.
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.