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

Add configuration property for allowed length of LNURL-pay comments #40

Merged
merged 1 commit into from
Aug 11, 2022
Merged

Add configuration property for allowed length of LNURL-pay comments #40

merged 1 commit into from
Aug 11, 2022

Conversation

yanascz
Copy link
Contributor

@yanascz yanascz commented Jul 22, 2022

No description provided.

@bumi
Copy link
Owner

bumi commented Aug 1, 2022

thanks for the PR! that looks good!

do you know why not more lnurlp implementations add support for comments like that?

@yanascz
Copy link
Contributor Author

yanascz commented Aug 2, 2022

do you know why not more lnurlp implementations add support for comments like that?

Maybe because it’s defined under different RFC? I can only guess. I saw support for comments by ln.tips so I started digging and discovered #12.

@ok300
Copy link
Contributor

ok300 commented Aug 8, 2022

Looking forward to this.

@ok300
Copy link
Contributor

ok300 commented Aug 8, 2022

Why wait when I can run your branch directly :)

Started doing that, works great.

Just one question, why is lnurlp-comment-allowed default 0? I doubt people don't want comments by default, I'd say a reasonable number (200-500) is a good sane default. Enough for comment, but small enough to not run into any limits.

@yanascz
Copy link
Contributor Author

yanascz commented Aug 8, 2022

Just one question, why is lnurlp-comment-allowed default 0? I doubt people don't want comments by default, I'd say a reasonable number (200-500) is a good sane default. Enough for comment, but small enough to not run into any limits.

I wanted the change to be backwards compatible. But I'm OK with a different default, e.g. 210 (as 21 is such a nice number 😉). Ultimately, it's up to @bumi to decide what default he wants.

@bumi
Copy link
Owner

bumi commented Aug 11, 2022

ok, cool. then let's enable it by default. @yanascz can you update that, then I'll merge it and make a new release
@ok300 thanks for testing and giving feedback.

@yanascz
Copy link
Contributor Author

yanascz commented Aug 11, 2022

ok, cool. then let's enable it by default. @yanascz can you update that, then I'll merge it and make a new release

Done! ✅

@bumi bumi merged commit a383a74 into bumi:master Aug 11, 2022
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