Throw errors when the address is not specified or invalid

9 files changed, 67 insertions, 42 deletions

diff --git a/packages/subproviders/src/subproviders/base_wallet_subprovider.ts b/packages/subproviders/src/subproviders/base_wallet_subprovider.ts
index 47b45a126..0a9b99ae4 100644
--- a/packages/subproviders/src/subproviders/base_wallet_subprovider.ts
+++ b/packages/subproviders/src/subproviders/base_wallet_subprovider.ts
@@ -21,7 +21,7 @@ export abstract class BaseWalletSubprovider extends Subprovider {
 
     public abstract async getAccountsAsync(): Promise<string[]>;
     public abstract async signTransactionAsync(txParams: PartialTxParams): Promise<string>;
-    public abstract async signPersonalMessageAsync(data: string, address?: string): Promise<string>;
+    public abstract async signPersonalMessageAsync(data: string, address: string): Promise<string>;
 
     /**
      * This method conforms to the web3-provider-engine interface.
diff --git a/packages/subproviders/src/subproviders/ledger.ts b/packages/subproviders/src/subproviders/ledger.ts
index 6f66e3018..3c0442e4c 100644
--- a/packages/subproviders/src/subproviders/ledger.ts
+++ b/packages/subproviders/src/subproviders/ledger.ts
@@ -105,11 +105,9 @@ export class LedgerSubprovider extends BaseWalletSubprovider {
     public async signTransactionAsync(txParams: PartialTxParams): Promise<string> {
         LedgerSubprovider._validateTxParams(txParams);
         const initialDerivedKey = await this._initialDerivedKeyAsync();
-        const derivedKey = _.isUndefined(txParams.from)
-            ? walletUtils._firstDerivedKey(initialDerivedKey)
-            : this._findDerivedKeyByPublicAddress(initialDerivedKey, txParams.from);
+        const derivedKey = this._findDerivedKeyByPublicAddress(initialDerivedKey, txParams.from);
 
-        this._ledgerClientIfExists = await this._createLedgerClientAsync();
+        const ledgerClient = await this._createLedgerClientAsync();
 
         const tx = new EthereumTx(txParams);
 
@@ -121,7 +119,7 @@ export class LedgerSubprovider extends BaseWalletSubprovider {
         const txHex = tx.serialize().toString('hex');
         try {
             const derivationPath = `${derivedKey.derivationPath}/${derivedKey.derivationIndex}`;
-            const result = await this._ledgerClientIfExists.signTransaction(derivationPath, txHex);
+            const result = await ledgerClient.signTransaction(derivationPath, txHex);
             // Store signature in transaction
             tx.r = Buffer.from(result.r, 'hex');
             tx.s = Buffer.from(result.s, 'hex');
@@ -145,7 +143,7 @@ export class LedgerSubprovider extends BaseWalletSubprovider {
     }
     /**
      * Sign a personal Ethereum signed message. The signing address will be the one
-     * either the provided address or the first address on the ledger device.
+     * the provided address.
      * The Ledger adds the Ethereum signed message prefix on-device.  If you've added
      * the LedgerSubprovider to your app's provider, you can simply send an `eth_sign`
      * or `personal_sign` JSON RPC request, and this method will be called auto-magically.
@@ -154,23 +152,18 @@ export class LedgerSubprovider extends BaseWalletSubprovider {
      * @param address Address to sign with
      * @return Signature hex string (order: rsv)
      */
-    public async signPersonalMessageAsync(data: string, address?: string): Promise<string> {
+    public async signPersonalMessageAsync(data: string, address: string): Promise<string> {
         if (_.isUndefined(data)) {
             throw new Error(WalletSubproviderErrors.DataMissingForSignPersonalMessage);
         }
         assert.isHexString('data', data);
         const initialDerivedKey = await this._initialDerivedKeyAsync();
-        const derivedKey = _.isUndefined(address)
-            ? walletUtils._firstDerivedKey(initialDerivedKey)
-            : this._findDerivedKeyByPublicAddress(initialDerivedKey, address);
+        const derivedKey = this._findDerivedKeyByPublicAddress(initialDerivedKey, address);
 
-        this._ledgerClientIfExists = await this._createLedgerClientAsync();
+        const ledgerClient = await this._createLedgerClientAsync();
         try {
             const derivationPath = `${derivedKey.derivationPath}/${derivedKey.derivationIndex}`;
-            const result = await this._ledgerClientIfExists.signPersonalMessage(
-                derivationPath,
-                ethUtil.stripHexPrefix(data),
-            );
+            const result = await ledgerClient.signPersonalMessage(derivationPath, ethUtil.stripHexPrefix(data));
             const v = result.v - 27;
             let vHex = v.toString(16);
             if (vHex.length < 2) {
@@ -192,6 +185,7 @@ export class LedgerSubprovider extends BaseWalletSubprovider {
         }
         const ledgerEthereumClient = await this._ledgerEthereumClientFactoryAsync();
         this._connectionLock.release();
+        this._ledgerClientIfExists = ledgerEthereumClient;
         return ledgerEthereumClient;
     }
     private async _destroyLedgerClientAsync() {
@@ -205,11 +199,11 @@ export class LedgerSubprovider extends BaseWalletSubprovider {
         this._connectionLock.release();
     }
     private async _initialDerivedKeyAsync(): Promise<DerivedHDKey> {
-        this._ledgerClientIfExists = await this._createLedgerClientAsync();
+        const ledgerClient = await this._createLedgerClientAsync();
 
         let ledgerResponse;
         try {
-            ledgerResponse = await this._ledgerClientIfExists.getAddress(
+            ledgerResponse = await ledgerClient.getAddress(
                 this._derivationPath,
                 this._shouldAlwaysAskForConfirmation,
                 SHOULD_GET_CHAIN_CODE,
@@ -229,6 +223,9 @@ export class LedgerSubprovider extends BaseWalletSubprovider {
         };
     }
     private _findDerivedKeyByPublicAddress(initalHDKey: DerivedHDKey, address: string): DerivedHDKey {
+        if (_.isUndefined(address)) {
+            throw new Error(WalletSubproviderErrors.FromAddressMissingOrInvalid);
+        }
         const matchedDerivedKey = walletUtils.findDerivedKeyByAddress(address, initalHDKey, this._addressSearchLimit);
         if (_.isUndefined(matchedDerivedKey)) {
             throw new Error(`${WalletSubproviderErrors.AddressNotFound}: ${address}`);
diff --git a/packages/subproviders/src/subproviders/mnemonic_wallet_subprovider.ts b/packages/subproviders/src/subproviders/mnemonic_wallet_subprovider.ts
index 53013c44c..730453bc6 100644
--- a/packages/subproviders/src/subproviders/mnemonic_wallet_subprovider.ts
+++ b/packages/subproviders/src/subproviders/mnemonic_wallet_subprovider.ts
@@ -63,6 +63,10 @@ export class MnemonicWalletSubprovider extends BaseWalletSubprovider {
      */
     public setPath(derivationPath: string) {
         this._derivationPath = derivationPath;
+        this._derivedKey = {
+            ...this._derivedKey,
+            derivationPath: this._derivationPath,
+        };
     }
     /**
      * Retrieve the accounts associated with the mnemonic.
@@ -88,10 +92,7 @@ export class MnemonicWalletSubprovider extends BaseWalletSubprovider {
      * @return Signed transaction hex string
      */
     public async signTransactionAsync(txParams: PartialTxParams): Promise<string> {
-        const derivedKey = _.isUndefined(txParams.from)
-            ? walletUtils._firstDerivedKey(this._derivedKey)
-            : this._findDerivedKeyByPublicAddress(txParams.from);
-        const privateKeyWallet = new PrivateKeyWalletSubprovider(derivedKey.hdKey.privateKey.toString('hex'));
+        const privateKeyWallet = this._privateKeyWalletFromAddress(txParams.from);
         const signedTx = privateKeyWallet.signTransactionAsync(txParams);
         return signedTx;
     }
@@ -105,15 +106,21 @@ export class MnemonicWalletSubprovider extends BaseWalletSubprovider {
      * @param address Address to sign with
      * @return Signature hex string (order: rsv)
      */
-    public async signPersonalMessageAsync(data: string, address?: string): Promise<string> {
-        const derivedKey = _.isUndefined(address)
-            ? walletUtils._firstDerivedKey(this._derivedKey)
-            : this._findDerivedKeyByPublicAddress(address);
-        const privateKeyWallet = new PrivateKeyWalletSubprovider(derivedKey.hdKey.privateKey.toString('hex'));
-        const sig = await privateKeyWallet.signPersonalMessageAsync(data, derivedKey.address);
+    public async signPersonalMessageAsync(data: string, address: string): Promise<string> {
+        const privateKeyWallet = this._privateKeyWalletFromAddress(address);
+        const sig = await privateKeyWallet.signPersonalMessageAsync(data, address);
         return sig;
     }
+    private _privateKeyWalletFromAddress(address: string): PrivateKeyWalletSubprovider {
+        const derivedKey = this._findDerivedKeyByPublicAddress(address);
+        const privateKeyHex = derivedKey.hdKey.privateKey.toString('hex');
+        const privateKeyWallet = new PrivateKeyWalletSubprovider(privateKeyHex);
+        return privateKeyWallet;
+    }
     private _findDerivedKeyByPublicAddress(address: string): DerivedHDKey {
+        if (_.isUndefined(address)) {
+            throw new Error(WalletSubproviderErrors.FromAddressMissingOrInvalid);
+        }
         const matchedDerivedKey = walletUtils.findDerivedKeyByAddress(
             address,
             this._derivedKey,
diff --git a/packages/subproviders/src/subproviders/private_key_wallet_subprovider.ts b/packages/subproviders/src/subproviders/private_key_wallet_subprovider.ts
index 005d36f93..f8afab722 100644
--- a/packages/subproviders/src/subproviders/private_key_wallet_subprovider.ts
+++ b/packages/subproviders/src/subproviders/private_key_wallet_subprovider.ts
@@ -63,15 +63,15 @@ export class PrivateKeyWalletSubprovider extends BaseWalletSubprovider {
      * @param address Address to sign with
      * @return Signature hex string (order: rsv)
      */
-    public async signPersonalMessageAsync(dataIfExists: string, address?: string): Promise<string> {
-        if (_.isUndefined(dataIfExists)) {
+    public async signPersonalMessageAsync(data: string, address: string): Promise<string> {
+        if (_.isUndefined(data)) {
             throw new Error(WalletSubproviderErrors.DataMissingForSignPersonalMessage);
         }
-        if (!_.isUndefined(address) && address !== this._address) {
-            throw new Error(`${WalletSubproviderErrors.AddressNotFound}: ${address}`);
+        if (_.isUndefined(address) || address !== this._address) {
+            throw new Error(`${WalletSubproviderErrors.FromAddressMissingOrInvalid}: ${address}`);
         }
-        assert.isHexString('data', dataIfExists);
-        const dataBuff = ethUtil.toBuffer(dataIfExists);
+        assert.isHexString('data', data);
+        const dataBuff = ethUtil.toBuffer(data);
         const msgHashBuff = ethUtil.hashPersonalMessage(dataBuff);
         const sig = ethUtil.ecsign(msgHashBuff, this._privateKeyBuffer);
         const rpcSig = ethUtil.toRpcSig(sig.v, sig.r, sig.s);
diff --git a/packages/subproviders/src/types.ts b/packages/subproviders/src/types.ts
index 9f557731f..140c7c2df 100644
--- a/packages/subproviders/src/types.ts
+++ b/packages/subproviders/src/types.ts
@@ -80,7 +80,7 @@ export interface PartialTxParams {
     gasPrice?: string;
     gas: string;
     to: string;
-    from?: string;
+    from: string;
     value?: string;
     data?: string;
     chainId: number; // EIP 155 chainId - mainnet: 1, ropsten: 3
@@ -101,10 +101,10 @@ export enum WalletSubproviderErrors {
     AddressNotFound = 'ADDRESS_NOT_FOUND',
     DataMissingForSignPersonalMessage = 'DATA_MISSING_FOR_SIGN_PERSONAL_MESSAGE',
     SenderInvalidOrNotSupplied = 'SENDER_INVALID_OR_NOT_SUPPLIED',
+    FromAddressMissingOrInvalid = 'FROM_ADDRESS_MISSING_OR_INVALID',
 }
 export enum LedgerSubproviderErrors {
     TooOldLedgerFirmware = 'TOO_OLD_LEDGER_FIRMWARE',
-    FromAddressMissingOrInvalid = 'FROM_ADDRESS_MISSING_OR_INVALID',
     MultipleOpenConnectionsDisallowed = 'MULTIPLE_OPEN_CONNECTIONS_DISALLOWED',
 }
 
diff --git a/packages/subproviders/test/integration/ledger_subprovider_test.ts b/packages/subproviders/test/integration/ledger_subprovider_test.ts
index 88a3c6db1..4a831af67 100644
--- a/packages/subproviders/test/integration/ledger_subprovider_test.ts
+++ b/packages/subproviders/test/integration/ledger_subprovider_test.ts
@@ -55,7 +55,10 @@ describe('LedgerSubprovider', () => {
         });
         it('signs a personal message', async () => {
             const data = ethUtils.bufferToHex(ethUtils.toBuffer(fixtureData.PERSONAL_MESSAGE_STRING));
-            const ecSignatureHex = await ledgerSubprovider.signPersonalMessageAsync(data);
+            const ecSignatureHex = await ledgerSubprovider.signPersonalMessageAsync(
+                data,
+                fixtureData.TEST_RPC_ACCOUNT_0,
+            );
             expect(ecSignatureHex.length).to.be.equal(132);
             expect(ecSignatureHex).to.be.equal(fixtureData.PERSONAL_MESSAGE_SIGNED_RESULT);
         });
diff --git a/packages/subproviders/test/unit/ledger_subprovider_test.ts b/packages/subproviders/test/unit/ledger_subprovider_test.ts
index d3a3fbe26..ad1154831 100644
--- a/packages/subproviders/test/unit/ledger_subprovider_test.ts
+++ b/packages/subproviders/test/unit/ledger_subprovider_test.ts
@@ -82,7 +82,7 @@ describe('LedgerSubprovider', () => {
             });
             it('signs a personal message', async () => {
                 const data = ethUtils.bufferToHex(ethUtils.toBuffer(fixtureData.PERSONAL_MESSAGE_STRING));
-                const ecSignatureHex = await ledgerSubprovider.signPersonalMessageAsync(data);
+                const ecSignatureHex = await ledgerSubprovider.signPersonalMessageAsync(data, FAKE_ADDRESS);
                 expect(ecSignatureHex).to.be.equal(
                     '0xa6cc284bff14b42bdf5e9286730c152be91719d478605ec46b3bebcd0ae491480652a1a7b742ceb0213d1e744316e285f41f878d8af0b8e632cbca4c279132d001',
                 );
@@ -94,7 +94,7 @@ describe('LedgerSubprovider', () => {
                 return expect(
                     Promise.all([
                         ledgerSubprovider.getAccountsAsync(),
-                        ledgerSubprovider.signPersonalMessageAsync(data),
+                        ledgerSubprovider.signPersonalMessageAsync(data, FAKE_ADDRESS),
                     ]),
                 ).to.be.rejectedWith(LedgerSubproviderErrors.MultipleOpenConnectionsDisallowed);
             });
@@ -168,6 +168,7 @@ describe('LedgerSubprovider', () => {
                     gasPrice: '0x00',
                     nonce: '0x00',
                     gas: '0x00',
+                    from: FAKE_ADDRESS,
                 };
                 const payload = {
                     jsonrpc: '2.0',
diff --git a/packages/subproviders/test/unit/mnemonic_wallet_subprovider_test.ts b/packages/subproviders/test/unit/mnemonic_wallet_subprovider_test.ts
index 77e5d35ae..a1a5a2296 100644
--- a/packages/subproviders/test/unit/mnemonic_wallet_subprovider_test.ts
+++ b/packages/subproviders/test/unit/mnemonic_wallet_subprovider_test.ts
@@ -36,7 +36,7 @@ describe('MnemonicWalletSubprovider', () => {
             });
             it('signs a personal message', async () => {
                 const data = ethUtils.bufferToHex(ethUtils.toBuffer(fixtureData.PERSONAL_MESSAGE_STRING));
-                const ecSignatureHex = await subprovider.signPersonalMessageAsync(data);
+                const ecSignatureHex = await subprovider.signPersonalMessageAsync(data, fixtureData.TEST_RPC_ACCOUNT_0);
                 expect(ecSignatureHex).to.be.equal(fixtureData.PERSONAL_MESSAGE_SIGNED_RESULT);
             });
             it('signs a transaction', async () => {
diff --git a/packages/subproviders/test/unit/private_key_wallet_subprovider_test.ts b/packages/subproviders/test/unit/private_key_wallet_subprovider_test.ts
index 8aaddeaf4..b84aebb18 100644
--- a/packages/subproviders/test/unit/private_key_wallet_subprovider_test.ts
+++ b/packages/subproviders/test/unit/private_key_wallet_subprovider_test.ts
@@ -32,7 +32,7 @@ describe('PrivateKeyWalletSubprovider', () => {
             });
             it('signs a personal message', async () => {
                 const data = ethUtils.bufferToHex(ethUtils.toBuffer(fixtureData.PERSONAL_MESSAGE_STRING));
-                const ecSignatureHex = await subprovider.signPersonalMessageAsync(data);
+                const ecSignatureHex = await subprovider.signPersonalMessageAsync(data, fixtureData.TEST_RPC_ACCOUNT_0);
                 expect(ecSignatureHex).to.be.equal(fixtureData.PERSONAL_MESSAGE_SIGNED_RESULT);
             });
             it('signs a transaction', async () => {
@@ -128,6 +128,23 @@ describe('PrivateKeyWalletSubprovider', () => {
                 });
                 provider.sendAsync(payload, callback);
             });
+            it('should throw if `address` param is not an address from private key when calling personal_sign', (done: DoneCallback) => {
+                const nonHexMessage = 'hello world';
+                const payload = {
+                    jsonrpc: '2.0',
+                    method: 'personal_sign',
+                    params: [nonHexMessage, fixtureData.TEST_RPC_ACCOUNT_1],
+                    id: 1,
+                };
+                const callback = reportCallbackErrors(done)((err: Error, response: JSONRPCResponsePayload) => {
+                    expect(err).to.not.be.a('null');
+                    expect(err.message).to.be.equal(
+                        `${WalletSubproviderErrors.FromAddressMissingOrInvalid}: ${fixtureData.TEST_RPC_ACCOUNT_1}`,
+                    );
+                    done();
+                });
+                provider.sendAsync(payload, callback);
+            });
             it('should throw if `from` param missing when calling eth_sendTransaction', (done: DoneCallback) => {
                 const tx = {
                     to: '0xafa3f8684e54059998bc3a7b0d2b0da075154d66',
@@ -175,7 +192,7 @@ describe('PrivateKeyWalletSubprovider', () => {
                 };
                 const callback = reportCallbackErrors(done)((err: Error, response: JSONRPCResponsePayload) => {
                     expect(err).to.not.be.a('null');
-                    expect(err.message).to.be.equal(`${WalletSubproviderErrors.AddressNotFound}: 0x0`);
+                    expect(err.message).to.be.equal(`${WalletSubproviderErrors.FromAddressMissingOrInvalid}: 0x0`);
                     done();
                 });
                 provider.sendAsync(payload, callback);