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(lightning): using on chain address for channel close #1459

Merged
merged 3 commits into from
Jan 8, 2024

Conversation

Jasonvdb
Copy link
Collaborator

@Jasonvdb Jasonvdb commented Jan 5, 2024

Description

  • Updates react-native-ldk to include a change where funds from a channel close are sent direct to Bitkit's on chain wallet and no longer require and extra sweeping step.
  • Also includes a fix preventing some users from completing the restore flow

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactoring (improving code without creating new functionality)

Tests

  • Detox test
  • Unit test
  • No test

QA Notes

  • Funds should show up directly on chain after channel close
  • Some users stuck on restoring screen should not be able to progress

pwltr
pwltr previously approved these changes Jan 5, 2024
Copy link
Collaborator

@pwltr pwltr left a comment

Choose a reason for hiding this comment

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

Can you fix the failing onboarding e2e test?

@pwltr pwltr dismissed their stale review January 5, 2024 10:30

needs UI work

@pwltr
Copy link
Collaborator

pwltr commented Jan 5, 2024

Actually although this works I think we'd want more work on the UI to avoid confusing the user. Right now it shows a pending card (60m) but at the same time also shows an activity item (10-20m) not marked as transfer.

image

Copy link
Collaborator

@pwltr pwltr left a comment

Choose a reason for hiding this comment

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

Will approve because UI was not in the scope

@coreyphillips
Copy link
Collaborator

coreyphillips commented Jan 5, 2024

Instead of using generateNewReceiveAddress we'll want to grab what the current addressIndex is instead. Since generateNewReceiveAddress will always create a new address regardless of whether or not the previous index was used. This means opening and closing the app a few times will end up pushing the user's addressIndex into the gap limit. I'll make a change suggestion for this shortly.

@Jasonvdb
Copy link
Collaborator Author

Jasonvdb commented Jan 5, 2024

Can you fix the failing onboarding e2e test?

I was seeing webRelayClient not ready yet in all the PR's? Unless you mean another test?

@Jasonvdb
Copy link
Collaborator Author

Jasonvdb commented Jan 5, 2024

Instead of using generateNewReceiveAddress we'll want to grab what the current addressIndex is instead. Since generateNewReceiveAddress will always create a new address regardless of whether or not the previous index was used. This means opening and closing the app a few times will end up pushing the user's addressIndex into the gap limit. I'll make a change suggestion for this shortly.

Oh yes right. So originally I had it use getReceiveAddress and I changed it to return IAddress instead of a string but I thought maybe I was breaking something with the store and didn't want to go down that path right before testing session.

@pwltr
Copy link
Collaborator

pwltr commented Jan 5, 2024

Can you fix the failing onboarding e2e test?

I was seeing webRelayClient not ready yet in all the PR's? Unless you mean another test?

Was talking about this:

FAIL e2e/onboarding.e2e.js (109.532 s)
  Onboarding
    ✕ Can pass onboarding correctly (101505 ms)

  ● Onboarding › Can pass onboarding correctly

    expect(received).toBe(expected) // Object.is equality

    Expected: "bitcoin:bcrt1q2zkt93dtl8cxqgtvzxurlu0mu6z6ahnh6sy5uf"
    Received: "bitcoin:bcrt1qjz82ap0k2x6r7zg9jketlthx8whm0dne8f0cvx"

      120 | 		const { label: address2 } = await element(by.id('QRCode')).getAttributes();
      121 |
    > 122 | 		jestExpect(address1).toBe(address2);
          | 		                     ^
      123 |
      124 | 		markComplete('onboarding-1');
      125 | 	});

Will be fixed by what @coreyphillips described above 👍

Update getAddress method.
Add getCurrentAddressIndex method.
@coreyphillips coreyphillips mentioned this pull request Jan 5, 2024
2 tasks
@coreyphillips
Copy link
Collaborator

coreyphillips commented Jan 5, 2024

Oh yes right. So originally I had it use getReceiveAddress and I changed it to return IAddress instead of a string but I thought maybe I was breaking something with the store and didn't want to go down that path right before testing session.

PR created here. Since most of this will be changed once the beignet migration is merged anyway I simply created a new method that is effectively the same as getReceiveAddress except it returns the entire object. Will make it easier to merge the changes in later on.

@Jasonvdb Jasonvdb merged commit 2362bec into master Jan 8, 2024
2 of 4 checks passed
@Jasonvdb Jasonvdb deleted the ldk-update2 branch January 8, 2024 10:22
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