refactor(wallet): Beignet Migration #1450

coreyphillips · 2024-01-02T15:27:16Z

Description

This PR migrates a majority of the on-chain wallet logic out of Bitkit.

Implementation details:

Instead of migrating existing user wallet data to match Beignet's data structure I've opted instead to map Bitkit's existing data structure to Beignet's using the setWalletData & getWalletData methods. The mapping can be seen here in UPDATE_WALLET_DATA. This allows all existing calls to Bitkit's store/state to remain intact and reduces the footprint of this PR.
Effectively, everything in the wallet store here is mapped accordingly with the exception of header, exchangeRates & feeEstimates that Beignet uses, but is stored elsewhere in Bitkit.

This PR is meant to act as a 1:1 migration. Updating Bitkit to use the higher level tx and fee API's from Beignet will arrive in a separate PR once this migration is complete.

Regarding the tests. I'm honestly not sure how we should proceed here. I'll need @limpbrains input on the damage I've inflicted once he returns.

Type of change

Refactoring (improving code without creating new functionality)

Migrates a majority of the on-chain wallet logic out of Bitkit.

* fix(profile): fix crash when saving profile (Android) * chore: update Podfile

pwltr · 2024-01-04T15:47:08Z

Will start posting as I go now for the initial click-through test:

When restoring the onchain wallet I have to pull-to-refresh for the balance to show up, seems to be a regression:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-01-04.at.16.28.37.mp4

pwltr · 2024-01-04T15:54:10Z

It looks like it's overpaying fees 2x:

1 input, 2 outputs, segwit = 141 bytes
"Normal" setting on regtest should be 2sats/vbyte = 282 sats fee

pwltr · 2024-01-04T16:04:43Z

Coming from the main scanner and "Paste QR Code" the "total required" in coin selection is not updated correctly, seems to be a regression:

EDIT: it works with max amount but not when I type in a custom amount
EDIT2: same when coming from "clipboard ease-of-use" feature

Also the review screen is broken thereafter, looks like SendTransaction is not updated in this case:

pwltr · 2024-01-04T16:29:26Z

Electrum Server settings falsely say "disconnected", when I switch to my local regtest the balance doesn't update (to 0) no matter what:

EDIT: also when I kill my local regtest network it doesn't give me the toasts for 'disconnected' & 'reconnected', regression (I am cross-testing everything with current master)

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-01-04.at.17.28.13.mp4

coreyphillips · 2024-01-04T16:36:52Z

This is intentional and due to a check added here. Some Electrum servers will not broadcast such low fee transactions for small transactions at 1 satPerVByte. This can sometimes be seen on our own regtest server as well for small 1 satPerVByte txs. We can tweak the min byte size lower, but need to be careful to avoid failed broadcasts.

pwltr · 2024-01-04T16:57:27Z

Some Electrum servers will not broadcast such low fee transactions for small transactions at 1 satPerVByte.

That's not the case here, this is a 2 sat/vbyte transaction. If I put a higher fee rate it is still overpaying:

Should be 141 * 5 = 705

coreyphillips · 2024-01-04T16:59:36Z

Electrum Server settings falsely say "disconnected", when I switch to my local regtest the balance doesn't update (to 0) no matter what:

EDIT: also when I kill my local regtest network it doesn't give me the toasts for 'disconnected' & 'reconnected', regression (I am cross-testing everything with current master)

I'll add an update to the Electrum config component to wipe the transaction history and balance/utxo information when specifying a new server on regtest.

yarn.lock

src/store/actions/wallet.ts

src/store/reducers/wallet.ts

pwltr · 2024-01-04T17:07:05Z

If I try to change address types it's crashing with:

# Conflicts: # src/components/Money.tsx # src/utils/lightning/index.ts

Updates beignet to 0.0.5. Adjusts several methods accordinly. Updates fee calculations on Amount screen for coin control. Updated getCurrentAddressIndex method to work with beignet.

Removed unneeded params.

Bumps beignet version to 0.0.6.

coreyphillips · 2024-01-09T12:17:54Z

@limpbrains Electrum is now handled through the beignet library. Any thoughts on how the tests should be re-wired to handle this update in Bitkit?

limpbrains · 2024-01-09T13:07:04Z

Looks like electrum instance is already available through getOnChainWalletElectrum. So I believe waiting for connection and closing it after tests would be enought.

Fixes wallet-restore tests.

limpbrains · 2024-01-11T10:22:39Z

I'm trying to run e2e tests and they are failing because of Error: Peer's feerate much too low. Actual: 253. Our expected lower limit: 7500

We need a way to set fees during e2e tests.
Or please change regtest network fees to

	onchain: {
		fast: 4,
		normal: 2,
		slow: 1,
		minimum: 1,
	},

Also, now we have 2 places where we fetch fees from mempool.space: in beignet and in bitkit getFeeEstimates function. This is confusing.

We also have fee override in dev settings, would be nice to have to working again

limpbrains · 2024-01-11T10:54:34Z

Send flow doesn't work for me

Simulator.Screen.Recording.-.iPhone.14.Pro.-.2024-01-11.at.10.53.32.mp4

coreyphillips · 2024-01-11T12:33:31Z

@limpbrains Fix incoming for the send flow.

limpbrains · 2024-01-25T13:55:12Z

All e2e tests are now working on my machine!
Also I've fixed detox not exiting issue by adding forceExit, so I think b2c789a should be reverted

coreyphillips · 2024-01-25T14:26:29Z

All e2e tests are now working on my machine! Also I've fixed detox not exiting issue by adding forceExit, so I think b2c789a should be reverted

Nice! Reverted here.

limpbrains · 2024-01-25T14:39:39Z

While using the wallet I often can see
TypeError: Cannot read property 'blockchainScripthash_getHistoryBatch' of undefined looks like a bug

limpbrains · 2024-01-25T14:41:37Z

My wallet only has 2 incoming transactions for 2 first addresses. I've sent some sats to address with index 19. Wallet can't see it. pull-to-refresh doesn't help. If I go to Address Viewer and tap check balances, it works and it finds the output

coreyphillips · 2024-01-25T15:03:47Z

My wallet only has 2 incoming transactions for 2 first addresses. I've sent some sats to address with index 19. Wallet can't see it. pull-to-refresh doesn't help. If I go to Address Viewer and tap check balances, it works and it finds the output

This is consistent with the master branch of Bitkit, but needs to be addressed/expanded on in the beignet library. Specifically, the gap limit is respected in that it will not let the user create more than 20 empty addresses, but the look-ahead is limited to one in order to reduce the number of queries made to Electrum on startup. However, we should be able to adjust this to suit our needs. For example, we may want to increase the look-ahead to 20 when restoring from a seed and bring it back down to 1 thereafter. We could also bring the look-ahead back to 20 when pulling to refresh, etc. So these params will need to be added in beignet for us to utilize in Bitkit accordingly.

limpbrains

Tests are passing, I can't find any major issues. I think we can merge it.
Or maybe @pwltr wants to take another look

src/screens/Wallets/Send/Amount.tsx

__tests__/wallet-restore.ts

…onent

…e test

pwltr · 2024-01-29T13:15:56Z

I can't switch Electrum server and the UI is behaving buggy still:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-01-29.at.14.14.36.mp4

pwltr · 2024-01-29T13:35:44Z

Fee calculation is still off and sometimes shown incorrectly in the UI, also having issues sending transactions:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-01-29.at.14.24.36.mp4

Coin selection UI is showing 2 utxos at first and then only one:

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-01-29.at.14.25.46.mp4

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-01-29.at.14.32.51.mp4

Also 'max' button font should be orange

pwltr · 2024-01-29T13:42:36Z

Coming from the main scanner and "Paste QR Code" the "total required" in coin selection is not updated correctly, seems to be a regression:

EDIT: it works with max amount but not when I type in a custom amount EDIT2: same when coming from "clipboard ease-of-use" feature

Seems to be still the case

JeanlChristophe · 2024-01-30T14:30:05Z

From a new wallet, using testnet I have received funds with a bech32 address. However, when I change to Native Segwit or legacy and try to send sats, there is $0.00 available.

JeanlChristophe · 2024-02-21T16:51:30Z

@coreyphillips Corey, I don't understand why the issues Phil raised last month still exist. Yet it was merged.

coreyphillips · 2024-02-21T17:02:53Z

@coreyphillips Corey, I don't understand why the issues Phil raised last month still exist. Yet it was merged.

What issue are you referring to? The custom Electrum server UI issue is the only one I know of as being unresolved, is considered minor and will take some time to resolve once we address the more immediate bugs/issues.

pwltr · 2024-02-21T17:16:14Z

@coreyphillips

This one is a major issue:

Coming from the main scanner and "Paste QR Code" the "total required" in coin selection is not updated correctly, seems to be a regression

This one is still there, sending tx seems fine, fees are off in calculation and UI shows something different from calc also:

Fee calculation is still off and sometimes shown incorrectly in the UI, also having issues sending transactions

and the mentioned Electrum UI bugs:

~~I can't switch Electrum server and the UI is behaving buggy still~~

Comment here is more up-to-date

refactor(wallet): Beignet Migration

0db90a2

Migrates a majority of the on-chain wallet logic out of Bitkit.

coreyphillips requested a review from pwltr January 2, 2024 15:27

coreyphillips mentioned this pull request Jan 2, 2024

refactor(wallet): Beignet Migration - DO NOT MERGE #1437

Closed

2 tasks

coreyphillips and others added 3 commits January 2, 2024 11:00

chore(wallet): Upgrade Beignet to 0.0.4

5b73ae9

fix(profile): fix crash when saving profile (Android) (#1448)

f74ff96

* fix(profile): fix crash when saving profile (Android) * chore: update Podfile

Merge remote-tracking branch 'origin/master' into beignet-migration

06f3533

coreyphillips marked this pull request as draft January 3, 2024 21:26

pwltr requested changes Jan 4, 2024

View reviewed changes

yarn.lock Outdated Show resolved Hide resolved

src/store/actions/wallet.ts Outdated Show resolved Hide resolved

src/store/reducers/wallet.ts Outdated Show resolved Hide resolved

coreyphillips added 4 commits January 8, 2024 12:29

Merge remote-tracking branch 'origin/master' into beignet-migration

ccc122a

# Conflicts: # src/components/Money.tsx # src/utils/lightning/index.ts

fix(wallet): Fee Calculation Updates

20574cd

Updates beignet to 0.0.5. Adjusts several methods accordinly. Updates fee calculations on Amount screen for coin control. Updated getCurrentAddressIndex method to work with beignet.

fix(wallet): Fix Errors

aa3189b

Removed unneeded params.

chore(wallet): Bump beignet version

de7b360

Bumps beignet version to 0.0.6.

coreyphillips added 5 commits January 9, 2024 14:56

fix(wallet): Fix Restore Tests

c68ef2a

Fixes wallet-restore tests.

fix(wallet): Fix Restore Tests

349122a

Merge remote-tracking branch 'origin/master' into beignet-migration

7817594

fix(wallet): Add startWalletServices method to wallet-restore

45aae65

fix(wallet): Remove connectToElectrum from wallet-restore.ts

d4c0f9f

fix(wallet): Fix linting errors

c7c08a6

fix(wallet): Add waitForElectrum back to onchain e2e test

a135849

limpbrains approved these changes Jan 25, 2024

View reviewed changes

src/screens/Wallets/Send/Amount.tsx Show resolved Hide resolved

limpbrains reviewed Jan 25, 2024

View reviewed changes

__tests__/wallet-restore.ts Outdated Show resolved Hide resolved

coreyphillips added 7 commits January 25, 2024 16:40

fix(wallet): Update useEffect deps for number pad text in Amount comp…

f414b07

…onent

fix(wallet): Restore initWaitForElectrumToSync logic in wallet restor…

75fa00b

…e test

fix(wallet): Remove dep from usEffect in Amount component

5b64da8

Merge remote-tracking branch 'origin/master' into beignet-migration

b6de952

chore(wallet): Bump beignet to 0.0.16

abd5226

Merge remote-tracking branch 'origin/master' into beignet-migration

4ba66c6

fix(wallet): Add condition to onOpen in SendNavigation.tsx

1ce3702

Merge remote-tracking branch 'origin/master' into beignet-migration

09e5e54

coreyphillips merged commit dcfe943 into master Jan 29, 2024
3 of 4 checks passed

coreyphillips deleted the beignet-migration branch January 29, 2024 14:20

JeanlChristophe assigned coreyphillips Feb 23, 2024

JeanlChristophe added this to the Beignet milestone Feb 23, 2024

JeanlChristophe added the bug Something isn't working label Feb 23, 2024

pwltr mentioned this pull request Jul 31, 2024

fix: keep electrum connection on background #2077

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(wallet): Beignet Migration #1450

refactor(wallet): Beignet Migration #1450

coreyphillips commented Jan 2, 2024 •

edited

Loading

pwltr commented Jan 4, 2024

pwltr commented Jan 4, 2024 •

edited

Loading

pwltr commented Jan 4, 2024 •

edited

Loading

pwltr commented Jan 4, 2024 •

edited

Loading

coreyphillips commented Jan 4, 2024 •

edited

Loading

pwltr commented Jan 4, 2024

coreyphillips commented Jan 4, 2024

pwltr commented Jan 4, 2024

coreyphillips commented Jan 9, 2024

limpbrains commented Jan 9, 2024

limpbrains commented Jan 11, 2024 •

edited

Loading

limpbrains commented Jan 11, 2024

coreyphillips commented Jan 11, 2024

limpbrains commented Jan 25, 2024

coreyphillips commented Jan 25, 2024 •

edited

Loading

limpbrains commented Jan 25, 2024

limpbrains commented Jan 25, 2024

coreyphillips commented Jan 25, 2024 •

edited

Loading

limpbrains left a comment

pwltr commented Jan 29, 2024

pwltr commented Jan 29, 2024 •

edited

Loading

pwltr commented Jan 29, 2024

JeanlChristophe commented Jan 30, 2024

JeanlChristophe commented Feb 21, 2024

coreyphillips commented Feb 21, 2024 •

edited

Loading

pwltr commented Feb 21, 2024 •

edited

Loading

refactor(wallet): Beignet Migration #1450

refactor(wallet): Beignet Migration #1450

Conversation

coreyphillips commented Jan 2, 2024 • edited Loading

Description

Type of change

pwltr commented Jan 4, 2024

pwltr commented Jan 4, 2024 • edited Loading

pwltr commented Jan 4, 2024 • edited Loading

pwltr commented Jan 4, 2024 • edited Loading

coreyphillips commented Jan 4, 2024 • edited Loading

pwltr commented Jan 4, 2024

coreyphillips commented Jan 4, 2024

pwltr commented Jan 4, 2024

coreyphillips commented Jan 9, 2024

limpbrains commented Jan 9, 2024

limpbrains commented Jan 11, 2024 • edited Loading

limpbrains commented Jan 11, 2024

coreyphillips commented Jan 11, 2024

limpbrains commented Jan 25, 2024

coreyphillips commented Jan 25, 2024 • edited Loading

limpbrains commented Jan 25, 2024

limpbrains commented Jan 25, 2024

coreyphillips commented Jan 25, 2024 • edited Loading

limpbrains left a comment

Choose a reason for hiding this comment

pwltr commented Jan 29, 2024

pwltr commented Jan 29, 2024 • edited Loading

pwltr commented Jan 29, 2024

JeanlChristophe commented Jan 30, 2024

JeanlChristophe commented Feb 21, 2024

coreyphillips commented Feb 21, 2024 • edited Loading

pwltr commented Feb 21, 2024 • edited Loading

coreyphillips commented Jan 2, 2024 •

edited

Loading

pwltr commented Jan 4, 2024 •

edited

Loading

pwltr commented Jan 4, 2024 •

edited

Loading

pwltr commented Jan 4, 2024 •

edited

Loading

coreyphillips commented Jan 4, 2024 •

edited

Loading

limpbrains commented Jan 11, 2024 •

edited

Loading

coreyphillips commented Jan 25, 2024 •

edited

Loading

coreyphillips commented Jan 25, 2024 •

edited

Loading

pwltr commented Jan 29, 2024 •

edited

Loading

coreyphillips commented Feb 21, 2024 •

edited

Loading

pwltr commented Feb 21, 2024 •

edited

Loading