-
Notifications
You must be signed in to change notification settings - Fork 13
Implement GetPaymentDetails and ListPayments Api. #15
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
Conversation
let payment_details = node.payment(&PaymentId(payment_id_bytes)); | ||
|
||
let response = GetPaymentDetailsResponse { | ||
payment: payment_details.map(|payment| payment_to_proto(payment)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we respond with an error if the payment is not found?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Payment being queried and not being present is a valid use-case, it shouldn't be an exception.
Moreover, it doesn't fit into invalidRequest, InternalError or Lightning Error.
I think returning empty response is correct here, ldk-node does the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... I thought "not found" was a standard error sort of REST error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, If we are trying to edit channel which doesn't exist, we should throw error.
or close channel that doesn't exist. "not found" in this case represents resource which we were trying to change but wasn't found.
But trying to fetch a non-existent payment or payment that hasn't been made isn't an error, at application layer.
It mostly is a valid scenario, which can be represented by returning empty response or None
in optional.
I think it should be documented in proto though.
Now rebased on top of #18 and addressed feedback. |
Need to rebase after #18 landed. |
Addressed comments, rebased on main, squashed fixups as part of rebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please squash.
Squashed fixups |
Adds implementation for:
Based on #13