Ship InMemoryPersister to close #1519#1528
Conversation
Move the in-memory persister out of the feature-gated test_utils module so external consumers can use it directly. The sync persister is now publicly available at payjoin::persist::InMemoryPersister without any feature gate. The async companion is renamed to InMemoryAsyncPersister but stays cfg(test)-only since tokio remains a dev dependency and only internal unit tests touch it. Update the payjoin-test-utils re-export and all in-tree consumers to import from the new location.
InMemoryPersister::default() is a strict superset of NoopSessionPersister: it accumulates events instead of discarding them, but no existing test asserts the no-events-saved behavior. Swap every call site over and tidy the variable names so the next commit can delete NoopSessionPersister.
Drop NoopSessionPersister and NoopPersisterEvent now that every caller uses InMemoryPersister::default(). InMemoryPersister gives the same drop-in ergonomics for callers that ignore the event log, plus the option to inspect events when needed. Swap the payjoin-ffi public re-export to InMemoryPersister so downstream FFI consumers keep a usable persister type.
Nothing in the payjoin crate is gated on this feature anymore now that InMemoryPersister is public unconditionally. Drop the feature declaration and stop requesting it from payjoin-test-utils. The unrelated _test-utils feature on payjoin-ffi is left alone; it gates the BitcoindEnv / TestServices uniffi exports used by FFI test scripts.
|
cACK. noting for follow-up: Each language binding reimplements InMemoryPersisters for the receiver and sender in tests, we could do a big code deletion there now that it's exposed. |
|
Also want to say that while the commit that moves InMemoryTestPersister should be mechanically identical, I sicced robots to check explicitly and the gaze down the line suggests that is indeed what changed here. |
Coverage Report for CI Build 25451287814Coverage increased (+0.03%) to 85.169%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
is it necessary for FFI stuff? if it was possible to split into a sharable reader that can replay the event log but not persist new events and a single writer that has ownership? something based on a channel abstraction that ensures there's only a single producer but allows multiple readers seems like a safer thing for sharing session events with say a GUI |
|
|
|
If we want to write from two threads, I think the change might need to apply to the regular All this is to say it may make more sense to EITHER
since review burden on this is the same either way (we need another PR), I'm getting clankers to write option 2 and we can just remove the footgun. |
4 participants
Used Claude Opus 4.7 on max effort by driving a modified version of Jesse Posner's
/meta-agentskill that had the #1519 context of the issue and preceding comment and a Q&A session with me marshaled to a worker commit-gated by a pre-commit hook that resembles CI. Meta strangely had the worker produce changelog updates in commits, which I had to manually remove because that's just not our process.Closes #1519 with a net delete!
Pinging @Arowolokehinde, who stepped up to help on this one, and looking forward to your review. I appreciate you! But I don't want to block on you so no big pressure, and formally requesting @spacebear21 as the ultimately responsible reviewer for this PR.
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.