Async persistence

[spacebear21]

It is becoming clear that we need to ship an async persister trait alongside the blocking one to accommodate real developer needs. We've been discussing this in #816 and most recently this need came up again regarding the LDK-node integration.

This PR introduces the AsyncSessionPersister trait as an async counterpart to the synchronous SessionPersister, along with the save_async method for persisting state transition results.

Leaving as draft because the current approach makes no attempt to refactor any internal logic and is rather WET, but it gives us a foundation for the new API surface and async unit tests to validate it. Claude code wrote most of this copypasta (sloppypasta?).

It seems to me the DRYest approach will involve macros (see lightning-macros for some async macro examples). I'm not super thrilled about the prospect because persist.rs is already the most abstract part of the codebase, and while macros are cool they're not exactly easy to reason about. I'd like to get other opinions before going down a refactoring route cc @arminsabouri @nothingmuch @DanGould

EDIT: we will also need to do this for the payjoin-ffi JsonReceiver/SenderPersisters which are not covered in this draft.

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 20388045567

Details

• 460 of 470 (97.87%) changed or added relevant lines in 1 file are covered.
• 2 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.2%) to 83.554%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin/src/core/persist.rs	460	470	97.87%

Files with Coverage Reduction	New Missed Lines	%
payjoin/src/core/persist.rs	2	96.27%

Totals
Change from base Build 20347892595:	0.2%
Covered Lines:	9831
Relevant Lines:	11766

---

💛 - Coveralls

[nothingmuch]

concept ACK.

for dealing with the extension trait duplication without macros implementing the moral equivalent of keyword generics, which i agree is too complex, the various methods of the extension trait can be replaced non-pub methods of the transition result objects instead

so for example:

persister.save_maybe_fatal_or_success_transition(self).await

would instead be something like:

let (outcome, event) = self.deconstruct()?;
persister.save_event(event);
Ok(outcome)

if the various deconstruction methods are pub(crate) or private then the abstraction boundaries are still respected, and the destructuring and control flow logic is identical for both async and non async

this would require only minimal boilerplate to be duplicated between the two "flavors" of function/trait

[nothingmuch]

async methods in traits are supported since 1.75, so within our msrv, so i think this can just be an async fn

does this need trait objects for a reason i'm missing?

[spacebear21]

I had the same thought, but according to the linter:

error: use of `async fn` in public traits is discouraged as auto trait bounds cannot be specified
   --> payjoin/src/core/persist.rs:531:5
    |
531 |     async fn save_event(&self, event: Self::SessionEvent)
    |     ^^^^^
    |
    = note: you can suppress this lint if you plan to use the trait only in your own code, or do not care about auto traits like `Send` on the `Future`
    = note: `-D async-fn-in-trait` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(async_fn_in_trait)]`
help: you can alternatively desugar to a normal `fn` that returns `impl Future` and add any desired bounds such as `Send`, but these cannot be relaxed without a breaking API change
    |
531 ~     fn save_event(&self, event: Self::SessionEvent)
532 ~         -> impl std::future::Future<Output = Result<(), Self::InternalStorageError>> + Send;
    |

InternalAsyncSessionPersister is private so it can and does use the async fn syntax.

[nothingmuch]

This still allows the use of async fn within impls of the trait. However, it also means that the trait will never be compatible with impls where the returned Future of the method does not implement Send.

hmm, IIUC i think that's fine for our use case since event data is value typed and that's all the persister deals with, but i'm not confident in this assement

assuming this stays desuraged, please add a comment linking to the warning's docs or at least mentioning its name

[DanGould]

The release notes and description of the error message are here. The desugared version seems fine, though the trait-variant macro crate option seems viable especially because it comes straight from the rust-lang org. IIRC Lexe is a multithreaded environment that would consume these async functions where the Send auto trait would be critical.

[arminsabouri]

Concept ACK
I had note about the test suite (happy to hack on that part with you as I originally wrote it). Most of the meat of this PR is going to be de-dup'ing the internal logic for handling the different state transition objects -- which I imagine claude will be pretty good at.

Thanks for starting this up!

[arminsabouri]

Do we need an async version of the test harness and test cases? I would think we can refactor this test suite so we can run the same tests with a sync and async persister. And the results should be equivalent.

[DanGould]

We discussed a bit by signal and I think adding the async API serves a real demand.

Regarding the first commit, excuse my slow climb back into programming here and let me ask: is refactoring those errors a necessary part of this PR or is it mostly unrelated?