Remove uninitialized session state

[arminsabouri]

Unintialized should be an unreachable state and we are misusing it today. If one does not have any events in their log and replays then they will receive an uninitilaized receiver. Which is not useful. Not having any events and deciding to replay an session is most likely a bug in the application and should get an error (NoEvents). Additionally, bc sessionState is pub applications have to match on uninitialized when processing their receiver states -- instead you should receive an error when you have no events in your log. The other reason Unintialized existed is to having a starting state when replaying the SEL. Which we can resolve by having a special carve out for the first sessionState (SessionState::new).

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]

Pull Request Test Coverage Report for Build 17583852116

Details

• 32 of 47 (68.09%) changed or added relevant lines in 5 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.07%) to 86.019%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/core/receive/v2/session.rs	9	10	90.0%
payjoin/src/core/send/v2/session.rs	9	10	90.0%
payjoin/src/core/receive/v2/mod.rs	8	10	80.0%
payjoin/src/core/send/v2/mod.rs	5	7	71.43%
payjoin/src/core/error.rs	1	10	10.0%

Totals
Change from base Build 17566690850:	0.07%
Covered Lines:	8232
Relevant Lines:	9570

---

💛 - Coveralls

[arminsabouri]

Seeking a concept ack/nack and an approach ack/nack.

[DanGould]

cACk

[arminsabouri]

@DanGould d83d478 expands the scope of this PR which you already Cack'd -- it creates a common replay error type for send and receive. Happy to cherry pick that off to another PR. Lmk

[nothingmuch]

perhaps it'd be simpler to leave process_event as a completely internal detail, and replace the new functions with two variants:

1. one that takes an iterator of the SessionEvent enum
2. one that takes just a statically typed initialization event (e.g. CreatedReplyKey for sender)

and the first would call the second after scrutinizing the enum variant

but most importantly neither should be pub, at most pub(crate), receiver's new is private but not sender's, so long as they aren't pub then we can worry about refactoring later

[arminsabouri]

perhaps it'd be simpler to leave process_event as a completely internal detail, and replace the new functions with two variants:

1. one that takes an iterator of the SessionEvent enum
2. one that takes just a statically typed initialization event (e.g. CreatedReplyKey for sender)

and the first would call the second after scrutinizing the enum variant

but most importantly neither should be pub, at most pub(crate), receiver's new is private but not sender's, so long as they aren't pub then we can worry about refactoring later

@nothingmuch Fixed the visibility issues. Thanks for pointing that out. I ended up scrutinizing the enum variant in replay_event_log. And the new method takes a static type used to create the first session state. Lmk if this is what you had in mind.

[DanGould]

I think the suggestion from @nothingmuch was to also make process_event a purely private function and have a new pub(super) process_events(mut logs: Iterator<SessionEvent>) that encapsulates the for loop (copied below for clarity)

Details

    let mut history = SessionHistory::default();
    let first_event = logs.next().ok_or(InternalReplayError::NoEvents)?.into();
    history.events.push(first_event.clone());
    let mut receiver = match first_event {
        SessionEvent::Created(context) => ReceiveSession::new(context),
        _ => return Err(InternalReplayError::InvalidEvent(Box::new(first_event), None).into()),
    };
    for event in logs {
        history.events.push(event.clone().into());
        receiver = receiver.process_event(event.into()).map_err(|e| {
            if let Err(storage_err) = persister.close() {
                return InternalReplayError::PersistenceFailure(ImplementationError::new(
                    storage_err,
                ))
                .into();
            }
            e
        })?;
    }

Other than that this LGTM. That encapsulation could be a follow up.

[arminsabouri]

@nothingmuch please drop another review

[spacebear21]

utACK e229cdd

@arminsabouri I'm thinking we should merge this and address any additional feedback in follow-ups, since a few other PRs build on top of this one.

[DanGould]

cACK

[DanGould]

I agree with @nothingmuch that this should be encapsulated in a process_events function that takes the [Into?]Iterator of events (aka logs) but that can be a follow up

[DanGould]

It could probably even be shared to de-duplicate code between the sender/receiver with some generic fn. Worth it if it's more beautiful and easier to maintain symmetry but not strictly neccessary

[nothingmuch]

agree on both counts (there is potential for refactoring but also since it's all private it doesn't need to be in this PR)

[nothingmuch]

ACK.

In regards to refactoring, I agree there seems to be some potential, but I can only envision doing that comprehensively with macros or heavy use of generics (for example shifting reply_key from the context to the typestate structs themselves is boilerplate heavy, but if we declare the state machine structs all at once then the macro could know that if a field is introduced in some state then all of the subsequent states also have the field). I'm not convinced it's worth the effort to try and remove all of these warts, some duplication or slightly hacky things are fine so long as they don't affect the public API, which this PR now simplifies.

[nothingmuch]

mainly out of curiosity what compelled this change?

[arminsabouri]

IIRC there was a ambigious definition for fmt once I imported Debug.