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.

Sign up for GitHub

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

Jump to bottom

Display LN Address on "Receive via Lightning Address" page #313

Open
wants to merge 12 commits into
base: savage-lnurl-username
Choose a base branch
from
Open

Display LN Address on "Receive via Lightning Address" page #313

wants to merge 12 commits into from

Conversation

erdemyerebasmaz
Copy link
Collaborator

@erdemyerebasmaz erdemyerebasmaz commented Jan 16, 2025 •
edited
Loading

Closes #319

With this PR, users LN Address is now displayed on Receive via Lightning Address page and it can be customized through Customize Address option on it's dropdown menu.

Both LNURLp & LN Address is now stored & accessible via WebhookState.

  • LNURLp is used on QR Code can be shared via Share button.
  • LN Address is shown below action buttons & can be copied via Copy button.

@erdemyerebasmaz erdemyerebasmaz changed the base branch from main to savage-lnurl-username January 16, 2025 17:30
@erdemyerebasmaz erdemyerebasmaz force-pushed the ln_address branch 2 times, most recently from 1b88833 to 4291ecd Compare January 16, 2025 17:43
@erdemyerebasmaz erdemyerebasmaz force-pushed the ln_address branch 2 times, most recently from 8321ee8 to f1c6095 Compare January 23, 2025 12:43
@erdemyerebasmaz erdemyerebasmaz marked this pull request as ready for review January 23, 2025 13:37
@danielgranhao
Copy link
Contributor

danielgranhao commented Jan 24, 2025 •
edited
Loading

@erdemyerebasmaz I haven't reviewed the code carefully, so maybe this is expected, but when testing, I was able to successfully receive to a new LN address after customizing it, but when coming back to the "Receive via Lightning Address" screen, the original default address gets shown again.

@erdemyerebasmaz
Copy link
Collaborator Author

@erdemyerebasmaz I haven't reviewed the code carefully, so maybe this is expected, but when testing, I was able to successfully receive to a new LN address after customizing it, but when coming back to the "Receive via Lightning Address" screen, the original default address gets shown again.

@danielgranhao That's unexpected. I suspect its because we're not updating the value on secure storage, will look into it.

@erdemyerebasmaz erdemyerebasmaz force-pushed the ln_address branch 3 times, most recently from bbb5cfe to d830e02 Compare January 27, 2025 04:48
@erdemyerebasmaz
Copy link
Collaborator Author

erdemyerebasmaz commented Jan 27, 2025 •
edited
Loading

@erdemyerebasmaz I haven't reviewed the code carefully, so maybe this is expected, but when testing, I was able to successfully receive to a new LN address after customizing it, but when coming back to the "Receive via Lightning Address" screen, the original default address gets shown again.

@danielgranhao That's unexpected. I suspect its because we're not updating the value on secure storage, will look into it.

@danielgranhao This should be fixed with d830e02 Testing.

This issue happened because we were re-registering(maybe refreshWebhooks isn't the correct fn name to use here) without utilizing the username each time we open "Receive via Lightning Address" page.

I'll review the WebhookCubit implementation and move the username from WebhookState to secure storage. This will also help prevent similar issues caused by missing data during app updates.

@erdemyerebasmaz
Display LN Address on "Receive via LN Address" page
beddf91 
- Check if lnAddress is not empty instead of relying on isLnAddress param
- Display LN Address below action buttons
- Set Copy actions value to LN Address
- Open options menu on clicking LN Address
- Open a bottom sheet to customize LN Address on clicking customize option

Misc:
- Rename DestinationActions's filename
@erdemyerebasmaz
Add TODO comments for LN Address
c527e33
@erdemyerebasmaz
Refactor WebhookCubit
f937e79 
Split WebhookCubit into two services, one for Webhook related logic one for LNURL service related logic.

Other changes on ChangeLnAddressUsernameBottomSheet
- fix: Use keyboard for email address
- fix: remove input formatter
- fix: trim the username value
@erdemyerebasmaz
Differentiate webhook & lnurlpay service errors
1bab410 
Add toString to WebhookState
@erdemyerebasmaz
Wait for update to finish before popping ln address sheet
6c158c0 
and show flushbars.

Remove loader.
@erdemyerebasmaz
convert username to lowercase and trim whitespace
5ae1521
@erdemyerebasmaz
Return lnurl data from updateLnAddressUsername
8714a29
@erdemyerebasmaz
fix: correct the error & title value for webhook registration errors
978c595 
misc: change ordering of params of emitted states for readability.
@erdemyerebasmaz
Store & use the LN Address username when refreshing webhooks
3e0b7a6
dangeross
dangeross reviewed Jan 27, 2025
View reviewed changes
lib/cubit/webhook/webhook_state.dart
this.lnurlPayError,
this.lnurlPayErrorTitle,
this.isLoading = false,
});

WebhookState copyWith({
String? lnurlPayUrl,
String? lnAddress,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add lnAddressUsername in the copyWith() and toString() also?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Re: #313 (comment)

Moved lnAddressUsername to secure storage to avoid persistence issues between app updates. 7119707

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dangeross dangeross dangeross left review comments

@roeierez roeierez Awaiting requested review from roeierez

@danielgranhao danielgranhao Awaiting requested review from danielgranhao

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@erdemyerebasmaz @danielgranhao @dangeross