search

payjoin / rust-payjoin

Supercharged payment batching to save you fees and preserve your privacy
https://payjoindevkit.org
85 stars 36 forks source link

Use sled Database to persist data #281

Closed DanGould closed 3 months ago

DanGould commented 4 months ago

TL;DR We keep including more and more persistent state in a miscellaneous .json files. An embedded db gives us the opportunity to fix this.

We need to make async payjoin-cli sessions pleasant to use.

Using payjoin-cli completely asynchronously, including program shutdowns, is a bit clunky and subject to at least one complaint https://github.com/payjoin/rust-payjoin/issues/267.

I revisited the experience by recording a screenshare of doing it and have to say I agree. There are a few problems:

  1. Only a single send and receive session are ever saved and not overwritten
  2. The sessions don't properly expire
  3. A manual --retry flag is required to resume a prior session

This change is the first of a handful of changes to fix this problem. By using a database, we can handle persistent data in a single file and interact with high performance. To provide a good experience we'll want to address each of the above issues.

  1. Persist multiple payjoin sessions and resume them as appropriate
  2. Expire sessions that will not complete
  3. Automate session resumption when the program comes back online

By addressing these and later including our solutions in the io feature we can create a reference implementation and toolkit for consumers to implement Payjoin V2 easily and appropriately.

This is the first step to fix the payjoin-cli async experience problem.

DanGould commented 4 months ago

Note, this database structure is almost definitely not what we'll end up using long term, but 1. payjoin sessions are short-lived, and losing them doesn't cause a huge disruption 2. we're in alpha anyway, so we can tolerate this kind of breaking change and 3. I plan to have a better structure out before we release payjoin-cli-0.0.7-alpha. Working on that immediately after this as a follow up.

DanGould commented 3 months ago

I changed 3900859 to store recv_sessions and send_sessions a tree. The other DB get methods just return the first element, and clear still clears. This is prep for #283 and fixes --retry in my manual testing.

Before the next version of payjoin-cli is released to fix async payjoin UX, we're going to need to have a test. I'd like to see an integration test with all of the networking / db implementation in the payjoin/io feature to test it without CLI dependencies. But either payjoin-cli or payjoin integration test would both work. I think we're better off adding the automated test in #283 since the interface is going to change quite a bit.

spacebear21 commented 3 months ago

Agreed, I can take a stab at adding the async integration test on top of #283

spacebear21 commented 3 months ago

I made an initial attempt at adding this to tests here. Not sure if it's sufficient but it at least catches the error fixed by #284 as it stands.

DanGould commented 3 months ago

@grizznaut I didn't realize you made a PR into the PR branch. Could you open a new one on this repo? https://github.com/DanGould/rust-payjoin/pull/3 seems like a sound addition to integration tests