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

REFACTOR: paid flag -> PaymentResult object #619

Closed
wants to merge 20 commits into from

Conversation

lollerfirst
Copy link
Contributor

@lollerfirst lollerfirst commented Sep 11, 2024

This PR is to deprecate the paid flag in PaymentResponse and PaymentStatus in favor of a more structured PaymentResult, that allows for more flexibility.
I have modified the backends to fill in this new field. The main difference is we DO NOT return SETTLED when paying an invoice unless we get an explicit confirmation, instead we return PENDING and then melt notices this and checks the payment status.
I have also adapted the code in get_mint_quote and melt to rely on the newly added result field.

def __str__(self):
return self.name

# We assume `None` is `FAILED`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that None should be PENDING, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe you are right. But we also used None to signal what now would be UNKNOWN

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't matter too much, though. One shouldn't be using that. I only ever created it to use it in FakeWallet

Copy link
Contributor

@prusnak prusnak Sep 11, 2024

Choose a reason for hiding this comment

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

Then I suggest to change this code to assume that None is PENDING.

That way it is consistent with rest of the old and new code.

@lollerfirst
Copy link
Contributor Author

lollerfirst commented Sep 12, 2024

I am having trouble with LNBits.

The code for get_invoice_status reads a response field paid that isn't event mentioned in the docs (https://demo.lnbits.com/docs#/Payments/Payment_List_api_v1_payments_get)

Also the docs don't mention what values the fields might take. For example state.

@lollerfirst lollerfirst marked this pull request as ready for review September 13, 2024 08:25

class PaymentResponse(BaseModel):
result: PaymentResult
ok: Optional[bool] = None # True: paid, False: failed, None: pending or unknown
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
ok: Optional[bool] = None # True: paid, False: failed, None: pending or unknown

I suggest we remove all legacy flags

Comment on lines 47 to 55
# We assume `None` is `PENDING`
@classmethod
def from_paid_flag(cls, paid: Optional[bool]):
if paid is None:
return cls.PENDING
elif not paid:
return cls.FAILED
elif paid:
return cls.SETTLED
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# We assume `None` is `PENDING`
@classmethod
def from_paid_flag(cls, paid: Optional[bool]):
if paid is None:
return cls.PENDING
elif not paid:
return cls.FAILED
elif paid:
return cls.SETTLED

We can get rid of the old paid flag altogether.


class PaymentStatus(BaseModel):
result: PaymentResult
paid: Optional[bool] = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
paid: Optional[bool] = None

@callebtc
Copy link
Collaborator

This will be used to fix #614

Copy link
Contributor Author

@lollerfirst lollerfirst left a comment

Choose a reason for hiding this comment

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

Nice! Left some suggestions...

except Exception as e:
print(e)
return PaymentStatus(paid=False)
return PaymentStatus(result=PaymentResult.FAILED)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it a given that the status is FAILED here?

@@ -135,7 +136,7 @@ async def test_blink_pay_invoice_failure():
state=MeltQuoteState.unpaid,
)
payment = await blink.pay_invoice(quote, 1000)
assert not payment.ok
assert payment.result != PaymentResult.SETTLED
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
assert payment.result != PaymentResult.SETTLED
assert payment.settled

@@ -155,7 +156,7 @@ async def test_blink_get_invoice_status():
}
respx.post(blink.endpoint).mock(return_value=Response(200, json=mock_response))
status = await blink.get_invoice_status("123")
assert status.paid
assert status.result == PaymentResult.SETTLED
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
assert status.result == PaymentResult.SETTLED
assert status.settled


t1.start()
t2.start()
t1.join()
t2.join()

assert wallet.balance <= 256 - 32
assert wallet.balance == 64
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
assert wallet.balance == 64
assert wallet.balance == 128 - 32

We have 128 in the wallet, but we only pay 32 from the wallet. The other 32 is paid from another node.

@@ -183,7 +184,7 @@ async def test_blink_get_payment_status():
}
respx.post(blink.endpoint).mock(return_value=Response(200, json=mock_response))
status = await blink.get_payment_status(payment_request)
assert status.paid
assert status.result == PaymentResult.SETTLED
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
assert status.result == PaymentResult.SETTLED
assert status.settled

@@ -102,7 +103,7 @@ async def test_blink_pay_invoice():
state=MeltQuoteState.unpaid,
)
payment = await blink.pay_invoice(quote, 1000)
assert payment.ok
assert payment.result == PaymentResult.SETTLED
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
assert payment.result == PaymentResult.SETTLED
assert payment.settled

Comment on lines +463 to +464
states = await ledger.db_read.get_proofs_states([p.Y for p in send_proofs])
assert all([s.unspent for s in states])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's awesome!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we only check this on startup?

cashu/mint/ledger.py Outdated Show resolved Hide resolved
@lollerfirst
Copy link
Contributor Author

I think this is not needed anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants