Details

Reviewers

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Commits

rABCb8860f5ce45f: [Cashtab] add publicKey property for each of the Path in the wallet

Summary

Currently, to determine if a transaction is incoming or outgoing, the wallet

retrieves the transaction data by calling BCH.RawTransactions.getTxData(txid)
looks through the returned transaction data’s “vin” to see if it contains the address of the current wallet
If it does, the transaction is outgoing,
Else, the transaction is incoming

There is a performance problem with BCH.RawTransactions.getTxData(txid).
It populates the address property of each “vin” by making a separate api call
to retrieve the data of the transaction that generates this input.
If a transaction has hundreds of inputs,
retrieving the data for that transaction will generate hundreds of api calls instead of just one.

This problem can be solved by calling BCH.RawTransaction.getRawTransaction(txid,true),
instead of BCH.RawTransaction.getTxData().

getRawTransaction() only makes one api call, however,
the returned data does not contain a wallet address in the “vin”.
We will not be able to determine if a transaction is incoming or outgoing
by comparing the vin’s address with the current wallet address.
Instead, we have the extract the public key in the scriptSig of each "vin",
and compare it with the public key of this wallet.
If they are the same, the transaction is outgoing.

Hence, we need to retrieve and store public key for each address of this wallet

see
https://bchjs.fullstack.cash/#api-RawTransactions-getRawTransaction
https://bchjs.fullstack.cash/#api-RawTransactions-getTxData

Test Plan

make sure there are multiple existing wallets on the local storage
npm start
make sure the currently active wallet is migrated to include the public key
make sure all operations perform as normal
switch to another wallet
make sure the previously active wallet is saved and includes the public key
make sure the currently active wallet is migrated and includes the public key
make sure all operations perform as normal

Diff Detail

Repository

rABC Bitcoin ABC

Branch

get-pubkey (branched from master)

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 17382
Build 34592: Build Diff	cashtab-tests
Build 34591: arc lint + arc unit

Event Timeline

hungsam requested review of this revision.Nov 26 2021, 06:31

hungsam created this revision.

Build Bitcoin ABC Diffs / Diff Testing (cashtab-tests) failed.

Failed tests logs:

====== CashTab Unit Tests:  Migrating legacy wallet on testnet ======
Error: expect(received).toStrictEqual(expected) // deep equality

- Expected  - 10
+ Received  + 13

  Object {
    "Path145": Object {
-     "cashAddress": "bitcoincash:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9v0lgx569z",
-     "fundingAddress": "simpleledger:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vryrap6mu",
-     "fundingWif": "L2xvTe6CdNxroR6pbdpGWNjAa55AZX5Wm59W5TXMuH31ihNJdDjt",
-     "legacyAddress": "1511T3ynXKgCwXhFijCUWKuTfqbPxFV1AF",
-     "slpAddress": "simpleledger:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vryrap6mu",
+     "cashAddress": "bchtest:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vtdvpkdz7",
+     "fundingAddress": "slptest:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vset6v6sr",
+     "fundingWif": "cTKuvZ644Sf7xra5z3dPshEECJNaDyBCq7HyBsysQPh1ySSpxtQ1",
+     "legacyAddress": "mjWxk74mLM7TieAsSJArLF7nXqC6oc2mof",
+     "publicKey": "02fe5308d77bcce825068a9e46adc6f032dbbe39167a7b6d05ac563ac71d8b186e",
+     "slpAddress": "slptest:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vset6v6sr",
    },
    "Path1899": Object {
      "cashAddress": "bchtest:qzagy47mvh6qxkvcn3acjnz73rkhkc6y7c9ex06hrx",
      "fundingAddress": "slptest:qzagy47mvh6qxkvcn3acjnz73rkhkc6y7c7dp5qq3m",
      "fundingWif": "cNRFB6MmkNhyhAj1TpGhXdbHgzWg4BsdHbAkKjiz4vt4vwgpC44F",
      "legacyAddress": "mxX888y8yaPpTYh3WhrB9GEkT3cgumYwPw",
+     "publicKey": "02a06bb380cf180d703f6f80796a13555aefff817d1f6f842f1e5c555b15f0fa70",
      "slpAddress": "slptest:qzagy47mvh6qxkvcn3acjnz73rkhkc6y7c7dp5qq3m",
    },
    "Path245": Object {
-     "cashAddress": "bitcoincash:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy54hkry298",
-     "fundingAddress": "simpleledger:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy5evac32me",
-     "fundingWif": "KwgNkyijAaxFr5XQdnaYyNMXVSZobgHzSoKKfWiC3Q7Xr4n7iYMG",
-     "legacyAddress": "1EgPUfBgU7ekho3EjtGze87dRADnUE8ojP",
-     "slpAddress": "simpleledger:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy5evac32me",
+     "cashAddress": "bchtest:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy539jyxazm",
+     "fundingAddress": "slptest:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy5234lu2sx",
+     "fundingWif": "cN3NDtiabeeX1Wzg2CPgLgrb7fsDG8PgWqTnmwAhYWmY6osSta7Q",
+     "legacyAddress": "muCLmiGfH961UuWrTTFNU3KxH9pVQJJx6Z",
+     "publicKey": "03c4a69fd90c8b196683216cffd2943a7b13b0db0812e44a4ff156ac7e03fc4ed7",
+     "slpAddress": "slptest:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy5234lu2sx",
    },
    "mnemonic": "apart vacuum color cream drama kind foil history hurt alone ask census",
    "name": "MigrationTestAlpha",
  }
    at Object.<anonymous> (/work/web/cashtab/src/hooks/__tests__/useWallet.test.js:61:20)
====== CashTab Unit Tests:  Migrating legacy wallet on mainnet ======
Error: expect(received).toStrictEqual(expected) // deep equality

- Expected  - 0
+ Received  + 3

@@ -2,24 +2,27 @@
    "Path145": Object {
      "cashAddress": "bitcoincash:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9v0lgx569z",
      "fundingAddress": "simpleledger:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vryrap6mu",
      "fundingWif": "L2xvTe6CdNxroR6pbdpGWNjAa55AZX5Wm59W5TXMuH31ihNJdDjt",
      "legacyAddress": "1511T3ynXKgCwXhFijCUWKuTfqbPxFV1AF",
+     "publicKey": "02fe5308d77bcce825068a9e46adc6f032dbbe39167a7b6d05ac563ac71d8b186e",
      "slpAddress": "simpleledger:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vryrap6mu",
    },
    "Path1899": Object {
      "cashAddress": "bitcoincash:qzagy47mvh6qxkvcn3acjnz73rkhkc6y7cptzgcqy6",
      "fundingAddress": "simpleledger:qzagy47mvh6qxkvcn3acjnz73rkhkc6y7cdsfndq6y",
      "fundingWif": "Kx4FiBMvKK1iXjFk5QTaAK6E4mDGPjmwDZ2HDKGUZpE4gCXMaPe9",
      "legacyAddress": "1J1Aq5tAAYxZgSDRo8soKM2Rb41z3xrYpm",
+     "publicKey": "02a06bb380cf180d703f6f80796a13555aefff817d1f6f842f1e5c555b15f0fa70",
      "slpAddress": "simpleledger:qzagy47mvh6qxkvcn3acjnz73rkhkc6y7cdsfndq6y",
    },
    "Path245": Object {
      "cashAddress": "bitcoincash:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy54hkry298",
      "fundingAddress": "simpleledger:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy5evac32me",
      "fundingWif": "KwgNkyijAaxFr5XQdnaYyNMXVSZobgHzSoKKfWiC3Q7Xr4n7iYMG",
      "legacyAddress": "1EgPUfBgU7ekho3EjtGze87dRADnUE8ojP",
+     "publicKey": "03c4a69fd90c8b196683216cffd2943a7b13b0db0812e44a4ff156ac7e03fc4ed7",
      "slpAddress": "simpleledger:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy5evac32me",
    },
    "mnemonic": "apart vacuum color cream drama kind foil history hurt alone ask census",
    "name": "MigrationTestAlpha",
  }
    at Object.<anonymous> (/work/web/cashtab/src/hooks/__tests__/useWallet.test.js:117:20)

Each failure log is accessible here:
CashTab Unit Tests: Migrating legacy wallet on testnet
CashTab Unit Tests: Migrating legacy wallet on mainnet

Harbormaster completed remote builds in B17381: Diff 31056.Nov 26 2021, 06:35

Address unit test failture

fix Migrating legacy wallet on mainnet test failure
failure in "Migrating legacy wallet on testnet" caused by previous update

as well as this update. Should I wait or go ahead and fix the test with
Path's details on test net?

Harbormaster completed remote builds in B17382: Diff 31057.Nov 26 2021, 07:40

Build Bitcoin ABC Diffs / Diff Testing (cashtab-tests) failed.

Failed tests logs:

====== CashTab Unit Tests:  Migrating legacy wallet on testnet ======
Error: expect(received).toStrictEqual(expected) // deep equality

- Expected  - 10
+ Received  + 13

  Object {
    "Path145": Object {
-     "cashAddress": "bitcoincash:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9v0lgx569z",
-     "fundingAddress": "simpleledger:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vryrap6mu",
-     "fundingWif": "L2xvTe6CdNxroR6pbdpGWNjAa55AZX5Wm59W5TXMuH31ihNJdDjt",
-     "legacyAddress": "1511T3ynXKgCwXhFijCUWKuTfqbPxFV1AF",
-     "slpAddress": "simpleledger:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vryrap6mu",
+     "cashAddress": "bchtest:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vtdvpkdz7",
+     "fundingAddress": "slptest:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vset6v6sr",
+     "fundingWif": "cTKuvZ644Sf7xra5z3dPshEECJNaDyBCq7HyBsysQPh1ySSpxtQ1",
+     "legacyAddress": "mjWxk74mLM7TieAsSJArLF7nXqC6oc2mof",
+     "publicKey": "02fe5308d77bcce825068a9e46adc6f032dbbe39167a7b6d05ac563ac71d8b186e",
+     "slpAddress": "slptest:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vset6v6sr",
    },
    "Path1899": Object {
      "cashAddress": "bchtest:qzagy47mvh6qxkvcn3acjnz73rkhkc6y7c9ex06hrx",
      "fundingAddress": "slptest:qzagy47mvh6qxkvcn3acjnz73rkhkc6y7c7dp5qq3m",
      "fundingWif": "cNRFB6MmkNhyhAj1TpGhXdbHgzWg4BsdHbAkKjiz4vt4vwgpC44F",
      "legacyAddress": "mxX888y8yaPpTYh3WhrB9GEkT3cgumYwPw",
+     "publicKey": "02a06bb380cf180d703f6f80796a13555aefff817d1f6f842f1e5c555b15f0fa70",
      "slpAddress": "slptest:qzagy47mvh6qxkvcn3acjnz73rkhkc6y7c7dp5qq3m",
    },
    "Path245": Object {
-     "cashAddress": "bitcoincash:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy54hkry298",
-     "fundingAddress": "simpleledger:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy5evac32me",
-     "fundingWif": "KwgNkyijAaxFr5XQdnaYyNMXVSZobgHzSoKKfWiC3Q7Xr4n7iYMG",
-     "legacyAddress": "1EgPUfBgU7ekho3EjtGze87dRADnUE8ojP",
-     "slpAddress": "simpleledger:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy5evac32me",
+     "cashAddress": "bchtest:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy539jyxazm",
+     "fundingAddress": "slptest:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy5234lu2sx",
+     "fundingWif": "cN3NDtiabeeX1Wzg2CPgLgrb7fsDG8PgWqTnmwAhYWmY6osSta7Q",
+     "legacyAddress": "muCLmiGfH961UuWrTTFNU3KxH9pVQJJx6Z",
+     "publicKey": "03c4a69fd90c8b196683216cffd2943a7b13b0db0812e44a4ff156ac7e03fc4ed7",
+     "slpAddress": "slptest:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy5234lu2sx",
    },
    "mnemonic": "apart vacuum color cream drama kind foil history hurt alone ask census",
    "name": "MigrationTestAlpha",
  }
    at Object.<anonymous> (/work/web/cashtab/src/hooks/__tests__/useWallet.test.js:61:20)

Each failure log is accessible here:
CashTab Unit Tests: Migrating legacy wallet on testnet

bytesofman requested changes to this revision.Nov 27 2021, 00:21

bytesofman added inline comments.

web/cashtab/src/hooks/__tests__/useWallet.test.js
11	This needs `publicKey` field added to each `Path...` object, similar to the change made on the test at line 64

This revision now requires changes to proceed.Nov 27 2021, 00:21

hungsam added inline comments.Nov 27 2021, 01:01

web/cashtab/src/hooks/__tests__/useWallet.test.js
11	I did that and it still failed. that was why i only fixed the failure with the mainnet. Notice the difference in the address prefixes in Path145 and Path245. bitcoincash vs bchtest

fix unit test failure in "Migrating legacy wallet on testnet"

Harbormaster completed remote builds in B17430: Diff 31127.Nov 28 2021, 23:22

Build Bitcoin ABC Diffs / Diff Testing (cashtab-tests) failed.

Failed tests logs:

====== CashTab Unit Tests:  Migrating legacy wallet on testnet ======
Error: expect(received).toStrictEqual(expected) // deep equality

- Expected  - 10
+ Received  + 12

  Object {
    "Path145": Object {
-     "cashAddress": "bitcoincash:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9v0lgx569z",
-     "fundingAddress": "simpleledger:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vryrap6mu",
-     "fundingWif": "L2xvTe6CdNxroR6pbdpGWNjAa55AZX5Wm59W5TXMuH31ihNJdDjt",
-     "legacyAddress": "1511T3ynXKgCwXhFijCUWKuTfqbPxFV1AF",
-     "slpAddress": "simpleledger:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vryrap6mu",
+     "cashAddress": "bchtest:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vtdvpkdz7",
+     "fundingAddress": "slptest:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vset6v6sr",
+     "fundingWif": "cTKuvZ644Sf7xra5z3dPshEECJNaDyBCq7HyBsysQPh1ySSpxtQ1",
+     "legacyAddress": "mjWxk74mLM7TieAsSJArLF7nXqC6oc2mof",
+     "publicKey": "02fe5308d77bcce825068a9e46adc6f032dbbe39167a7b6d05ac563ac71d8b186e",
+     "slpAddress": "slptest:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vset6v6sr",
    },
    "Path1899": Object {
      "cashAddress": "bchtest:qzagy47mvh6qxkvcn3acjnz73rkhkc6y7c9ex06hrx",
      "fundingAddress": "slptest:qzagy47mvh6qxkvcn3acjnz73rkhkc6y7c7dp5qq3m",
      "fundingWif": "cNRFB6MmkNhyhAj1TpGhXdbHgzWg4BsdHbAkKjiz4vt4vwgpC44F",
      "legacyAddress": "mxX888y8yaPpTYh3WhrB9GEkT3cgumYwPw",
      "publicKey": "02a06bb380cf180d703f6f80796a13555aefff817d1f6f842f1e5c555b15f0fa70",
      "slpAddress": "slptest:qzagy47mvh6qxkvcn3acjnz73rkhkc6y7c7dp5qq3m",
    },
    "Path245": Object {
-     "cashAddress": "bitcoincash:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy54hkry298",
-     "fundingAddress": "simpleledger:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy5evac32me",
-     "fundingWif": "KwgNkyijAaxFr5XQdnaYyNMXVSZobgHzSoKKfWiC3Q7Xr4n7iYMG",
-     "legacyAddress": "1EgPUfBgU7ekho3EjtGze87dRADnUE8ojP",
-     "slpAddress": "simpleledger:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy5evac32me",
+     "cashAddress": "bchtest:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy539jyxazm",
+     "fundingAddress": "slptest:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy5234lu2sx",
+     "fundingWif": "cN3NDtiabeeX1Wzg2CPgLgrb7fsDG8PgWqTnmwAhYWmY6osSta7Q",
+     "legacyAddress": "muCLmiGfH961UuWrTTFNU3KxH9pVQJJx6Z",
+     "publicKey": "03c4a69fd90c8b196683216cffd2943a7b13b0db0812e44a4ff156ac7e03fc4ed7",
+     "slpAddress": "slptest:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy5234lu2sx",
    },
    "mnemonic": "apart vacuum color cream drama kind foil history hurt alone ask census",
    "name": "MigrationTestAlpha",
  }
    at Object.<anonymous> (/work/web/cashtab/src/hooks/__tests__/useWallet.test.js:63:20)

Each failure log is accessible here:
CashTab Unit Tests: Migrating legacy wallet on testnet

fix unit test failure on testnet

Harbormaster completed remote builds in B17431: Diff 31128.Nov 29 2021, 04:29

I did that and it still failed. that was why i only fixed the failure with the mainnet.
Notice the difference in the address prefixes in Path145 and Path245. bitcoincash vs bchtest

Ah ok. Looks like there is a pre-existing issue where this unit test is not using the correct address formats.

That issue is not related to this diff -- so, it's okay to keep the non-testnet formats in that unit test even though it is "wrong." That needs to be fixed in a separate diff.

Cashtab needs to be fully reviewed and potentially refactored to actually support testnet API. We have these APIs available if you would like to take that on, but that task is distinct from this diff. When Cashtab was forked from mint.bitcoin.com, there was support for testnet...but I am not sure if it has been consistently maintained throughout the development process, as Cashtab has never used it.

This revision now requires changes to proceed.Nov 29 2021, 20:07

rebase to latest master
revert previous fix to 'Mirgrating legacy wallet on testnet' unit test

Build Bitcoin ABC Diffs / Diff Testing (cashtab-tests) failed.

Failed tests logs:

====== CashTab Unit Tests:  Migrating legacy wallet on testnet ======
Error: expect(received).toStrictEqual(expected) // deep equality

- Expected  - 10
+ Received  + 12

  Object {
    "Path145": Object {
-     "cashAddress": "bitcoincash:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9v0lgx569z",
-     "fundingAddress": "simpleledger:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vryrap6mu",
-     "fundingWif": "L2xvTe6CdNxroR6pbdpGWNjAa55AZX5Wm59W5TXMuH31ihNJdDjt",
-     "legacyAddress": "1511T3ynXKgCwXhFijCUWKuTfqbPxFV1AF",
-     "slpAddress": "simpleledger:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vryrap6mu",
+     "cashAddress": "bchtest:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vtdvpkdz7",
+     "fundingAddress": "slptest:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vset6v6sr",
+     "fundingWif": "cTKuvZ644Sf7xra5z3dPshEECJNaDyBCq7HyBsysQPh1ySSpxtQ1",
+     "legacyAddress": "mjWxk74mLM7TieAsSJArLF7nXqC6oc2mof",
+     "publicKey": "02fe5308d77bcce825068a9e46adc6f032dbbe39167a7b6d05ac563ac71d8b186e",
+     "slpAddress": "slptest:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vset6v6sr",
    },
    "Path1899": Object {
      "cashAddress": "bchtest:qzagy47mvh6qxkvcn3acjnz73rkhkc6y7c9ex06hrx",
      "fundingAddress": "slptest:qzagy47mvh6qxkvcn3acjnz73rkhkc6y7c7dp5qq3m",
      "fundingWif": "cNRFB6MmkNhyhAj1TpGhXdbHgzWg4BsdHbAkKjiz4vt4vwgpC44F",
      "legacyAddress": "mxX888y8yaPpTYh3WhrB9GEkT3cgumYwPw",
      "publicKey": "02a06bb380cf180d703f6f80796a13555aefff817d1f6f842f1e5c555b15f0fa70",
      "slpAddress": "slptest:qzagy47mvh6qxkvcn3acjnz73rkhkc6y7c7dp5qq3m",
    },
    "Path245": Object {
-     "cashAddress": "bitcoincash:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy54hkry298",
-     "fundingAddress": "simpleledger:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy5evac32me",
-     "fundingWif": "KwgNkyijAaxFr5XQdnaYyNMXVSZobgHzSoKKfWiC3Q7Xr4n7iYMG",
-     "legacyAddress": "1EgPUfBgU7ekho3EjtGze87dRADnUE8ojP",
-     "slpAddress": "simpleledger:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy5evac32me",
+     "cashAddress": "bchtest:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy539jyxazm",
+     "fundingAddress": "slptest:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy5234lu2sx",
+     "fundingWif": "cN3NDtiabeeX1Wzg2CPgLgrb7fsDG8PgWqTnmwAhYWmY6osSta7Q",
+     "legacyAddress": "muCLmiGfH961UuWrTTFNU3KxH9pVQJJx6Z",
+     "publicKey": "03c4a69fd90c8b196683216cffd2943a7b13b0db0812e44a4ff156ac7e03fc4ed7",
+     "slpAddress": "slptest:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy5234lu2sx",
    },
    "mnemonic": "apart vacuum color cream drama kind foil history hurt alone ask census",
    "name": "MigrationTestAlpha",
  }
    at Object.<anonymous> (/work/web/cashtab/src/hooks/__tests__/useWallet.test.js:63:20)

Each failure log is accessible here:
CashTab Unit Tests: Migrating legacy wallet on testnet

Harbormaster completed remote builds in B17454: Diff 31161.Nov 30 2021, 00:06

bytesofman requested changes to this revision.Nov 30 2021, 00:11

bytesofman added inline comments.

web/cashtab/src/hooks/useWallet.js
127 ↗	(On Diff #31161)	Why this change from `const` to `let`? Does not appear to be related to this diff
332 ↗	(On Diff #31161)	Because this check is used twice, please make a separate function for it, `isLegacyMigrationRequired()`, that simply returns this boolean Add it to `utils/Validation.js` and include some unit tests.
561 ↗	(On Diff #31161)	See comment on line 332 -- also call the new validation function here
606 ↗	(On Diff #31161)	Path1899, not Path1800
608 ↗	(On Diff #31161)	Also call new validation function here instead of this list of `if` conditions

This revision now requires changes to proceed.Nov 30 2021, 00:11

update per feedback

Harbormaster completed remote builds in B17455: Diff 31162.Nov 30 2021, 05:31

Build Bitcoin ABC Diffs / Diff Testing (cashtab-tests) failed.

Failed tests logs:

====== CashTab Unit Tests:  Migrating legacy wallet on testnet ======
Error: expect(received).toStrictEqual(expected) // deep equality

- Expected  - 10
+ Received  + 12

  Object {
    "Path145": Object {
-     "cashAddress": "bitcoincash:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9v0lgx569z",
-     "fundingAddress": "simpleledger:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vryrap6mu",
-     "fundingWif": "L2xvTe6CdNxroR6pbdpGWNjAa55AZX5Wm59W5TXMuH31ihNJdDjt",
-     "legacyAddress": "1511T3ynXKgCwXhFijCUWKuTfqbPxFV1AF",
-     "slpAddress": "simpleledger:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vryrap6mu",
+     "cashAddress": "bchtest:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vtdvpkdz7",
+     "fundingAddress": "slptest:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vset6v6sr",
+     "fundingWif": "cTKuvZ644Sf7xra5z3dPshEECJNaDyBCq7HyBsysQPh1ySSpxtQ1",
+     "legacyAddress": "mjWxk74mLM7TieAsSJArLF7nXqC6oc2mof",
+     "publicKey": "02fe5308d77bcce825068a9e46adc6f032dbbe39167a7b6d05ac563ac71d8b186e",
+     "slpAddress": "slptest:qq47pcxfn8n7w7jy86njd7pvgsv39l9f9vset6v6sr",
    },
    "Path1899": Object {
      "cashAddress": "bchtest:qzagy47mvh6qxkvcn3acjnz73rkhkc6y7c9ex06hrx",
      "fundingAddress": "slptest:qzagy47mvh6qxkvcn3acjnz73rkhkc6y7c7dp5qq3m",
      "fundingWif": "cNRFB6MmkNhyhAj1TpGhXdbHgzWg4BsdHbAkKjiz4vt4vwgpC44F",
      "legacyAddress": "mxX888y8yaPpTYh3WhrB9GEkT3cgumYwPw",
      "publicKey": "02a06bb380cf180d703f6f80796a13555aefff817d1f6f842f1e5c555b15f0fa70",
      "slpAddress": "slptest:qzagy47mvh6qxkvcn3acjnz73rkhkc6y7c7dp5qq3m",
    },
    "Path245": Object {
-     "cashAddress": "bitcoincash:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy54hkry298",
-     "fundingAddress": "simpleledger:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy5evac32me",
-     "fundingWif": "KwgNkyijAaxFr5XQdnaYyNMXVSZobgHzSoKKfWiC3Q7Xr4n7iYMG",
-     "legacyAddress": "1EgPUfBgU7ekho3EjtGze87dRADnUE8ojP",
-     "slpAddress": "simpleledger:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy5evac32me",
+     "cashAddress": "bchtest:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy539jyxazm",
+     "fundingAddress": "slptest:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy5234lu2sx",
+     "fundingWif": "cN3NDtiabeeX1Wzg2CPgLgrb7fsDG8PgWqTnmwAhYWmY6osSta7Q",
+     "legacyAddress": "muCLmiGfH961UuWrTTFNU3KxH9pVQJJx6Z",
+     "publicKey": "03c4a69fd90c8b196683216cffd2943a7b13b0db0812e44a4ff156ac7e03fc4ed7",
+     "slpAddress": "slptest:qztqe8k4v8ckn8cvfxt5659nhd7dcyvxy5234lu2sx",
    },
    "mnemonic": "apart vacuum color cream drama kind foil history hurt alone ask census",
    "name": "MigrationTestAlpha",
  }
    at Object.<anonymous> (/work/web/cashtab/src/hooks/__tests__/useWallet.test.js:63:20)

Each failure log is accessible here:
CashTab Unit Tests: Migrating legacy wallet on testnet

Need to find out why this unit test is failing.

This revision now requires changes to proceed.Nov 30 2021, 19:49

hungsam added inline comments.Nov 30 2021, 22:01

web/cashtab/src/hooks/useWallet.js
393 ↗	(On Diff #31162)	This causes the unit test failure. Previously, it only generated new account on Path1899. With this update, it re-generates wallets/accounts on Path245 and Path145 as well, so that they now have public keys. This code correctly creates wallets/accounts on testnet, with addresses in testnet format. However, the unit test specified addresses in mainnet format. Simple fix would be to update the unit test with the correct testnet addresses.

Why are the private keys also changing?

The failure in "Migrating legacy wallet on testnet" was caused by different returned data on Path145 and Path245. Path1899 has the correct data as expected (except additional publicKey).

Comparing two unit tests, "Migrating legacy wallet on testnet" vs "Migrating legacy wallet on mainnet".
Looking at the Path1899, we can see that the two test cases expected 2 different private keys.
From this, I am assuming that private key generated for a path on the testnet is expected to be different from the private key for the same path on the mainnet.

From this reasoning, the private keys for Path145 and Path245 on the testnet did not change. The failed unit test specified the wrong expected data.

This can be further confirmed by looking at the mockLegacyWallets.js file, the same wallet mockLegacyWallets.legacyAlpha (which is a mainnet wallet) is used for for both tests on mainnet and testnet.

The "Migrating legacy wallet on testnet" unit test should be using a testnet wallet instead of a mainet wallet. This could be another diff.

This unit test did not fail in all previous diffs because we did not re-create accounts/wallets on the testnet for Path145 and Path245, only Path1899 was created.

Ok -- please correct the mock data for this unit test. Thanks.

rebase to the latest master - that fixes the unit test failure

Harbormaster completed remote builds in B17518: Diff 31240.Dec 2 2021, 20:56

bytesofman accepted this revision.Dec 2 2021, 21:59

This revision is now accepted and ready to land.Dec 2 2021, 21:59

Closed by commit rABCb8860f5ce45f: [Cashtab] add publicKey property for each of the Path in the wallet (authored by hungsam). · Explain WhyDec 2 2021, 22:07

This revision was automatically updated to reflect the committed changes.

hungsam added a commit: rABCb8860f5ce45f: [Cashtab] add publicKey property for each of the Path in the wallet.

hungsam mentioned this in D10622: T1920 - fix excessive API Calls on Tx History Fetching.Dec 2 2021, 23:56

hungsam mentioned this in rABC205de92b29e9: T1920 - fix excessive API Calls on Tx History Fetching.Dec 13 2021, 22:35

bytesofman mentioned this in D11476: [Cashtab] [chronik] Add hash160 to wallet state.May 16 2022, 22:34