-
Notifications
You must be signed in to change notification settings - Fork 197
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
Validate zap receipt #412
base: master
Are you sure you want to change the base?
Validate zap receipt #412
Conversation
Thank you. I'll leave this for @alexgleason to review. |
@@ -1,4 +1,5 @@ | |||
import { bech32 } from '@scure/base' | |||
const bolt11 = require('light-bolt11-decoder') |
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.
Why is this a require instead of an ES import?
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.
Because there's no types for it.
I haven't had time to review thoroughly yet. But my first gut reaction is that adding a dependency just for zaps should be avoided. |
One of the requirements to validate a zap receipt is by decoding the bolt11 and checking if it's amount matches with the amount in the zap request. The only way to decode is by using this dependency (that is part of this nbd-wtf organization) or by just copying and pasting the code of the dependency, which seems would make things uglier. |
This implements the
Appendix F: Validating Zap Receipts
section.I could've added more tests but they would be too repetitive and I felt it wouldn't look good.
I added a new dependency but since it's a project inside the "nbd - open source bitcoin & lightning development" organization then I think it would not be a problem.
Validating zap requests is very useful for it's own sake, and also useful when someone needs to implement a zap counter, for example.
Most of the code uses
let
variables, I ended up not refactoring that because there's probably a reason behind it, in my code I ended up usingconst
and all the tests passed.