Page MenuHomePhabricator

Refactor from path 145/245 to path 1899 with migration support
ClosedPublic

Authored by josephroyking on Dec 31 2020, 17:21.

Details

Summary

Implement the new path 1899 for SLPA-aware BCHA wallets

Test Plan

Do not test this diff with high value wallets

  1. Run cashtab locally on the existing master.
git clone git@github.com:Bitcoin-ABC/bitcoin-abc.git
cd bitcoin-abc/web/cashtab
npm i --legacy-peer-deps
npm start
  1. If you have real money wallets at the localhost:3000/ instance of Cashtab,

back them up and delete them.

  1. Create 2-5 new wallets
  2. Send some BCHA and SLPA to these wallets
  3. Stop this instance of Cashtab
  4. Now, run Cashtab from this diff
arc patch D----
cd web/cashtab/
npm start
  1. Observe that BCHA and SLPA balances from legacy wallets exist and are displayed
  2. Observe that BCHA/SLPA receiving addresses are now different
  3. Send some test BCHA and SLPA transactions
  4. Observe that change goes to the new Path1899 addresses

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

deadalnix requested changes to this revision.Jan 2 2021, 20:52
deadalnix added a subscriber: deadalnix.

It doesn't looks like there is a test for the migration, why?

web/cashtab/src/hooks/useBCH.js
310 ↗(On Diff #26585)

commented out logs pretty poor style. If these logs are generally useful, then they should stay, if they aren't then the comment should go. If they are useful in specific situations, then they need to be gated properly. This is just bad.

323 ↗(On Diff #26585)

dito

355 ↗(On Diff #26585)

dito

web/cashtab/src/hooks/useWallet.js
288 ↗(On Diff #26585)

braces

302 ↗(On Diff #26585)

It doesn't seems to me like ignoring the error, beside logging, is the right thing to do here.

This revision now requires changes to proceed.Jan 2 2021, 20:52

Responding to comments, additional testing (manual, real money)

Unit tests updated mocks, changes to Wallet.js

Added unit tests for the migration function.

Add unit tests for testnet cases, covering the threshold

Add 'BCH' param to migrateLegacyWallet call + remove test function from useWallet.js

This revision is now accepted and ready to land.Jan 6 2021, 16:36