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

Validate zap receipt #412

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

patrickReiis
Copy link

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 using const and all the tests passed.

@fiatjaf
Copy link
Collaborator

fiatjaf commented Jun 9, 2024

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')
Copy link
Collaborator

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?

Copy link
Author

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.

@alexgleason
Copy link
Collaborator

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.

@patrickReiis
Copy link
Author

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.

@fiatjaf fiatjaf force-pushed the master branch 2 times, most recently from 4d4b83e to bf0c4d4 Compare November 4, 2024 18:37
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