[Refactor] Clean up wallet related code (#200)

* Decouple masterkey from wallet * Rewrite network * Refactor wipePrivateData * Various fix * Implement review suggestion * Fix testnet switch issue * Fix await * kitty fix
PIVX-Labs · Sep 21, 2023 · 04b7d40 · 04b7d40
1 parent cd341de
commit 04b7d40
Show file tree

Hide file tree

Showing 8 changed files with 223 additions and 215 deletions.
diff --git a/index.template.html b/index.template.html
@@ -475,7 +475,7 @@ <h3 class="noselect balance-title">
                                         <a id="guiExportWalletItem" class="dropdown-item ptr" data-toggle="modal" data-target="#exportPrivateKeysModal" data-backdrop="static" data-keyboard="false" onclick="MPW.toggleExportUI()">
                                           <i class="fas fa-key"></i> <span data-i18n="export">Export</span>
                                         </a>
-                                        <a class="dropdown-item ptr" id="guiNewAddress" data-toggle="modal" data-target="#qrModal" onclick="MPW.wallet.getNewAddress({updateGUI: true, verify: true});">
+                                        <a class="dropdown-item ptr" id="guiNewAddress" data-toggle="modal" data-target="#qrModal" onclick="MPW.getNewAddress({updateGUI: true, verify: true});">
                                           <i class="fas fa-sync-alt"></i> <span data-i18n="refreshAddress">Refresh address</span>
                                         </a>
                                         <a class="dropdown-item ptr" data-toggle="modal" data-target="#redeemCodeModal">

diff --git a/scripts/contacts-book.js b/scripts/contacts-book.js
@@ -343,17 +343,7 @@ export async function guiRenderReceiveModal(
             let strPubkey = '';
 
             // If HD: use xpub, otherwise we'll fallback to our single address
-            if (wallet.isHD()) {
-                // Get our current wallet XPub
-                const derivationPath = wallet
-                    .getDerivationPath()
-                    .split('/')
-                    .slice(0, 4)
-                    .join('/');
-                strPubkey = await wallet.getMasterKey().getxpub(derivationPath);
-            } else {
-                strPubkey = await wallet.getMasterKey().getCurrentAddress();
-            }
+            strPubkey = await wallet.getKeyToExport();
 
             // Construct the Contact Share URI
             const strContactURI = await localContactToURI(cAccount, strPubkey);
@@ -374,7 +364,7 @@ export async function guiRenderReceiveModal(
             document.getElementById('clipboard').value = strPubkey;
         } else {
             // Get our current wallet address
-            const strAddress = await wallet.getMasterKey().getCurrentAddress();
+            const strAddress = await wallet.getCurrentAddress();
 
             // Update the QR Label (we'll show the address here for now, user can set Contact "Name" optionally later)
             doms.domModalQrLabel.innerHTML =
@@ -404,7 +394,7 @@ export async function guiRenderReceiveModal(
         }
     } else if (cReceiveType === RECEIVE_TYPES.ADDRESS) {
         // Get our current wallet address
-        const strAddress = await wallet.getMasterKey().getCurrentAddress();
+        const strAddress = await wallet.getCurrentAddress();
         createQR('pivx:' + strAddress, doms.domModalQR);
         doms.domModalQrLabel.innerHTML =
             strAddress +
@@ -415,12 +405,7 @@ export async function guiRenderReceiveModal(
         document.getElementById('clipboard').value = strAddress;
     } else {
         // Get our current wallet XPub
-        const derivationPath = wallet
-            .getDerivationPath()
-            .split('/')
-            .slice(0, 4)
-            .join('/');
-        const strXPub = await wallet.getMasterKey().getxpub(derivationPath);
+        const strXPub = await wallet.getXPub();
 
         // Update the QR Label (we'll show the address here for now, user can set Contact "Name" optionally later)
         doms.domModalQrLabel.innerHTML =
@@ -522,15 +507,8 @@ export async function guiAddContact() {
     // Ensure we're not adding our own XPub
     if (isXPub(strAddr)) {
         if (wallet.isHD()) {
-            const derivationPath = wallet
-                .getDerivationPath()
-                .split('/')
-                .slice(0, 4)
-                .join('/');
             // Compare the XPub against our own
-            const fOurs =
-                strAddr ===
-                (await wallet.getMasterKey().getxpub(derivationPath));
+            const fOurs = strAddr === (await wallet.getXPub());
             if (fOurs) {
                 createAlert(
                     'warning',
@@ -621,15 +599,8 @@ export async function guiAddContactPrompt(
     // Ensure we're not adding our own XPub
     if (isXPub(strPubkey)) {
         if (wallet.isHD()) {
-            const derivationPath = wallet
-                .getDerivationPath()
-                .split('/')
-                .slice(0, 4)
-                .join('/');
             // Compare the XPub against our own
-            const fOurs =
-                strPubkey ===
-                (await wallet.getMasterKey().getxpub(derivationPath));
+            const fOurs = strPubkey === (await wallet.getXPub());
             if (fOurs) {
                 createAlert(
                     'warning',
@@ -982,19 +953,7 @@ export async function localContactToURI(account, pubkey) {
     let strPubkey = pubkey || '';
 
     // If HD: use xpub, otherwise we'll fallback to our single address
-    if (!strPubkey) {
-        if (wallet.isHD()) {
-            // Get our current wallet XPub
-            const derivationPath = wallet
-                .getDerivationPath()
-                .split('/')
-                .slice(0, 4)
-                .join('/');
-            strPubkey = await wallet.getMasterKey().getxpub(derivationPath);
-        } else {
-            strPubkey = await wallet.getMasterKey().getCurrentAddress();
-        }
-    }
+    if (!strPubkey) strPubkey = await wallet.getKeyToExport();
 
     // Construct the Contact URI Root
     const strURL = window.location.origin + window.location.pathname;

diff --git a/scripts/global.js b/scripts/global.js
@@ -7,6 +7,7 @@ import {
     hasEncryptedWallet,
     importWallet,
     decryptWallet,
+    getNewAddress,
 } from './wallet.js';
 import { LegacyMasterKey } from './masterkey.js';
 import { getNetwork, HistoricalTxType } from './network.js';
@@ -800,16 +801,10 @@ export async function openSendQRScanner() {
  */
 export async function openExplorer(strAddress = '') {
     if (wallet.isLoaded() && wallet.isHD() && !strAddress) {
-        const derivationPath = wallet
-            .getDerivationPath()
-            .split('/')
-            .slice(0, 4)
-            .join('/');
-        const xpub = await wallet.getMasterKey().getxpub(derivationPath);
+        const xpub = await wallet.getxpub();
         window.open(cExplorer.url + '/xpub/' + xpub, '_blank');
     } else {
-        const address =
-            strAddress || (await wallet.getMasterKey().getAddress());
+        const address = strAddress || (await wallet.getAddress());
         window.open(cExplorer.url + '/address/' + address, '_blank');
     }
 }
@@ -1270,7 +1265,7 @@ export async function guiImportWallet() {
             if (wallet.isLoaded()) {
                 // Prepare a new Account to add
                 const cAccount = new Account({
-                    publicKey: await wallet.getMasterKey().keyToExport,
+                    publicKey: await wallet.getKeyToExport(),
                     encWif: strPrivKey,
                 });
 
@@ -1521,7 +1516,7 @@ export async function sweepAddress(arrUTXOs, sweepingMasterKey, nFixedFee = 0) {
     const nFee = nFixedFee || getNetwork().getFee(cTx.serialize().length);
 
     // Use a new address from our wallet to sweep the UTXOs in to
-    const strAddress = (await wallet.getNewAddress(true, false))[0];
+    const strAddress = (await getNewAddress(true, false))[0];
 
     // Sweep the full funds amount, minus the fee, leaving no change from any sweeped UTXOs
     cTx.addoutput(strAddress, (nTotal - nFee) / COIN);
@@ -1601,7 +1596,7 @@ export async function wipePrivateData() {
             html,
         })
     ) {
-        wallet.getMasterKey().wipePrivateData();
+        wallet.wipePrivateData();
         doms.domWipeWallet.hidden = true;
         if (isEncrypted) {
             doms.domRestoreWallet.hidden = false;
@@ -2356,7 +2351,7 @@ export async function updateMasternodeTab() {
             for (const [key] of mapCollateralAddresses) {
                 const option = document.createElement('option');
                 option.value = key;
-                option.innerText = await wallet.getMasterKey().getAddress(key);
+                option.innerText = await wallet.getAddress(key);
                 doms.domMnTxId.appendChild(option);
             }
         }

diff --git a/scripts/index.js b/scripts/index.js
@@ -44,7 +44,12 @@ export {
     switchSettings,
     govVote,
 } from './global.js';
-export { wallet, generateWallet, importWallet } from './wallet.js';
+export {
+    wallet,
+    getNewAddress,
+    generateWallet,
+    importWallet,
+} from './wallet.js';
 export {
     toggleTestnet,
     toggleDebug,

diff --git a/scripts/masterkey.js b/scripts/masterkey.js
@@ -1,23 +1,18 @@
 import HDKey from 'hdkey';
-import { getNetwork } from './network.js';
 import { bytesToHex } from './utils.js';
 import { getHardwareWalletKeys } from './ledger.js';
-import { cChainParams, MAX_ACCOUNT_GAP } from './chain_params.js';
+import { cChainParams } from './chain_params.js';
 
 import { deriveAddress, generateOrEncodePrivkey } from './encoding.js';
 
 /**
- * Abstract class masterkey
+ * Abstract class masterkey, it handles address generation
+ * this class must not know anything about the wallet it self
+ * so for example don't take for granted nAccount when generating.
+ * Ideally the only class having access to those functions is the wallet itself.
  * @abstract
  */
 export class MasterKey {
-    #addressIndex = 0;
-    /**
-     * Map our own address -> Path
-     * @type {Map<String, String?>}
-     */
-    #ownAddresses = new Map();
-
     constructor() {
         if (this.constructor === MasterKey) {
             throw new Error('initializing virtual class');
@@ -66,7 +61,7 @@ export class MasterKey {
      * @return {void}
      * @abstract
      */
-    wipePrivateData() {
+    wipePrivateData(_nAccount) {
         throw new Error('Not implemented');
     }
 
@@ -82,7 +77,7 @@ export class MasterKey {
      * @return {Promise<String>} public key to export. Only suitable for monitoring balance.
      * @abstract
      */
-    get keyToExport() {
+    getKeyToExport(_nAccount) {
         throw new Error('Not implemented');
     }
 
@@ -108,7 +103,7 @@ export class MasterKey {
     }
 
     // Construct a full BIP44 pubkey derivation path from it's parts
-    getDerivationPath(nAccount = 0, nReceiving = 0, nIndex = 0) {
+    getDerivationPath(nAccount, nReceiving, nIndex) {
         // Coin-Type is different on Ledger, as such, for local wallets; we modify it if we're using a Ledger to derive a key
         const strCoinType = this.isHardwareWallet
             ? cChainParams.current.BIP44_TYPE_LEDGER
@@ -118,63 +113,6 @@ export class MasterKey {
         }
         return `m/44'/${strCoinType}'/${nAccount}'/${nReceiving}/${nIndex}`;
     }
-
-    /**
-     * @param {string} address - address to check
-     * @return {Promise<String?>} BIP32 path or null if it's not your address
-     */
-    async isOwnAddress(address) {
-        if (this.#ownAddresses.has(address)) {
-            return this.#ownAddresses.get(address);
-        }
-        const last = getNetwork().lastWallet;
-        this.#addressIndex =
-            this.#addressIndex > last ? this.#addressIndex : last;
-        if (this.isHD) {
-            for (let i = 0; i < this.#addressIndex; i++) {
-                const path = this.getDerivationPath(0, 0, i);
-                const testAddress = await this.getAddress(path);
-                if (address === testAddress) {
-                    this.#ownAddresses.set(address, path);
-                    return path;
-                }
-            }
-        } else {
-            const value = address === (await this.keyToExport) ? ':)' : null;
-            this.#ownAddresses.set(address, value);
-            return value;
-        }
-
-        this.#ownAddresses.set(address, null);
-        return null;
-    }
-
-    /**
-     * @return Promise<[string, string]> Address and its BIP32 derivation path
-     */
-    async getNewAddress() {
-        const last = getNetwork().lastWallet;
-        this.#addressIndex =
-            (this.#addressIndex > last ? this.#addressIndex : last) + 1;
-        if (this.#addressIndex - last > MAX_ACCOUNT_GAP) {
-            // If the user creates more than ${MAX_ACCOUNT_GAP} empty wallets we will not be able to sync them!
-            this.#addressIndex = last;
-        }
-        const path = this.getDerivationPath(0, 0, this.#addressIndex);
-        const address = await this.getAddress(path);
-        return [address, path];
-    }
-
-    /**
-     * Derive the current address (by internal index)
-     * @return {Promise<String>} Address
-     * @abstract
-     */
-    async getCurrentAddress() {
-        return await this.getAddress(
-            this.getDerivationPath(0, 0, this.#addressIndex)
-        );
-    }
 }
 
 export class HdMasterKey extends MasterKey {
@@ -229,18 +167,20 @@ export class HdMasterKey extends MasterKey {
         return deriveAddress({ publicKey: bytesToHex(child.publicKey) });
     }
 
-    wipePrivateData() {
+    wipePrivateData(nAccount) {
         if (this._isViewOnly) return;
 
-        this._hdKey = HDKey.fromExtendedKey(this.keyToExport);
+        this._hdKey = HDKey.fromExtendedKey(this.getKeyToExport(nAccount));
         this._isViewOnly = true;
     }
-
-    get keyToExport() {
+    getKeyToExport(nAccount) {
         if (this._isViewOnly) return this._hdKey.publicExtendedKey;
         // We need the xpub to point at the account level
         return this._hdKey.derive(
-            this.getDerivationPath(0, 0, 0).split('/').slice(0, 4).join('/')
+            this.getDerivationPath(nAccount, 0, 0)
+                .split('/')
+                .slice(0, 4)
+                .join('/')
         ).publicExtendedKey;
     }
 }
@@ -280,13 +220,13 @@ export class HardwareWalletMasterKey extends MasterKey {
     }
 
     // Hardware Wallets don't have exposed private data
-    wipePrivateData() {}
+    wipePrivateData(_nAccount) {}
 
     get isViewOnly() {
         return false;
     }
-    get keyToExport() {
-        const derivationPath = this.getDerivationPath()
+    getKeyToExport(nAccount) {
+        const derivationPath = this.getDerivationPath(nAccount, 0, 0)
             .split('/')
             .slice(0, 4)
             .join('/');
@@ -308,7 +248,7 @@ export class LegacyMasterKey extends MasterKey {
         return this._address;
     }
 
-    get keyToExport() {
+    getKeyToExport(_nAccount) {
         return this._address;
     }
 
@@ -331,7 +271,7 @@ export class LegacyMasterKey extends MasterKey {
         );
     }
 
-    wipePrivateData() {
+    wipePrivateData(_nAccount) {
         this._pkBytes = null;
         this._isViewOnly = true;
     }