Page MenuHomePhabricator

[Cashtab] Additional mitigation for legacy wallet addresses
ClosedPublic

Authored by emack on Jan 5 2024, 06:38.

Details

Summary

As noted in D15079, every now and again during testing where, for an old wallet, the address is stored with bitcoincash: prefix. This diff checks for all three derivations upon wallet load on startup in useWallet and convert to eCash prefix if needed.

Test Plan
  • npm test
  • npm start
  • Open a wallet and manually simulate this edge case where the wallet.Path1899.cashAddress value contains a bitcoincash: prefixed address. The easiest way to achieve this is to temporarily add:
const { type, hash } = cashaddr.decode(wallet.Path1899.cashAddress);
wallet.Path1899.cashAddress = cashaddr.encode('bitcoincash', type, hash);

near the top of isLegacyMigrationRequired along with debug logs right.

  • Re-load cashtab then ensure debug logs reflect the detection of a non-eCash prefixed address for Path1899 and subsequent conversion to eCash prefix.
  • Repeat for the Path245 and Path145 derivation path cash addresses.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
migrateLegacyAddresses
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 26234
Build 52040: Build Diffcashtab-tests
Build 52039: arc lint + arc unit

Event Timeline

emack requested review of this revision.Jan 5 2024, 06:38
emack edited the test plan for this revision. (Show Details)
bytesofman requested changes to this revision.Jan 5 2024, 14:43

you want to update the isLegacyMigrationRequired and the migrateLegacyWallet functions

This way,

  • you get some unit test support
  • you cover the "on cashtab load" case
  • you also cover the wallet switching case
This revision now requires changes to proceed.Jan 5 2024, 14:43

Updated isLegacyMigrationRequired to check 1899, 145 and 245 derivation path cashAddresses for non-eCash prefixes, along with unit test coverage.
I didn't do this for migrateLegacyWallet as per feedback because each path in that method already uses deriveAccount() which explictly encodes all addresses to the ecash prefix as below, so didn't see a need to double handle this once isLegacyMigrationRequired flags it for conversion:

const deriveAccount = async ({ masterHDNode, path }) => {
...
	const cashAddress = cashaddr.encode('ecash', 'P2PKH', node.identifier);
bytesofman requested changes to this revision.Jan 8 2024, 17:29
bytesofman added inline comments.
cashtab/src/hooks/useWallet.js
146 ↗(On Diff #43965)

this logic belongs in the migrateLegacyWallet function

This revision now requires changes to proceed.Jan 8 2024, 17:29
emack marked an inline comment as done.
emack edited the test plan for this revision. (Show Details)

Moved address conversion logic to migrateLegacyWallet per feedback

bytesofman added inline comments.
cashtab/src/hooks/useWallet.js
391

Sorry -- we actually don't need this logic at all.

If the migrateLegacyWallet function is called, then deriveAccount above will create an ecash: prefixed address.

It's enough to just update the conditions in isLegacyMigrationRequired

cashtab/src/utils/cashMethods.js
999

isValidXecAddress is overkill. We are calling this function every time a wallet is loaded, and we do not create wallets with invalid addresses.

Just check for wallet.PathXXXX.cashAddress.startsWith('bitcoincash:')

This revision now requires changes to proceed.Jan 10 2024, 14:07
emack marked 2 inline comments as done.

Updated checks to isLegacyMigrationRequired

legacy migration required if address doesn't start with ecash: (instead of current state: when it does start with bitcoincash:

cashtab/src/utils/cashMethods.js
999 ↗(On Diff #44044)

hate to put you through another rev, esp since i specifically suggested doing it this way

but we prob should use this instead. I don't think we ever created wallets with addresses prefixed other than bitcoincash: or ecash:, but what we are actually checking is whether or not we have an address that starts with ecash

This revision now requires changes to proceed.Jan 11 2024, 04:51
emack marked an inline comment as done.

Updated check condition for 'ecash:..' prefix

This revision is now accepted and ready to land.Jan 11 2024, 14:13