Skip to content

Rename session events#1116

Merged
spacebear21 merged 2 commits intopayjoin:masterfrom
spacebear21:rename-session-events
Sep 29, 2025
Merged

Rename session events#1116
spacebear21 merged 2 commits intopayjoin:masterfrom
spacebear21:rename-session-events

Conversation

@spacebear21
Copy link
Collaborator

Follow up to discussion in #1106 (comment). Note that I stuck with past tense over imperative mood because IMO that indicates more clearly that this event happen-ed (e.g. RetrievedOriginalPayload would be difficult to express in imperative mood), also we already use past tense for Created and Closed.

SessionEvent variants should describe "what event happened" instead of "what this event produces". This makes the inner values less ambiguous and better abides by the principle that events only capture primary information.

We already do this for the Created and Closed variants. SessionInvalid was left as-is because it will be replaced soon.

Pull Request Checklist

Please confirm the following before requesting review:

@spacebear21 spacebear21 added this to the payjoin-1.0 milestone Sep 26, 2025
@spacebear21 spacebear21 self-assigned this Sep 26, 2025
@coveralls
Copy link
Collaborator

coveralls commented Sep 26, 2025

Pull Request Test Coverage Report for Build 18106191436

Details

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

Totals Coverage Status
Change from base Build 18045273466: 0.01%
Covered Lines: 8589
Relevant Lines: 10155

💛 - Coveralls

Copy link
Collaborator

@arminsabouri arminsabouri left a comment

Choose a reason for hiding this comment

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

cACK af66471

@DanGould
Copy link
Contributor

DanGould commented Sep 28, 2025

I'd really like to see verbs in Imperative mood here. An event is only past tense once it's actually been applied.

Scratch that. Past tense makes sense here because they're events that happened, not commands. Names make sense as-is.

Copy link
Contributor

@DanGould DanGould left a comment

Choose a reason for hiding this comment

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

As discussed offline, fn apply_... names still need updates

For example apply_maybe_inputs_owned must become apply_checked_broadcastable (Or really apply_checked_broadcast_suitability if we're keeping congruence with the check method names)

@spacebear21
Copy link
Collaborator Author

spacebear21 commented Sep 29, 2025

Changes since last review:

  • I changed the names of the variants CheckedBroadcastSuitability and CheckedNoInputsSeenBefore to match the transition method names.
  • The second commit updates the apply_event transition methods.

Note that CommittedFees doesn't match the method name apply_fee_range. This is intentional because I believe we should separate fee application from fee commitment similar to how we do in WantsOutputs/WantsInputs as discussed in #1108. I changed this to AppliedFeeRange to be consistent, per the discussion on dev call we likely won't use a commit_fees method.

@spacebear21 spacebear21 mentioned this pull request Sep 29, 2025
3 tasks
SessionEvent variants should describe "what event happened" instead of
"what this event produces". This makes the inner values less ambiguous
and better abides by the principle that events only capture primary
information.

We already do this for the `Created` and `Closed` variants.
`SessionInvalid` was left as-is because it will be replaced soon.
Rename the transition methods according to the corresponding events.
Copy link
Contributor

@DanGould DanGould left a comment

Choose a reason for hiding this comment

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

ACK 7586c53 LGTM

@spacebear21 spacebear21 merged commit 2b3d960 into payjoin:master Sep 29, 2025
10 checks passed
@spacebear21 spacebear21 deleted the rename-session-events branch September 29, 2025 21:15
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.

4 participants