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.
Details
Details
- Reviewers
bytesofman - Group Reviewers
Restricted Project - Commits
- rABCf57103ac9621: [Cashtab] Additional mitigation for legacy wallet addresses
- 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
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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
Comment Actions
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);| cashtab/src/hooks/useWallet.js | ||
|---|---|---|
| 146 ↗ | (On Diff #43965) | this logic belongs in the migrateLegacyWallet function |
| cashtab/src/hooks/useWallet.js | ||
|---|---|---|
| 391 ↗ | (On Diff #44010) | 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 ↗ | (On Diff #44010) | 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:') |
Comment Actions
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 |