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

feat: add keysend messenger to custom permissions #684

Merged
merged 3 commits into from Nov 21, 2023

Conversation

secondl1ght
Copy link
Contributor

@secondl1ght secondl1ght commented Nov 19, 2023

Added a new option to the Custom Permissions view when creating a new LNC session. It is called 'Keysend Messenger' which allows to send/receive payments, plus sign messages. This preset can be used in apps such as Cipherchat for example. Users will now also be able to add Sign Message to their Custom permissions as well.

image

Copy link
Member

@jamaljsr jamaljsr left a comment

Choose a reason for hiding this comment

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

tACK LGTM 🚀

Thanks for submitting the PR. I've tested this and confirmed it works as expected. The code looks great as well.

I'd just like to confirm with @levmi that the copy is ok before merging.

@jamaljsr jamaljsr requested a review from levmi November 20, 2023 16:24
Copy link

@levmi levmi left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, love to see it! Happy to get this in :)

Just one nit on copy, ideally, we could adjust to "User can send and receive payments plus sign messages". I'm okay with title remaining "Keysend Messenger".

@dstrukt dstrukt self-requested a review November 20, 2023 21:02
Copy link
Contributor

@dstrukt dstrukt left a comment

Choose a reason for hiding this comment

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

Had to check this out - very cool - thanks for the PR @secondl1ght!

And ACK to @levmi's copy suggestion to keep the description consistent.

@secondl1ght
Copy link
Contributor Author

secondl1ght commented Nov 21, 2023

Woot! Thanks for the review guys - I adjusted the copy, I agree it reads better this way.

One other thing I was thinking about is that instead of having the expected values for permissionType in a comment, we could change it to use an actual type for these values (see here for the line I am speaking of: https://github.com/lightninglabs/lightning-terminal/pull/684/files#diff-281dcaa6b4e26633152a0b6ce3832cc66285a0ec1d9f1601c3b5bfa56e7855a4R10).

Happy to push one more commit to this PR to add typing to this variable if you guys think it is a good idea.

@jamaljsr
Copy link
Member

One other thing I was thinking about is that instead of having the expected values for permissionType in a comment, we could change it to use an actual type for these values (see here for the line I am speaking of: https://github.com/lightninglabs/lightning-terminal/pull/684/files#diff-281dcaa6b4e26633152a0b6ce3832cc66285a0ec1d9f1601c3b5bfa56e7855a4R10).

Happy to push one more commit to this PR to add typing to this variable if you guys think it is a good idea.

Sounds good. Feel free to make that refactoring here in this PR.

@secondl1ght
Copy link
Contributor Author

Ok cool, I just pushed a commit adding a type. Please let me know what you think! 🙌🏼

@levmi
Copy link

levmi commented Nov 21, 2023

Awesome! Really love to see the external contribution here. If you ever want to add anything else to litd or the LNC side, don't hesitate to put up a PR or reach out. Oftentimes, we'll really want to implement something, but not necessarily have the resourcing to prioritize it. This is immensely helpful so thanks!

Copy link
Member

@jamaljsr jamaljsr left a comment

Choose a reason for hiding this comment

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

Tested the latest commits. Everything looks great!

Thanks for the refactoring as well. LGTM 🚀

@jamaljsr jamaljsr merged commit e2e5779 into lightninglabs:master Nov 21, 2023
12 checks passed
@secondl1ght
Copy link
Contributor Author

Awesome thanks everyone! I will definitely continue to contribute to the LL repos when I have some time. 🙂

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.

None yet

4 participants