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

New screen: On-chain addresses #2242

Merged
merged 14 commits into from
Sep 27, 2024
Merged

Conversation

myxmaster
Copy link
Contributor

@myxmaster myxmaster commented Jun 15, 2024

Description

This adds "On-chain addresses" to the menu:

grafik

This displays all addresses from LND's ListAddresses grouped by account and address type. Allows hiding 0-balance addresses and sorting by creation time and balance.

grafik

This pull request is categorized as a:

  • New feature
  • Bug fix
  • Code refactor
  • Configuration change
  • Locales update
  • Quality assurance
  • Other

Checklist

  • I’ve run yarn run tsc and made sure my code compiles correctly
  • I’ve run yarn run lint and made sure my code didn’t contain any problematic patterns
  • I’ve run yarn run prettier and made sure my code is formatted correctly
  • I’ve run yarn run test and made sure all of the tests pass

Testing

If you modified or added a utility file, did you add new unit tests?

  • No, I’m a fool
  • Yes
  • N/A

I have tested this PR on the following platforms (please specify OS version and phone model/VM):

  • Android
  • iOS

I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):

  • Embedded LND
  • LND (REST)
  • LND (Lightning Node Connect)
  • Core Lightning (c-lightning-REST)
  • LndHub
  • [DEPRECATED] Core Lightning (Spark)
  • [DEPRECATED] Eclair

Locales

  • I’ve added new locale text that requires translations
  • I’m aware that new translations should be made on the ZEUS Transfix page and not directly to this repo

Third Party Dependencies and Packages

  • Contributors will need to run yarn after this PR is merged in
  • 3rd party dependencies have been modified:
    • verify that package.json and yarn.lock have been properly updated
    • verify that dependencies are installed for both iOS and Android platforms

Other:

  • Changes were made that require an update to the README
  • Changes were made that require an update to onboarding

@kaloudis kaloudis added this to the v0.9.1 milestone Jul 18, 2024
@myxmaster myxmaster force-pushed the on-chain-addresses branch 2 times, most recently from e712673 to bd7e3ca Compare August 14, 2024 18:21
@myxmaster
Copy link
Contributor Author

Now additionally grouping change addresses:

grafik

@kaloudis
Copy link
Contributor

We gotta change the icon. Currently it's credit cards?

@kaloudis
Copy link
Contributor

Simulator Screenshot - iPhone 15 Pro Max - 2024-09-18 at 11 07 30

Dropdown values not displaying correctly on iOS

@kaloudis
Copy link
Contributor

We need to configure this for Lightning Node Connect and Embedded LND as well

@kaloudis
Copy link
Contributor

This view should also probably just replace the current Coins view

@kaloudis
Copy link
Contributor

Lastly, we should think about adding the ability for users to pick the account they want to generate addresses for

Copy link
Contributor

@kaloudis kaloudis left a comment

Choose a reason for hiding this comment

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

see comments above

@kaloudis
Copy link
Contributor

I think the sections should be collapsible (perhaps by default) - that way users will be able to navigate more easily if they have a ton of addresses


private loadAddresses = async () => {
this.setState({ loading: true });
const listAddressesResponse = await BackendUtils.listAddresses();
Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably move this call to the UTXOsStore

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I think this is problematic. Fetching all addresses at once can lead to timeouts on nodes that have a bunch generated. Instead, I think it would be better to fetch the accounts list and then fetch the addresses for each account, one at a time.

We'd have to refactor quite a bit, but I don't think things will hold up as is.

@kaloudis
Copy link
Contributor

This view should also probably just replace the current Coins view

after giving some thought to this, I think if supportsAddressesWithDerivationPaths - then we should show this new view, otherwise show the old view.

In keeping in sync with the Coins button under the On-chain balance slider, we should label the button on the Menu view Coins as well, and use the same icon.

@kaloudis
Copy link
Contributor

Tons of changes wired up:

  • All three LND interfaces wired up
  • Error handling for timed out ListAddresses call
  • Ability to collapse address groups
  • Ability to generate addresses for each address group
  • Address count added to each address group header
  • Menu icon reverted to Coins icon

simulator_screenshot_03769D24-6D89-499E-B6C1-003361D6CCC4

Copy link
Contributor

@shubhamkmr04 shubhamkmr04 left a comment

Choose a reason for hiding this comment

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

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-09-26.at.17.03.06.mp4

I think hide 0-balance should not remove the complete view in case we only have o-balance addresses

Copy link
Contributor

@shubhamkmr04 shubhamkmr04 left a comment

Choose a reason for hiding this comment

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

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-09-26.at.17.10.42.mp4

the view glitch when we tap to add

@kaloudis
Copy link
Contributor

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-09-26.at.17.10.42.mp4

the view glitch when we tap to add

I think we need a separate issue for this. Mind filing one?

Copy link
Contributor

@shubhamkmr04 shubhamkmr04 left a comment

Choose a reason for hiding this comment

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

Simulator Screenshot - iPhone 15 Pro Max - 2024-09-27 at 19 43 53
should we have one option here to generate in case we have no addresses?

@shubhamkmr04
Copy link
Contributor

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-09-26.at.17.10.42.mp4
the view glitch when we tap to add

I think we need a separate issue for this. Mind filing one?

we can track the issue here #2441

@kaloudis
Copy link
Contributor

Simulator Screenshot - iPhone 15 Pro Max - 2024-09-27 at 19 43 53 should we have one option here to generate in case we have no addresses?

simulator_screenshot_11D196C3-D77B-4195-8F2C-1302CDF87A4C

Create button now added to the corner when no addresses have been generated yet

Copy link
Contributor

@shubhamkmr04 shubhamkmr04 left a comment

Choose a reason for hiding this comment

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

tACK

@kaloudis kaloudis merged commit 10f3f4d into ZeusLN:master Sep 27, 2024
3 checks passed
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