Skip to content

only fail UNPAID melt quotes#681

Merged
gudnuf merged 1 commit intomasterfrom
handle-retrying-pending-melts
Oct 20, 2025
Merged

only fail UNPAID melt quotes#681
gudnuf merged 1 commit intomasterfrom
handle-retrying-pending-melts

Conversation

@gudnuf
Copy link
Contributor

@gudnuf gudnuf commented Oct 10, 2025

Related to #674

In the linked issue you can see that we failed the send quote for reason "melt quote is not unpaid: pending". This error message comes from the mint. Somehow the call to melt proofs happened 2x before the send quote was updated to PENDING in our database which then resulted in us failing a send quote where the melt quote was in a PENDING state. If a melt quote is PENDING, that means the LN payment is still being attempted so we should not fail. Also if the melt quote is PAID we should be completing it not failing it.

@gudnuf gudnuf self-assigned this Oct 10, 2025
@gudnuf gudnuf requested a review from jbojcic1 October 10, 2025 19:24
@vercel
Copy link

vercel bot commented Oct 10, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
agicash Ready Ready Preview Comment Oct 20, 2025 3:56pm

💡 Enable Vercel Agent with $100 free credit for automated AI reviews

@supabase
Copy link

supabase bot commented Oct 10, 2025

This pull request has been ignored for the connected project hrebgkfhjpkbxpztqqke because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@gudnuf gudnuf added the P1 High label Oct 10, 2025

const latestMeltQuote = await account.wallet.checkMeltQuote(quote.quoteId);
if (latestMeltQuote.state !== MeltQuoteState.UNPAID) {
// If the call to melt proofs retries before the send quote is marked as pending in the agicash
Copy link
Collaborator

Choose a reason for hiding this comment

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

how does this happen? by killing the app/tab and opening it again while the send quote is still in unpaid state?

Copy link
Collaborator

@jbojcic1 jbojcic1 Oct 13, 2025

Choose a reason for hiding this comment

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

I don't think this is the right solution. I think the proper solution is to make cashu send service initiate send not throw if the quote is already pending or paid

what does mint return exactly if you call melt for already pending/paid melt quote?

Copy link
Collaborator

Choose a reason for hiding this comment

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

additionally we need to change completeSendQuote to allow completing the quote that is maybe still in unpaid state (if pending update was missed because the app was closed)

Copy link
Collaborator

@jbojcic1 jbojcic1 Oct 13, 2025

Choose a reason for hiding this comment

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

also regarding num 6 from the ticket (I told him to send his entire balance as ecash, and this immediately got marked as completed because he got back the original 2,048 sat proof which was already spent to pay the invoice) should we implement the check if the proofs was spent before creating the send swap?

similarly maybe we should do the same check before adding proofs back to account or decide how we want to handle this (we discussed that briefly on discord where we were thinking about other possible approaches as well like tracking all proofs)

Copy link
Contributor Author

@gudnuf gudnuf Oct 13, 2025

Choose a reason for hiding this comment

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

how does this happen? by killing the app/tab and opening it again while the send quote is still in unpaid state?

I don't know exactly. When it happened the app was still open, but we were at the airport so maybe slow internet. Generally, it will happen if initiateSend is called 2x, but I don't know why that would happen.

I think the proper solution is to make cashu send service initiate send not throw if the quote is already pending or paid

I don't think this helps. The problem is that initiateSend was called 2x

what does mint return exactly if you call melt for already pending/paid melt quote?

Nutshell returns an unspecified error code (I think 11000) and the error message that got set as the failure reason: "melt quote is not unpaid: pending".

Overall, to me it seems like a good idea to make sure that the mint quote is UNPAID before failing no matter what causes it because if we failed a PENDING or PAID quote we will lose money.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this helps. The problem is that initiateSend was called 2x

why do you think it doesn't help if when called second time we just swallow the error so the calls to initiate are idempotent (meaning you can call it any num of times) and fail won't be called

Overall, to me it seems like a good idea to make sure that the mint quote is UNPAID before failing no matter what causes it because if we failed a PENDING or PAID quote we will lose money.

it will be uncessary if we add check to only add unspent proofs to account, unless I am missing something

Copy link
Collaborator

@jbojcic1 jbojcic1 Oct 13, 2025

Choose a reason for hiding this comment

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

Maybe, but it seems better to make sure we only add unspent proofs to the account and figure out how to monitor them there. Otherwise we will have an extra network request when sending which we want to be as fast as possible

true but I don't like the current behavior where such swap gets created and just completed then. if we add that check we can avoid that

is there some way to detect this when trying to complete the swap and fail it instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it will be uncessary if we add check to only add unspent proofs to account, unless I am missing something

The way we lose money is by failing a send quote that will later complete because then we don't get the change. So we could add the check to not add spent proofs back to the balance which would keep our proofs correct, but we would still lose the change. Adding spent proofs to the balance doesn't lose money it just causes bugs

is there some way to detect this when trying to complete the swap and fail it instead?

Nothing I can think, we would have to check before.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nothing I can think, we would have to check before.

still you don't think checking before is worth extra latency for average (successful) scenario? idk I just find it very odd that we mark swap as completed in this case. I expect this to be very confusing for the users

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea if we can't be sure that all stored proofs are unspent then we should do the check. I create #684 for this

@gudnuf
Copy link
Contributor Author

gudnuf commented Oct 13, 2025

why do you think it doesn't help if when called second time we just swallow the error so the calls to initiate are idempotent (meaning you can call it any num of times) and fail won't be called

Ahh I think I misunderstood what you meant. So we can catch the error in initiate send after calling meltProofs, then if the error is due to the melt quote not being unpaid we ignore the error, is that right?

One problem with this I found when trying to do this just now is that when calling meltProofs in quick succession, I sometimes get this error first:

{
    "detail": "(sqlite3.IntegrityError) UNIQUE constraint failed: proofs_used.y\n[SQL: \n            INSERT INTO proofs_used\n            (amount, c, secret, y, id, witness, created, melt_quote)\n            VALUES (?, ?, ?, ?, ?, ?, ?, ?)\n            ]\n[parameters: (8, '0261f514a5645e640d8884e6483528f7200f574ed2c4f1c71a7a6633cd58d15aa8', 'b4e3a415374220389c82edaec3608bc0c8ab845f2a244210958718f9b0e29de3', '02beebaa7ea8ee1f8aee09e0b568b86a0f1c6b8ed82eb880ba815fbfa9a9bbedd3', '00e00f55075a92f5', None, '1760395586', 'ayW_arsL4LsIzBote2kDAjJ1AxntwX4I3D646u5s')]\n(Background on this error at: https://sqlalche.me/e/20/gkpj)",
    "code": 0
}

Also, the MintOperationError that we get is not standardized which is why I did the check in the fail method.

I think we will still need to check the quote state when we get an error to know for sure the state of the quote before deciding whether to throw the error or not. The other option is also look for this error "(sqlite3.IntegrityError) UNIQUE constraint failed: proofs_used.y", but that doesn't seem like a good idea.

I pushed my changes so you can see where I'm at. It does work because we have retries on the call to initiate send, so the first call fails with this sql error, then we retry and it fails with melt quote not paid.

@jbojcic1
Copy link
Collaborator

why do you think it doesn't help if when called second time we just swallow the error so the calls to initiate are idempotent (meaning you can call it any num of times) and fail won't be called

Ahh I think I misunderstood what you meant. So we can catch the error in initiate send after calling meltProofs, then if the error is due to the melt quote not being unpaid we ignore the error, is that right?

One problem with this I found when trying to do this just now is that when calling meltProofs in quick succession, I sometimes get this error first:

{
    "detail": "(sqlite3.IntegrityError) UNIQUE constraint failed: proofs_used.y\n[SQL: \n            INSERT INTO proofs_used\n            (amount, c, secret, y, id, witness, created, melt_quote)\n            VALUES (?, ?, ?, ?, ?, ?, ?, ?)\n            ]\n[parameters: (8, '0261f514a5645e640d8884e6483528f7200f574ed2c4f1c71a7a6633cd58d15aa8', 'b4e3a415374220389c82edaec3608bc0c8ab845f2a244210958718f9b0e29de3', '02beebaa7ea8ee1f8aee09e0b568b86a0f1c6b8ed82eb880ba815fbfa9a9bbedd3', '00e00f55075a92f5', None, '1760395586', 'ayW_arsL4LsIzBote2kDAjJ1AxntwX4I3D646u5s')]\n(Background on this error at: https://sqlalche.me/e/20/gkpj)",
    "code": 0
}

Also, the MintOperationError that we get is not standardized which is why I did the check in the fail method.

I think we will still need to check the quote state when we get an error to know for sure the state of the quote before deciding whether to throw the error or not. The other option is also look for this error "(sqlite3.IntegrityError) UNIQUE constraint failed: proofs_used.y", but that doesn't seem like a good idea.

I pushed my changes so you can see where I'm at. It does work because we have retries on the call to initiate send, so the first call fails with this sql error, then we retry and it fails with melt quote not paid.

Yes your current solution is what I had in mind. It sucks that mints don't handle errors properly so we have to do these weird workarounds. What could solve the issue here is to call check quote method when the error happens and then check the state. If the state is still unpaid it means it's an actual error (right?) if not we can consider it initiated and we can also use that melt quote response as a function response instead of having to extract the state from the error message.

@jbojcic1
Copy link
Collaborator

Regarding the fail function, we could still keep that check but I would have it throw instead of be a noop if state is not what we expect

@gudnuf
Copy link
Contributor Author

gudnuf commented Oct 14, 2025

@jbojcic1 I made initiateSend idempotent by checking the latest melt quote state before throwing and I also kept the check in the fail method, but changed it to throw if we got an unexpected state

@gudnuf gudnuf force-pushed the handle-retrying-pending-melts branch from 2c7d3f0 to ec87b60 Compare October 20, 2025 15:55
@gudnuf gudnuf merged commit 4d0ffed into master Oct 20, 2025
4 checks passed
@gudnuf gudnuf deleted the handle-retrying-pending-melts branch October 20, 2025 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 High

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants