From 8e84f9c4e89c3abcd6f3f6a0f818dcc478c783f3 Mon Sep 17 00:00:00 2001 From: Leon Date: Mon, 15 Aug 2022 19:17:05 +0800 Subject: [PATCH 01/15] Support for differentiating hardware wallet vendors --- CHANGELOG.md | 3 +++ index.js | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c52265..6cfe126 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.10.1] +- Support for differentiating hardware wallet vendors + ## [0.10.0] ### Added - Support for EIP-721 signTypedData_v4 ([#117](https://github.com/MetaMask/eth-trezor-keyring/pull/117)) diff --git a/index.js b/index.js index 9b76a29..e8c18b0 100644 --- a/index.js +++ b/index.js @@ -22,6 +22,31 @@ const TREZOR_CONNECT_MANIFEST = { appUrl: 'https://metamask.io', }; +/** + * Distinguish the OneKey hardware wallet by the serialNo prefix + * @param {*} features + * @returns {'mini' | 'touch' | 'classic' | 'trezor'} + */ +function isOneKeyDevice(features) { + const serialNo = features.serial_no; + if (serialNo) { + const miniFlag = serialNo.slice(0, 2); + if (miniFlag.toLowerCase() === 'mi') { + return 'mini'; + } + + if (miniFlag.toLowerCase() === 'tc') { + return 'touch'; + } + } + const name = features.ble_name; + const re = /(BixinKey\d{10})|(K\d{4})|(T\d{4})/iu; + if (name && re.exec(name)) { + return 'classic'; + } + return 'trezor'; +} + function wait(ms) { return new Promise((resolve) => setTimeout(resolve, ms)); } @@ -58,11 +83,14 @@ class TrezorKeyring extends EventEmitter { this.perPage = 5; this.unlockedAccount = 0; this.paths = {}; + this.vendor = 'trezor'; this.deserialize(opts); TrezorConnect.on('DEVICE_EVENT', (event) => { if (event && event.payload && event.payload.features) { this.model = event.payload.features.model; + const vendor = isOneKeyDevice(event.payload.features); + this.vendor = vendor === 'trezor' ? 'trezor' : 'onekey'; } }); TrezorConnect.init({ manifest: TREZOR_CONNECT_MANIFEST }); @@ -78,6 +106,15 @@ class TrezorKeyring extends EventEmitter { return this.model; } + /** + * Gets the vendor, if known. + * + * @returns {"trezor" | "onekey"} + */ + getVendor() { + return this.vendor; + } + dispose() { // This removes the Trezor Connect iframe from the DOM // This method is not well documented, but the code it calls can be seen From 0ff5cf0e0cb34f9a7e15120b8a4280e8c58f7347 Mon Sep 17 00:00:00 2001 From: Leon Date: Tue, 16 Aug 2022 08:33:02 +0800 Subject: [PATCH 02/15] Fix vendor case --- index.js | 42 ++++++++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/index.js b/index.js index e8c18b0..39a3608 100644 --- a/index.js +++ b/index.js @@ -22,27 +22,28 @@ const TREZOR_CONNECT_MANIFEST = { appUrl: 'https://metamask.io', }; +const oneKeySpecialVersion = 99; /** * Distinguish the OneKey hardware wallet by the serialNo prefix * @param {*} features * @returns {'mini' | 'touch' | 'classic' | 'trezor'} */ function isOneKeyDevice(features) { - const serialNo = features.serial_no; - if (serialNo) { - const miniFlag = serialNo.slice(0, 2); - if (miniFlag.toLowerCase() === 'mi') { - return 'mini'; - } - - if (miniFlag.toLowerCase() === 'tc') { - return 'touch'; - } + if ( + !features || + typeof features !== 'object' || + !features.minor_version || + !features.patch_version + ) { + return 'trezor'; } - const name = features.ble_name; - const re = /(BixinKey\d{10})|(K\d{4})|(T\d{4})/iu; - if (name && re.exec(name)) { - return 'classic'; + const minorVersion = Number(features.minor_version); + const patchVersion = Number(features.patch_version); + if ( + minorVersion === oneKeySpecialVersion && + patchVersion === oneKeySpecialVersion + ) { + return 'onekey'; } return 'trezor'; } @@ -83,14 +84,12 @@ class TrezorKeyring extends EventEmitter { this.perPage = 5; this.unlockedAccount = 0; this.paths = {}; - this.vendor = 'trezor'; + this.vendor = undefined; this.deserialize(opts); TrezorConnect.on('DEVICE_EVENT', (event) => { if (event && event.payload && event.payload.features) { this.model = event.payload.features.model; - const vendor = isOneKeyDevice(event.payload.features); - this.vendor = vendor === 'trezor' ? 'trezor' : 'onekey'; } }); TrezorConnect.init({ manifest: TREZOR_CONNECT_MANIFEST }); @@ -158,7 +157,14 @@ class TrezorKeyring extends EventEmitter { if (response.success) { this.hdk.publicKey = Buffer.from(response.payload.publicKey, 'hex'); this.hdk.chainCode = Buffer.from(response.payload.chainCode, 'hex'); - resolve('just unlocked'); + // Determine the vendor for statistics + TrezorConnect.getFeatures((features) => { + if (features.success) { + this.vendor = isOneKeyDevice(features.payload); + } + }).finally(() => { + resolve('just unlocked'); + }); } else { reject( new Error( From cf071748aa42cdfeaa58e9e60fe343373293d740 Mon Sep 17 00:00:00 2001 From: Leon Date: Tue, 16 Aug 2022 09:40:23 +0800 Subject: [PATCH 03/15] Typo for call getFeatures --- index.js | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/index.js b/index.js index 39a3608..57e3cd3 100644 --- a/index.js +++ b/index.js @@ -158,13 +158,15 @@ class TrezorKeyring extends EventEmitter { this.hdk.publicKey = Buffer.from(response.payload.publicKey, 'hex'); this.hdk.chainCode = Buffer.from(response.payload.chainCode, 'hex'); // Determine the vendor for statistics - TrezorConnect.getFeatures((features) => { - if (features.success) { - this.vendor = isOneKeyDevice(features.payload); - } - }).finally(() => { - resolve('just unlocked'); - }); + TrezorConnect.getFeatures() + .then((features) => { + if (features.success) { + this.vendor = isOneKeyDevice(features.payload); + } + }) + .finally(() => { + resolve('just unlocked'); + }); } else { reject( new Error( From 768bff1c8a5b40a63908d0a206262b93a4baabf3 Mon Sep 17 00:00:00 2001 From: Leon Date: Thu, 18 Aug 2022 11:07:49 +0800 Subject: [PATCH 04/15] Tagging devices by the vendor field --- index.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/index.js b/index.js index 57e3cd3..cf21932 100644 --- a/index.js +++ b/index.js @@ -23,6 +23,7 @@ const TREZOR_CONNECT_MANIFEST = { }; const oneKeySpecialVersion = 99; +const oneKeyVendor = 'onekey.so'; /** * Distinguish the OneKey hardware wallet by the serialNo prefix * @param {*} features @@ -39,9 +40,11 @@ function isOneKeyDevice(features) { } const minorVersion = Number(features.minor_version); const patchVersion = Number(features.patch_version); + const vendor = features.vendor || ''; if ( - minorVersion === oneKeySpecialVersion && - patchVersion === oneKeySpecialVersion + vendor === oneKeyVendor || + (minorVersion === oneKeySpecialVersion && + patchVersion === oneKeySpecialVersion) ) { return 'onekey'; } From dec994e3b1de563354ec6db9cc0083f6c2002ce9 Mon Sep 17 00:00:00 2001 From: Leon Date: Fri, 19 Aug 2022 10:40:25 +0800 Subject: [PATCH 05/15] Adding setVendor instance method --- index.js | 74 ++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 45 insertions(+), 29 deletions(-) diff --git a/index.js b/index.js index cf21932..6e10543 100644 --- a/index.js +++ b/index.js @@ -24,32 +24,6 @@ const TREZOR_CONNECT_MANIFEST = { const oneKeySpecialVersion = 99; const oneKeyVendor = 'onekey.so'; -/** - * Distinguish the OneKey hardware wallet by the serialNo prefix - * @param {*} features - * @returns {'mini' | 'touch' | 'classic' | 'trezor'} - */ -function isOneKeyDevice(features) { - if ( - !features || - typeof features !== 'object' || - !features.minor_version || - !features.patch_version - ) { - return 'trezor'; - } - const minorVersion = Number(features.minor_version); - const patchVersion = Number(features.patch_version); - const vendor = features.vendor || ''; - if ( - vendor === oneKeyVendor || - (minorVersion === oneKeySpecialVersion && - patchVersion === oneKeySpecialVersion) - ) { - return 'onekey'; - } - return 'trezor'; -} function wait(ms) { return new Promise((resolve) => setTimeout(resolve, ms)); @@ -117,6 +91,44 @@ class TrezorKeyring extends EventEmitter { return this.vendor; } + /** + * set the vendor name of the hardware wallet + * @param {*} features + * @returns {'onekey' | 'trezor'} + */ + __setVendor(features) { + // If the value of features is null, set vendor to the default value + if (features === null) { + this.vendor = undefined; + return; + } + + // No special field, default is trezor device + if ( + !features || + typeof features !== 'object' || + !features.minor_version || + !features.patch_version + ) { + this.vendor = 'trezor'; + return; + } + + const minorVersion = Number(features.minor_version); + const patchVersion = Number(features.patch_version); + const vendor = features.vendor || ''; + if ( + vendor === oneKeyVendor || + (minorVersion === oneKeySpecialVersion && + patchVersion === oneKeySpecialVersion) + ) { + this.vendor = 'onekey'; + return; + } + + this.vendor = 'trezor'; + } + dispose() { // This removes the Trezor Connect iframe from the DOM // This method is not well documented, but the code it calls can be seen @@ -163,9 +175,13 @@ class TrezorKeyring extends EventEmitter { // Determine the vendor for statistics TrezorConnect.getFeatures() .then((features) => { - if (features.success) { - this.vendor = isOneKeyDevice(features.payload); - } + const featuresPayload = features.success + ? features.payload + : null; + this.__setVendor(featuresPayload); + }) + .catch(() => { + this.__setVendor(null); }) .finally(() => { resolve('just unlocked'); From d5209cb34787fea33037af66dcf44c520f6ae68f Mon Sep 17 00:00:00 2001 From: Leon Date: Fri, 19 Aug 2022 11:10:00 +0800 Subject: [PATCH 06/15] Adding setVendor instance method --- index.js | 74 ++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 45 insertions(+), 29 deletions(-) diff --git a/index.js b/index.js index cf21932..6e10543 100644 --- a/index.js +++ b/index.js @@ -24,32 +24,6 @@ const TREZOR_CONNECT_MANIFEST = { const oneKeySpecialVersion = 99; const oneKeyVendor = 'onekey.so'; -/** - * Distinguish the OneKey hardware wallet by the serialNo prefix - * @param {*} features - * @returns {'mini' | 'touch' | 'classic' | 'trezor'} - */ -function isOneKeyDevice(features) { - if ( - !features || - typeof features !== 'object' || - !features.minor_version || - !features.patch_version - ) { - return 'trezor'; - } - const minorVersion = Number(features.minor_version); - const patchVersion = Number(features.patch_version); - const vendor = features.vendor || ''; - if ( - vendor === oneKeyVendor || - (minorVersion === oneKeySpecialVersion && - patchVersion === oneKeySpecialVersion) - ) { - return 'onekey'; - } - return 'trezor'; -} function wait(ms) { return new Promise((resolve) => setTimeout(resolve, ms)); @@ -117,6 +91,44 @@ class TrezorKeyring extends EventEmitter { return this.vendor; } + /** + * set the vendor name of the hardware wallet + * @param {*} features + * @returns {'onekey' | 'trezor'} + */ + __setVendor(features) { + // If the value of features is null, set vendor to the default value + if (features === null) { + this.vendor = undefined; + return; + } + + // No special field, default is trezor device + if ( + !features || + typeof features !== 'object' || + !features.minor_version || + !features.patch_version + ) { + this.vendor = 'trezor'; + return; + } + + const minorVersion = Number(features.minor_version); + const patchVersion = Number(features.patch_version); + const vendor = features.vendor || ''; + if ( + vendor === oneKeyVendor || + (minorVersion === oneKeySpecialVersion && + patchVersion === oneKeySpecialVersion) + ) { + this.vendor = 'onekey'; + return; + } + + this.vendor = 'trezor'; + } + dispose() { // This removes the Trezor Connect iframe from the DOM // This method is not well documented, but the code it calls can be seen @@ -163,9 +175,13 @@ class TrezorKeyring extends EventEmitter { // Determine the vendor for statistics TrezorConnect.getFeatures() .then((features) => { - if (features.success) { - this.vendor = isOneKeyDevice(features.payload); - } + const featuresPayload = features.success + ? features.payload + : null; + this.__setVendor(featuresPayload); + }) + .catch(() => { + this.__setVendor(null); }) .finally(() => { resolve('just unlocked'); From f05be0db9cdda5620bb298ca1b68f7e8ad2d44ff Mon Sep 17 00:00:00 2001 From: Leon Date: Sat, 20 Aug 2022 12:08:10 +0800 Subject: [PATCH 07/15] Apply suggestions from code review Co-authored-by: legobeat <109787230+legobeat@users.noreply.github.com> --- index.js | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/index.js b/index.js index 6e10543..e184882 100644 --- a/index.js +++ b/index.js @@ -98,29 +98,22 @@ class TrezorKeyring extends EventEmitter { */ __setVendor(features) { // If the value of features is null, set vendor to the default value - if (features === null) { + if (!features) { this.vendor = undefined; return; } // No special field, default is trezor device if ( - !features || - typeof features !== 'object' || - !features.minor_version || - !features.patch_version + !features?.minor_version || + !features?.patch_version ) { - this.vendor = 'trezor'; - return; + return 'trezor'; } - const minorVersion = Number(features.minor_version); - const patchVersion = Number(features.patch_version); - const vendor = features.vendor || ''; if ( - vendor === oneKeyVendor || - (minorVersion === oneKeySpecialVersion && - patchVersion === oneKeySpecialVersion) + this.vendor === oneKeyVendor + || (features.minor_version === oneKeySpecialVersion && features.patch_version === oneKeySpecialVersion) ) { this.vendor = 'onekey'; return; From 4871cdda251489c68bcc9b143842b957cbe5956f Mon Sep 17 00:00:00 2001 From: Leon Date: Sat, 20 Aug 2022 13:14:11 +0800 Subject: [PATCH 08/15] Lazy load for getVendor --- index.js | 66 ++++++++++++++++++++++++++++++-------------------------- 1 file changed, 36 insertions(+), 30 deletions(-) diff --git a/index.js b/index.js index e184882..8052aea 100644 --- a/index.js +++ b/index.js @@ -87,39 +87,58 @@ class TrezorKeyring extends EventEmitter { * * @returns {"trezor" | "onekey"} */ - getVendor() { - return this.vendor; + async getVendor() { + return this.vendor || (this.vendor = await this.fetchVendor()); } /** - * set the vendor name of the hardware wallet - * @param {*} features + * fetch vendor by call getFeatures + * @param {object} features * @returns {'onekey' | 'trezor'} */ - __setVendor(features) { + fetchVendor() { + return new Promise((resolve) => { + TrezorConnect.getFeatures() + .then((response) => { + if (!response.success) { + resolve(null); + return; + } + const features = response.payload; + const vendor = this.__getVendor(features); + resolve(vendor); + }) + .catch(() => { + resolve(null); + }); + }); + } + + /** + * get the vendor name of the hardware wallet + * @param {object} features + * @returns {'onekey' | 'trezor'} + */ + __getVendor(features) { // If the value of features is null, set vendor to the default value if (!features) { - this.vendor = undefined; - return; + return undefined; } // No special field, default is trezor device - if ( - !features?.minor_version || - !features?.patch_version - ) { + if (!features.minor_version || !features.patch_version) { return 'trezor'; } if ( - this.vendor === oneKeyVendor - || (features.minor_version === oneKeySpecialVersion && features.patch_version === oneKeySpecialVersion) + features.vendor === oneKeyVendor || + (features.minor_version === oneKeySpecialVersion && + features.patch_version === oneKeySpecialVersion) ) { - this.vendor = 'onekey'; - return; + return 'onekey'; } - this.vendor = 'trezor'; + return 'trezor'; } dispose() { @@ -165,20 +184,7 @@ class TrezorKeyring extends EventEmitter { if (response.success) { this.hdk.publicKey = Buffer.from(response.payload.publicKey, 'hex'); this.hdk.chainCode = Buffer.from(response.payload.chainCode, 'hex'); - // Determine the vendor for statistics - TrezorConnect.getFeatures() - .then((features) => { - const featuresPayload = features.success - ? features.payload - : null; - this.__setVendor(featuresPayload); - }) - .catch(() => { - this.__setVendor(null); - }) - .finally(() => { - resolve('just unlocked'); - }); + resolve('just unlocked'); } else { reject( new Error( From e6ae4076646ded4d11420403a1ae020b3462b212 Mon Sep 17 00:00:00 2001 From: Leon Date: Sat, 20 Aug 2022 13:17:29 +0800 Subject: [PATCH 09/15] Lazy load for getVendor method --- index.js | 65 ++++++++++++++++++++++++++++++-------------------------- 1 file changed, 35 insertions(+), 30 deletions(-) diff --git a/index.js b/index.js index e184882..575151a 100644 --- a/index.js +++ b/index.js @@ -87,39 +87,57 @@ class TrezorKeyring extends EventEmitter { * * @returns {"trezor" | "onekey"} */ - getVendor() { - return this.vendor; + async getVendor() { + return this.vendor || (this.vendor = await this.fetchVendor()); } /** - * set the vendor name of the hardware wallet - * @param {*} features + * fetch vendor by call getFeatures + * @param {object} features * @returns {'onekey' | 'trezor'} */ - __setVendor(features) { + fetchVendor() { + return new Promise((resolve) => { + TrezorConnect.getFeatures() + .then((response) => { + if (!response.success) { + resolve(null); + return; + } + const vendor = this.__getVendor(response.payload); + resolve(vendor); + }) + .catch(() => { + resolve(null); + }); + }); + } + + /** + * get the vendor name of the hardware wallet + * @param {object} features + * @returns {'onekey' | 'trezor'} + */ + __getVendor(features) { // If the value of features is null, set vendor to the default value if (!features) { - this.vendor = undefined; - return; + return undefined; } // No special field, default is trezor device - if ( - !features?.minor_version || - !features?.patch_version - ) { + if (!features.minor_version || !features.patch_version) { return 'trezor'; } if ( - this.vendor === oneKeyVendor - || (features.minor_version === oneKeySpecialVersion && features.patch_version === oneKeySpecialVersion) + features.vendor === oneKeyVendor || + (features.minor_version === oneKeySpecialVersion && + features.patch_version === oneKeySpecialVersion) ) { - this.vendor = 'onekey'; - return; + return 'onekey'; } - this.vendor = 'trezor'; + return 'trezor'; } dispose() { @@ -165,20 +183,7 @@ class TrezorKeyring extends EventEmitter { if (response.success) { this.hdk.publicKey = Buffer.from(response.payload.publicKey, 'hex'); this.hdk.chainCode = Buffer.from(response.payload.chainCode, 'hex'); - // Determine the vendor for statistics - TrezorConnect.getFeatures() - .then((features) => { - const featuresPayload = features.success - ? features.payload - : null; - this.__setVendor(featuresPayload); - }) - .catch(() => { - this.__setVendor(null); - }) - .finally(() => { - resolve('just unlocked'); - }); + resolve('just unlocked'); } else { reject( new Error( From fe96b2f305a1d372c8e5bbef9e79a2ff52b2b2b4 Mon Sep 17 00:00:00 2001 From: Leon Date: Mon, 22 Aug 2022 10:07:59 +0800 Subject: [PATCH 10/15] Add Unit Test --- index.js | 59 +++++++++++++++-------------- test/test-eth-trezor-keyring.js | 66 +++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 30 deletions(-) diff --git a/index.js b/index.js index 8052aea..af2b6d7 100644 --- a/index.js +++ b/index.js @@ -25,6 +25,33 @@ const TREZOR_CONNECT_MANIFEST = { const oneKeySpecialVersion = 99; const oneKeyVendor = 'onekey.so'; +/** + * get the vendor name of the hardware wallet + * @param {object} features + * @returns {'onekey' | 'trezor'} + */ +const getVendorName = (features) => { + // If the value of features is null, set vendor to the default value + if (!features) { + return undefined; + } + + // No special field, default is trezor device + if (!features.minor_version || !features.patch_version) { + return 'trezor'; + } + + if ( + features.vendor === oneKeyVendor || + (features.minor_version === oneKeySpecialVersion && + features.patch_version === oneKeySpecialVersion) + ) { + return 'onekey'; + } + + return 'trezor'; +}; + function wait(ms) { return new Promise((resolve) => setTimeout(resolve, ms)); } @@ -94,7 +121,7 @@ class TrezorKeyring extends EventEmitter { /** * fetch vendor by call getFeatures * @param {object} features - * @returns {'onekey' | 'trezor'} + * @returns {'onekey' | 'trezor' | null} */ fetchVendor() { return new Promise((resolve) => { @@ -104,8 +131,7 @@ class TrezorKeyring extends EventEmitter { resolve(null); return; } - const features = response.payload; - const vendor = this.__getVendor(features); + const vendor = getVendorName(response.payload); resolve(vendor); }) .catch(() => { @@ -114,33 +140,6 @@ class TrezorKeyring extends EventEmitter { }); } - /** - * get the vendor name of the hardware wallet - * @param {object} features - * @returns {'onekey' | 'trezor'} - */ - __getVendor(features) { - // If the value of features is null, set vendor to the default value - if (!features) { - return undefined; - } - - // No special field, default is trezor device - if (!features.minor_version || !features.patch_version) { - return 'trezor'; - } - - if ( - features.vendor === oneKeyVendor || - (features.minor_version === oneKeySpecialVersion && - features.patch_version === oneKeySpecialVersion) - ) { - return 'onekey'; - } - - return 'trezor'; - } - dispose() { // This removes the Trezor Connect iframe from the DOM // This method is not well documented, but the code it calls can be seen diff --git a/test/test-eth-trezor-keyring.js b/test/test-eth-trezor-keyring.js index 538550f..94e0b5a 100644 --- a/test/test-eth-trezor-keyring.js +++ b/test/test-eth-trezor-keyring.js @@ -694,4 +694,70 @@ describe('TrezorKeyring', function () { } }); }); + + describe('getVendor', function () { + it('should call TrezorConnect.getFeatures if we dont have vendor name', async function () { + sinon + .stub(TrezorConnect, 'getFeatures') + .callsFake(() => Promise.resolve({})); + + await keyring.getVendor(); + assert(TrezorConnect.getFeatures.calledOnce); + }); + + it('should return null when getFeatures not success', async function () { + sinon + .stub(TrezorConnect, 'getFeatures') + .callsFake(() => Promise.resolve({})); + + const vendor = await keyring.getVendor(); + assert.equal(vendor, null); + }); + + it('should return trezor when getFeatures does not have minor_version or patch_version', async function () { + sinon.stub(TrezorConnect, 'getFeatures').callsFake(() => + Promise.resolve({ + success: true, + payload: { + vendor: 'trezor.io', + }, + }), + ); + + const vendor = await keyring.getVendor(); + assert.equal(vendor, 'trezor'); + }); + + it('should return onekey when minor_version and patch_version is a special version', async function () { + sinon.stub(TrezorConnect, 'getFeatures').callsFake(() => + Promise.resolve({ + success: true, + payload: { + minor_version: 99, + patch_version: 99, + vendor: 'trezor.io', + }, + }), + ); + + const vendor = await keyring.getVendor(); + assert.equal(vendor, 'onekey'); + }); + + it('should return onekey when vendor field is onekey.so', async function () { + sinon.stub(TrezorConnect, 'getFeatures').callsFake(() => + Promise.resolve({ + success: true, + payload: { + minor_version: 1, + patch_version: 1, + vendor: 'onekey.so', + }, + }), + ); + + const vendor = await keyring.getVendor(); + assert.equal(vendor, 'onekey'); + }); + }); }); From 5b458005b754efffcfb8ce31904756ef7c6979cf Mon Sep 17 00:00:00 2001 From: Leon Date: Mon, 22 Aug 2022 10:09:14 +0800 Subject: [PATCH 11/15] Use getVendorName to replace instance method --- index.js | 58 ++++++++++++++++++++++++++++---------------------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/index.js b/index.js index 575151a..af2b6d7 100644 --- a/index.js +++ b/index.js @@ -25,6 +25,33 @@ const TREZOR_CONNECT_MANIFEST = { const oneKeySpecialVersion = 99; const oneKeyVendor = 'onekey.so'; +/** + * get the vendor name of the hardware wallet + * @param {object} features + * @returns {'onekey' | 'trezor'} + */ +const getVendorName = (features) => { + // If the value of features is null, set vendor to the default value + if (!features) { + return undefined; + } + + // No special field, default is trezor device + if (!features.minor_version || !features.patch_version) { + return 'trezor'; + } + + if ( + features.vendor === oneKeyVendor || + (features.minor_version === oneKeySpecialVersion && + features.patch_version === oneKeySpecialVersion) + ) { + return 'onekey'; + } + + return 'trezor'; +}; + function wait(ms) { return new Promise((resolve) => setTimeout(resolve, ms)); } @@ -94,7 +121,7 @@ class TrezorKeyring extends EventEmitter { /** * fetch vendor by call getFeatures * @param {object} features - * @returns {'onekey' | 'trezor'} + * @returns {'onekey' | 'trezor' | null} */ fetchVendor() { return new Promise((resolve) => { @@ -104,7 +131,7 @@ class TrezorKeyring extends EventEmitter { resolve(null); return; } - const vendor = this.__getVendor(response.payload); + const vendor = getVendorName(response.payload); resolve(vendor); }) .catch(() => { @@ -113,33 +140,6 @@ class TrezorKeyring extends EventEmitter { }); } - /** - * get the vendor name of the hardware wallet - * @param {object} features - * @returns {'onekey' | 'trezor'} - */ - __getVendor(features) { - // If the value of features is null, set vendor to the default value - if (!features) { - return undefined; - } - - // No special field, default is trezor device - if (!features.minor_version || !features.patch_version) { - return 'trezor'; - } - - if ( - features.vendor === oneKeyVendor || - (features.minor_version === oneKeySpecialVersion && - features.patch_version === oneKeySpecialVersion) - ) { - return 'onekey'; - } - - return 'trezor'; - } - dispose() { // This removes the Trezor Connect iframe from the DOM // This method is not well documented, but the code it calls can be seen From d8531f1407af171f899f5a6a768303fbeb80f403 Mon Sep 17 00:00:00 2001 From: Leon Date: Mon, 22 Aug 2022 10:09:44 +0800 Subject: [PATCH 12/15] Add unit test for getVendor --- test/test-eth-trezor-keyring.js | 66 +++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/test/test-eth-trezor-keyring.js b/test/test-eth-trezor-keyring.js index 538550f..94e0b5a 100644 --- a/test/test-eth-trezor-keyring.js +++ b/test/test-eth-trezor-keyring.js @@ -694,4 +694,70 @@ describe('TrezorKeyring', function () { } }); }); + + describe('getVendor', function () { + it('should call TrezorConnect.getFeatures if we dont have vendor name', async function () { + sinon + .stub(TrezorConnect, 'getFeatures') + .callsFake(() => Promise.resolve({})); + + await keyring.getVendor(); + assert(TrezorConnect.getFeatures.calledOnce); + }); + + it('should return null when getFeatures not success', async function () { + sinon + .stub(TrezorConnect, 'getFeatures') + .callsFake(() => Promise.resolve({})); + + const vendor = await keyring.getVendor(); + assert.equal(vendor, null); + }); + + it('should return trezor when getFeatures does not have minor_version or patch_version', async function () { + sinon.stub(TrezorConnect, 'getFeatures').callsFake(() => + Promise.resolve({ + success: true, + payload: { + vendor: 'trezor.io', + }, + }), + ); + + const vendor = await keyring.getVendor(); + assert.equal(vendor, 'trezor'); + }); + + it('should return onekey when minor_version and patch_version is a special version', async function () { + sinon.stub(TrezorConnect, 'getFeatures').callsFake(() => + Promise.resolve({ + success: true, + payload: { + minor_version: 99, + patch_version: 99, + vendor: 'trezor.io', + }, + }), + ); + + const vendor = await keyring.getVendor(); + assert.equal(vendor, 'onekey'); + }); + + it('should return onekey when vendor field is onekey.so', async function () { + sinon.stub(TrezorConnect, 'getFeatures').callsFake(() => + Promise.resolve({ + success: true, + payload: { + minor_version: 1, + patch_version: 1, + vendor: 'onekey.so', + }, + }), + ); + + const vendor = await keyring.getVendor(); + assert.equal(vendor, 'onekey'); + }); + }); }); From 0ef72d54b26f33e77d374a442c00735d4f2757e6 Mon Sep 17 00:00:00 2001 From: Leon Date: Mon, 22 Aug 2022 10:41:06 +0800 Subject: [PATCH 13/15] Remove extra promise --- index.js | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/index.js b/index.js index af2b6d7..2ce4e5f 100644 --- a/index.js +++ b/index.js @@ -124,20 +124,17 @@ class TrezorKeyring extends EventEmitter { * @returns {'onekey' | 'trezor' | null} */ fetchVendor() { - return new Promise((resolve) => { - TrezorConnect.getFeatures() - .then((response) => { - if (!response.success) { - resolve(null); - return; - } - const vendor = getVendorName(response.payload); - resolve(vendor); - }) - .catch(() => { - resolve(null); - }); - }); + return TrezorConnect.getFeatures() + .then((response) => { + if (!response.success) { + return null; + } + const vendor = getVendorName(response.payload); + return vendor; + }) + .catch(() => { + return null; + }); } dispose() { From 9eebac483c4ee5c032be21a171e8fa53ac8a6cfd Mon Sep 17 00:00:00 2001 From: Leon Date: Tue, 4 Oct 2022 22:09:49 +0800 Subject: [PATCH 14/15] Update test/test-eth-trezor-keyring.js Co-authored-by: Elliot Winkler --- test/test-eth-trezor-keyring.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test-eth-trezor-keyring.js b/test/test-eth-trezor-keyring.js index 94e0b5a..fb4ea9d 100644 --- a/test/test-eth-trezor-keyring.js +++ b/test/test-eth-trezor-keyring.js @@ -705,7 +705,7 @@ describe('TrezorKeyring', function () { assert(TrezorConnect.getFeatures.calledOnce); }); - it('should return null when getFeatures not success', async function () { + it('should return null when TrezorConnect.getFeatures resolves with an object that does not have a success property', async function () { sinon .stub(TrezorConnect, 'getFeatures') .callsFake(() => Promise.resolve({})); From 6726d073355e28cc3abf21d4b8ebf13d06ea6f6b Mon Sep 17 00:00:00 2001 From: Leon Date: Tue, 4 Oct 2022 22:24:44 +0800 Subject: [PATCH 15/15] Fix review problem --- CHANGELOG.md | 3 --- index.js | 35 +++++++++++++++------------------ test/test-eth-trezor-keyring.js | 31 +++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8566f2e..bac9d59 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,9 +8,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - **BREAKING:** Removed support for Node v12 in favor of v14 ([#137](https://github.com/MetaMask/eth-json-rpc-middleware/pull/137)) -## [0.10.1] -- Support for differentiating hardware wallet vendors - ## [0.10.0] ### Added - Support for EIP-721 signTypedData_v4 ([#117](https://github.com/MetaMask/eth-trezor-keyring/pull/117)) diff --git a/index.js b/index.js index af2b6d7..673d704 100644 --- a/index.js +++ b/index.js @@ -28,9 +28,9 @@ const oneKeyVendor = 'onekey.so'; /** * get the vendor name of the hardware wallet * @param {object} features - * @returns {'onekey' | 'trezor'} + * @returns {'onekey' | 'trezor' | undefined} */ -const getVendorName = (features) => { +function getVendorName(features) { // If the value of features is null, set vendor to the default value if (!features) { return undefined; @@ -50,7 +50,7 @@ const getVendorName = (features) => { } return 'trezor'; -}; +} function wait(ms) { return new Promise((resolve) => setTimeout(resolve, ms)); @@ -112,7 +112,7 @@ class TrezorKeyring extends EventEmitter { /** * Gets the vendor, if known. * - * @returns {"trezor" | "onekey"} + * @returns {"trezor" | "onekey" | null} */ async getVendor() { return this.vendor || (this.vendor = await this.fetchVendor()); @@ -120,24 +120,21 @@ class TrezorKeyring extends EventEmitter { /** * fetch vendor by call getFeatures + * @private * @param {object} features * @returns {'onekey' | 'trezor' | null} */ - fetchVendor() { - return new Promise((resolve) => { - TrezorConnect.getFeatures() - .then((response) => { - if (!response.success) { - resolve(null); - return; - } - const vendor = getVendorName(response.payload); - resolve(vendor); - }) - .catch(() => { - resolve(null); - }); - }); + async fetchVendor() { + try { + const response = await TrezorConnect.getFeatures(); + if (!response.success) { + return null; + } + const vendor = getVendorName(response.payload); + return vendor; + } catch (err) { + return null; + } } dispose() { diff --git a/test/test-eth-trezor-keyring.js b/test/test-eth-trezor-keyring.js index 94e0b5a..492dba1 100644 --- a/test/test-eth-trezor-keyring.js +++ b/test/test-eth-trezor-keyring.js @@ -759,5 +759,36 @@ describe('TrezorKeyring', function () { const vendor = await keyring.getVendor(); assert.equal(vendor, 'onekey'); }); + + it('should return null when TrezorConnect.getFeatures rejects with an error', async function () { + sinon.stub(TrezorConnect, 'getFeatures').callsFake(() => + // eslint-disable-next-line prefer-promise-reject-errors + Promise.reject({ + success: false, + payload: { error: 'mock error', code: 'mock error code' }, + }), + ); + + const vendor = await keyring.getVendor(); + assert.equal(vendor, null); + }); + + it('should called once TrezorConnect.getFeatures function when getVendor called more then once', async function () { + sinon.stub(TrezorConnect, 'getFeatures').callsFake(() => + Promise.resolve({ + success: true, + payload: { + minor_version: 1, + patch_version: 1, + vendor: 'onekey.so', + }, + }), + ); + + for (let i = 0; i < 10; i++) { + await keyring.getVendor(); + } + assert(TrezorConnect.getFeatures.calledOnce); + }); }); });