Skip to content

Conversation

@arminsabouri
Copy link
Collaborator

@arminsabouri arminsabouri commented Aug 27, 2025

The initial typestate has always been an odd case. Stricly speaking you shouldn't be able to resume to an uninitialized state. The earliest state you can resume from is Initialized. So this commit removes UnInitialized as a "state" and replaces create_session with a builder model.


Note that Uninitialized is still a variant on the RecevierSession which represents a state before any events have been applied. I think there is value to removing the Uninitialized variant as applications have to still match on it when resuming. Instead we could have pub(crate)'d struct method serving the same purpose. Where the private struct method can only process the first event and return the RecevierSession sum type.
Cherry picked off #995

Pull Request Checklist

Please confirm the following before requesting review:

  • A human has reviewed every single line of this code before opening the PR (no auto-generated, unreviewed LLM/robot submissions).
  • I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

@arminsabouri arminsabouri marked this pull request as draft August 27, 2025 19:32
@coveralls
Copy link
Collaborator

coveralls commented Aug 27, 2025

Pull Request Test Coverage Report for Build 17304182660

Details

  • 40 of 43 (93.02%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.008%) to 85.78%

Changes Missing Coverage Covered Lines Changed/Added Lines %
payjoin/src/core/receive/v2/mod.rs 37 40 92.5%
Totals Coverage Status
Change from base Build 17301530031: 0.008%
Covered Lines: 8041
Relevant Lines: 9374

💛 - Coveralls

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.

I dig the first commit (which passes tests)

The second one I think is more complicated than having a simple Uninitialized state (+26 -9). The empty Uninitialized state seems good enough for sender and I'd rather leave it for the receiver too. ReceiverBuilder design LGTM. I just ask that it's build is chained where possible and not assigned unless it needs to be in one case.

Comment on lines 161 to 167
let receiver_builder =
ReceiverBuilder::new(address, self.config.v2()?.pj_directory.clone(), ohttp_keys)?
.with_amount(amount);
let persister = ReceiverPersister::new(self.db.clone())?;
let session = Receiver::create_session(
address,
self.config.v2()?.pj_directory.clone(),
ohttp_keys,
None,
Some(amount),
)?
.save(&persister)?;
let session = receiver_builder.build().save(&persister)?;

Copy link
Contributor

Choose a reason for hiding this comment

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

I think let session = ReceiverBuilder::new() ..build().save(&persister)?; All chained is the way to go

@DanGould
Copy link
Contributor

If one of the states is to go, I think it's the Receiver<UninitializedReceiver> state rather than the SessionEvent::Uninitialized.

@DanGould
Copy link
Contributor

The way to get rid of that Receiver<UninitializedReceiver> is by following the v2::SenderBuilder pattern and having the ReceiverBuilder create the actual receiver instead of only the event:

pub fn build_non_incentivizing(
self,
min_fee_rate: FeeRate,
) -> Result<NextStateTransition<SessionEvent, Sender<WithReplyKey>>, BuildSenderError> {
let psbt_ctx = self
.psbt_ctx_builder
.build_non_incentivizing(min_fee_rate, self.output_substitution)?;
Ok(Self::v2_transition_from_psbt_ctx(self.pj_param, psbt_ctx))
}
/// Helper function that takes a V1 sender build result and wraps it in a V2 Sender,
/// returning the appropriate state transition.
fn v2_transition_from_psbt_ctx(
pj_param: PjParam,
psbt_ctx: PsbtContext,
) -> NextStateTransition<SessionEvent, Sender<WithReplyKey>> {
let with_reply_key = WithReplyKey::new(pj_param, psbt_ctx);
NextStateTransition::success(
SessionEvent::CreatedReplyKey(with_reply_key.clone()),
Sender { state: with_reply_key },
)
}
}

@DanGould
Copy link
Contributor

If you have a strong preference to only build the transition, then we should be changing the SenderBuilder to adopt the same pattern as the receiver once this goes in.

@arminsabouri arminsabouri marked this pull request as ready for review August 28, 2025 18:00
@arminsabouri arminsabouri changed the title WIP - Add receiver builder Add receiver builder Aug 28, 2025
The first typestate has always been an odd case. Stricly speaking you
cannot resume from the uninitialized state. The earliest state you can
resume from is `Initialized`. So this commit removes UnInitialized as a
"state" and replaces it with a builder.

Additionally a builder model allows the application to access
the short id after the builder is created. The application may use
session id as the key for the perister which is needed when you build().
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.

utACK abfc6d3

This makes sense and mirrors SenderBuilder.

let's see about removing the ReceiveSession::Uninitialized (now with no contents) in a follow up along with SendSession to see if congruence makes sense.

@DanGould DanGould merged commit f079815 into payjoin:master Aug 28, 2025
14 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