Codecov Report

Merging #1099 into master will decrease coverage by 0.23%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #1099      +/-   ##
==========================================
- Coverage    82.6%   82.36%   -0.24%     
==========================================
  Files         101      101              
  Lines        7692     7696       +4     
  Branches      301      307       +6     
==========================================
- Hits         6354     6339      -15     
- Misses       1338     1357      +19

Can you detail why that feature would be useful?
You will receive the UUID when submitting the payment, what's the use-case for generating it before-hand?

@t-bast it's to eliminate a possible case of:

1. app instructs Eclair to send a payment and gets UUID.
2. app only has UUID after payment is in process, tries to save it to app db (UUID -> app_user_id for example).
3. for whatever reason this fails so app has lost track of payment.

with this change enabled it becomes:

1. app creates a UUID, saves UUID -> app_user_id to app database before instructing Eclair to send it.
2. Either it's a success and app db definitely has a record or a failure and nothing happens.

@t-bast it's to eliminate a possible case of:

1. app instructs Eclair to send a payment and gets UUID.
2. app only has UUID after payment is in process, tries to save it to app db (UUID -> app_user_id for example).
3. for whatever reason this fails so app has lost track of payment.

with this change enabled it becomes:

1. app creates a UUID, saves UUID -> app_user_id to app database before instructing Eclair to send it.
2. Either it's a success and app db definitely has a record or a failure and nothing happens.

Have you observed eclair failing to return a UUID? I understand there is always a possibility of this happening but the payment code was designed to return the UUID as soon as possible and even before the payment is actually processed, looking at the interaction between the initiator and the lifecycle we could improve this by storing the OutgoingPayment even before we reach the lifecycle and return the UUID to the caller only after it has been [successfully] saved to the DB.

3. for whatever reason this fails so app has lost track of payment.

It seems to me that this is what you really want to fix.
I don't see a valid reason why this would fail permanently, can you detail?

To further clarify, in "3. for whatever reason this fails" I specifically mean my app failing to save UUID to app db, not Eclair failing to return a UUID (Eclair failing would be fine as will result in exception on app side which would also halt further progress).

I don't see a valid reason why this would fail permanently

For example, app db may get corrupted (or disk detached etc) which would allow users to withdraw funds without recording them in app db.

For example, app db may get corrupted (or disk detached etc) which would allow users to withdraw funds without recording them in app db.

But in such extreme cases you would basically have everything failing right?
Correct me if I'm wrong, but the change you propose would only be useful in the case where:

• the DB works fine while you generate the UUID client-side and save it
• then the DB starts failing after you saved the UUID but before eclair returns the acknowledgement for payment submit (which would currently give you back the UUID)
• and the DB starts working again right after

This looks like quite a rare event honestly. And you still have the payment hash that allows your app to discover that payments were successfully made (by querying eclair for succeeded payments after the app DB is recovered).

I'm not a big fan of letting clients generate that value TBH, so I'd like to make sure this really fixes an existing pain point before doing it. It seems to me that this can be recoverable from the app's side entirely without that change...

@t-bast it's important to specify which db we mean here: one is Eclair internal db and it's irrelevant, the other one is my app db where I link payments to users' IDs. A worst case which is currently possible is:

1. My app instructs Eclair to send a payment and then in turn fails to save it to it's own db (because there is a bug in my app or some external reason).
2. I can see Eclair made some payments but don't know which user they belong to.
3. This can last for arbitrarily long time until noticed.

So basically from my app perspective it's "send, then (maybe) save", I want it to be possible to "(maybe) save, then send if saved successfully".

It's true that I have a payment hash before making a payment and can rely on it, but it feels awkward for these reasons:

1. I would have two payment identifiers instead of one (plus payment hash not really an identifier as users may provide invoices with same hashes, that's one of reasons why UUID is there at all).
2. If I do want to have a UUID recorded I can't have it right away: I need to save a withdrawal record in app db with hash only and then update it with UUID, this complicates things.
3. In future there may be scenarios where payment hash does not represent a whole payment (perhaps OG AMP which uses many hashes for chunks or whatever unknown development which comes up) while UUID always represents (or at least should) a payment as a whole.

Ok I have a better understanding of your use-case and the failure.
It does seem to be a good quick-fix for your issue, but I think it may hide more important issues in your app.

It seems to me that if you fail to save something as important as a paymentId -> userId mapping to the app DB, the app should halt and stop accepting payment requests until that's fixed. Otherwise I'm afraid a whole lot of other issues will arise anyway.

Can you detail why/how your app would keep running and accepting payment requests while the app DB is failing?

If the app halts until the app DB is available again, then you don't need to generate the UUID client-side and I think you avoid a lot of potential recovery issues. But I'm probably missing more context from your app's point of view?

Well in my case app does halt completely once any db error happens but even in this situation the very first problematic withdrawal will still get processed by Eclair before halting my app a few moments later, and it may happen to be a large withdrawal.

Also I can imagine an app which does it's main thing in one process and withdrawals in another one, with this change applied app can be designed such that it may continue doing it's main thing while withdrawals fail while now it really has to shut down completely.

Well in my case app does halt completely once any db error happens but even in this situation the very first problematic withdrawal will still get processed by Eclair before halting my app a few moments later, and it may happen to be a large withdrawal.

But you don't have an issue in this case then, you are at a failing point where you have (in-memory) both the payment ID returned by eclair and the user ID. So you can save this anywhere (app storage, special log, etc) to reconcile once the DB is back online (or if the app keeps running until the DB is back online, it can simply be kept in memory in a hanging routine).

Also I can imagine an app which does it's main thing in one process and withdrawals in another one, with this change applied app can be designed such that it may continue doing it's main thing while withdrawals fail while now it really has to shut down completely.

While I agree in principle, in reality we are talking of a very specific failure (the DB is failing). And I expect that if the DB is failing, even in the app's main thing you would need a DB to do any meaningful side-effect task so that would be failing as well...

Sorry for nitpicking on this, but I wanted to understand in more details what your app was currently doing to see if I could suggest a change that would provide bigger overall benefits than fixing this specific case in eclair (because I think handling an unavailable/failing DB is a problem that concerns most features of an app and shouldn't be fixed feature by feature but rather holistically). This doesn't mean I'm completely opposed to this PR, don't worry ;)

I think this is a typical case of db inconsistencies. The standard way to solve this would be to offer a custom_id that you would use to store the paymentId -> userId. In case of app failure you would be able to sync after restart.

I'm also not very keen on letting an external user/app choose internal payment identifiers (NB: in any case you should have checked for duplicate keys in PaymentLifecycle.when(WAITING_FOR_REQUEST) { }.

@pm47

The standard way to solve this would be to offer a custom_id that you would use to store the paymentId -> userId.

Is this something a client app or Eclair should do?

Is this something a client app or Eclair should do?

This is a bit of both: your app will send us this value in the send API (in your case you could use your userID), and eclair should add this value in the payment row in the payment DB. We're trying to minimize DB schema changes to avoid having too many migrations: I can probably add this customer_id to the paymentDB in my next PR that will change the payment DB schema. @pm47 what do you think?

#1130 offers an externalId that your application can set.
This should let you resolve this issue ;)