Fix depleted daily limit from unspent hits

[iWarpBTC]

If an invoice payment failed (for example an over-limit payment is attempted, or just there's no route) this is still recorded as a spent hit and it may deplete the daily limit. This PR fixes it.

Also now payment request of precisely maxWithdrawable amount doesn't pass since this is probably a PoS error or stealing, not a common payment.

[gorrdy]

Tried it and it's working as expected.

[gimme]

If an invoice payment failed (for example an over-limit payment is attempted, or just there's no route) this is still recorded as a spent hit and it may deplete the daily limit. This PR fixes it.

I don't see the code that fixes this.

Also now payment request of precisely maxWithdrawable amount doesn't pass since this is probably a PoS error or stealing, not a common payment.

Could you create a separate PR or issue for this? I think it needs some discussion.

[gorrdy]

You can see the code that fixes sit here. It is pretty straight forward.
https://github.com/lnbits/boltcards/pull/4/files

[gimme]

I can only see the code for the unrelated issue, not what the title claims to fix.

[gorrdy]

Basically what this PR does is that it checks if the amount in invoice is lower than the card limit.

Currently it tries to do the payment but the payment obviously fails (if higher than the card limit). This would be ok but it also counts the amount as spent which is

1. not true
2. the payment counts to the daily limit - other payments may not go through because of the previous failed payment.

It doesn't make sense to try payment that i guaranteed to fail (above card tx limit). That's why there is the if statement at the beginning. Also it resolves the point 2. It won't mark the hit as spend.

if invoice.amount_msat < card.tx_limit * 1000:
         hit = await spend_hit(id=hit.id, amount=int(invoice.amount_msat / 1000))
         assert hit
         try:
             await pay_invoice(
                 wallet_id=card.wallet,
                 payment_request=pr,
                 max_sat=card.tx_limit,
                 extra={"tag": "boltcard", "hit": hit.id},
             )
             return {"status": "OK"}
         except Exception as exc:
             return {"status": "ERROR", "reason": f"Payment failed - {exc}"}
     else:
         return {"status": "ERROR", "reason": "Amount exceeds card limit"}

[gimme]

Oh, I see what you mean, but it doesn't fix if the payment fails for another reason than tx limit (e.g., like you mentioned, if there's no route).

[gorrdy]

This is true but it is not that big of a deal if the payment is reasonable amount. The problem is someone can disable your bolt card/bolt ring for a day just by a simple tap and requesting more sats than allowed.

It is not a big deal if there is 10k sats more of 300k daily limit spent... But this should also be resolved, that's right.

[iWarpBTC]

I believeI believe it would be quite safe to "unspent" a hit when an Exception occurs?

[gimme]

It should be pretty simple to only add the hit if the payment succeeds.

Otherwise, I guess it's fine to fix this first, but I still think we shouldn't prevent requests for precisely maxWithdrawable without more discussion about that.

[gimme]

I believeI believe it would be quite safe to "unspent" a hit when an Exception occurs?

Yes, I think so

[bota87]

What's blocking the merge of this PR?

[prusnak]

What's blocking the merge of this PR?

It needs to be rebased on top of the current main branch. Are you up for the task?

[bota87]

It needs to be rebased on top of the current main branch. Are you up for the task?

Not really (I don't know python) but I tried anyway.
The rebased version is here but I don't know the workflow to add commits to this PR nor if I have permissions to do so

[CabtonWithaK]

issue still persists

state before

for example unsuccesful payment/withdrawal in Wallet of Satoshi

state after

AS IS: both for "daily limit reached" or "internal invoices only" the boltcard scan is recorded and counts for the daily limit
TO BE: both for "daily limit reached" or "internal invoices only" the boltcard scan is recorded , but it does NOT count for the daily limit

suggestion: It's a good thing every scan is recorded, but maybe some property can store the consequent payment/withdrawal was succesful or not. This would allow for checking acurately wether the daily spent limit was reached or not.

[talvasconcelos]

Closes #55
Closes #48

[talvasconcelos]

@iWarpBTC and @bota87 , could you please test the PR again?

I rebased and fixed a couple of things. Should be ready to merge.