Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NUT-05: add new state enum, deprecate paid. NUT-05 + NUT-08: use PostMeltQuoteBolt11Response instead of PostMeltBolt11Response #136

Merged
merged 3 commits into from
Aug 9, 2024

Conversation

callebtc
Copy link
Contributor

@callebtc callebtc commented Jun 17, 2024

What

New enum field: state

This change to NUT-05 deprecates the paid field to PostMeltQuoteBolt11Response and replaces it with a state field that is a string enum with three possible values: UNPAID, PENDING, PAID.

  • "UNPAID" means that the request has not been paid yet.
  • "PENDING" means that the request is currently being paid.
  • "PAID" means that the request has been paid successfully.

New return type for /v1/melt/bolt11

We also replace PostMeltBolt11Response by PostMeltQuoteBolt11Response as the response of POST /v1/melt/bolt11 but in a backwards-compatible way.

This means

  • we add a new field payment_preimage to PostMeltQuoteBolt11Response, which holds the bolt11 preimage after a successful payment
  • to adjust for NUT-08, we add change to PostMeltQuoteBolt11Response, which returns overspent Lightning fees

Why

This change enables wallet to know whether a Lightning payment is still in flight, if the user closes the wallet during a payment. When the wallet comes back online, it can request the melt quote via GET /v1/melt/quote/bolt11/{quote_id} and check its state.

  • If the payment was successful in the mean time, the wallet also finds payment_preimage there.
  • If the mint supports NUT-08, the wallet also finds the change and can unblind the response. Previously, wallets would have to restore these tokens to get the overpaid fees back if the payment was interrupted.

Implementation

Mints

  • add new state field to PostMeltQuoteBolt11Response
  • keep the paid field around until all wallets update
  • replace PostMeltBolt11Response by PostMeltQuoteBolt11Response
  • which means, add payment_preimage
  • make according changes to NUT-08: add change to PostMeltQuoteBolt11Response
  • return PostMeltQuoteBolt11Response for POST /v1/melt/bolt11

Wallets

  • replace the paid field with state and check it instead

As long as paid is kept around, wallets can still function the same way as before, also when the POST /v1/melt/bolt11 is changed (since JSON is "backwards compatible" to new fields).

Tracking progress:

  • nutshell (mint backwards compatible)
  • CDK (mint backwards compatible)
  • cashu-ts
  • nutmix
  • gonuts
  • ...

@callebtc callebtc added the needs discussion Needs more discussion label Jun 17, 2024
05.md Show resolved Hide resolved
@callebtc callebtc added the ready Ready to merge label Jun 17, 2024
Copy link
Contributor

@elnosh elnosh Jun 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. Only thing, I think this warning: **Attention:** This call will block until the Lightning payment either succeeds or fails. This can take quite a long time in case the Lightning payment is slow. Make sure to **use no (or a very long) timeout when making this call**! can be removed given that now it can return pending.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on how the mint implements it. The mint may not return pending immediately and may still block.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, but still no need to have the warning. It can be confusing to read that the status of a melt could be UNPAID, PENDING or PAID and then reading the statement

This call will block until the Lightning payment either succeeds or fails

@minibits-cash
Copy link

Ack. Lightning payments should be modelled as async by the mint api.

@thesimplekid
Copy link
Collaborator

Implemented in CDK cashubtc/cdk#181, is backwards compatible

Copy link
Collaborator

@thesimplekid thesimplekid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 777886c

@callebtc
Copy link
Contributor Author

Ack. Lightning payments should be modelled as async by the mint api.

I suggest we add a new endpoint in a different NUT-05 PR that is async (returns immediately) so we can have both. This should be very easy to add to mints.

@callebtc callebtc changed the title NUT-05: add new state enum, deprecate paid NUT-05: add new state enum, deprecate paid, add payment_preimage to PostMeltQuoteBolt11Response Jun 26, 2024
@callebtc callebtc changed the title NUT-05: add new state enum, deprecate paid, add payment_preimage to PostMeltQuoteBolt11Response NUT-05: add new state enum, deprecate paid. NUT-05 + NUT-08: merge PostMeltBolt11Response to PostMeltQuoteBolt11Response Jun 26, 2024
@callebtc callebtc changed the title NUT-05: add new state enum, deprecate paid. NUT-05 + NUT-08: merge PostMeltBolt11Response to PostMeltQuoteBolt11Response NUT-05: add new state enum, deprecate paid. NUT-05 + NUT-08: use PostMeltQuoteBolt11Response instead of PostMeltBolt11Response Jun 26, 2024
@gudnuf
Copy link
Contributor

gudnuf commented Jun 28, 2024

Should there be a FAILED state? Otherwise, if the lighting payment fails would the lifecycle of a quote be UNPAID -> PENDING -> UNPAID?

@lescuer97
Copy link

I feel PENDING and UNPAID sound a little to close to each other. Maybe PENDING should be called something like ONGOING

@lescuer97
Copy link

Implemented in Nutmix

@callebtc
Copy link
Contributor Author

Should there be a FAILED state? Otherwise, if the lighting payment fails would the lifecycle of a quote be UNPAID -> PENDING -> UNPAID?

Correct, it would go back to UNPAID if it fails.

Copy link
Collaborator

@thesimplekid thesimplekid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 23db305

@callebtc callebtc merged commit de7faf0 into main Aug 9, 2024
@callebtc
Copy link
Contributor Author

callebtc commented Aug 9, 2024

LFG

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Needs more discussion ready Ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants