gridplus / gridplus-sdk Goto Github PK

In #82 we added padding of one byte for shared secrets that had a leading zero byte lopped off by elliptic.

We should be doing it like this:

buf = Buffer.alloc(32);
secret = ecdh.derive(...)
secret.copy(buf, buf.length - secret.length)

This is more generalizable and will capture super corner cases where there is more than one byte of leading zeros (P~1/256^2).

Furthermore, I need to verify that the secrets should be left-padded as opposed to right-padded. elliptic and basically every JS util uses big endian, which means leading zeros could conceivably be dropped. But I still need to verify.

• Make more generalizable secret writer
• Verify numbers are big endian in elliptic and that it is leading zeros that get dropped

🚨 You need to enable Continuous Integration on all branches of this repository. 🚨

To enable Greenkeeper, you need to make sure that a commit status is reported on all branches. This is required by Greenkeeper because it uses your CI build statuses to figure out when to notify you about breaking changes.

Since we didn’t receive a CI status on the greenkeeper/initial branch, it’s possible that you don’t have CI set up yet. We recommend using Travis CI, but Greenkeeper will work with every other CI service as well.

If you have already set up a CI for this repository, you might need to check how it’s configured. Make sure it is set to run on all new branches. If you don’t want it to run on absolutely every branch, you can whitelist branches starting with greenkeeper/.

Once you have installed and configured CI on this repository correctly, you’ll need to re-trigger Greenkeeper’s initial pull request. To do this, please delete the greenkeeper/initial branch in this repository, and then remove and re-add this repository to the Greenkeeper App’s white list on Github. You'll find this list on your repo or organization’s settings page, under Installed GitHub Apps.

In the future we will likely make changes to the firmware message protocol, which will require a change of protocol version. We will need to respond to an "unsupported version" error from the Lattice and decrement our SDK version until we run out of versions or find one that matches the Lattice in question. We can do this via retries on _request.

We will still be using version 1 in firmware for the foreseeable future, so this is not urgent.

We have not yet documented how to use the personal_sign method and should do this.

Theoretically, I believe we should be able to contact the GCE over MQTT if both the requester (SDK) and Lattice are on the same LAN.

Currently we route all connections via MQTT using a cloud message broker. The Lattice subscribes to the router and the SDK makes request to signing.gridpl.us, which is hosting an instance of signing-api-proxy.

We may be able to build a similar adapter into the SDK and allow a user to make direct MQTT requests. The biggest unknown to explore is how to create the initial connection. The GCE hosts its own MQTT server so it's possible the SDK could connect to the Lattice, rather than the other way around.

BLOCKED BY FIRMWARE: https://github.com/GridPlus/k8x_firmware_production/issues/2024

Some contracts have functions with zero-length param names, for example see the below function coming from this contract: https://etherscan.io/address/0x11111112542d85b3ef69ae05771c2dccff4faa26#code

The 4th param has no name, but the name is not undefined so it should be allowed once it is supported in firmware.

This is a small change to ethereumAbi.js in buildAddAbiPayload:

Current:

    if (numParams > 0) {
      // First copy param names (first 20 bytes)
      def.params.forEach((param) => {
        if (!param.name || !param.latticeTypeIdx || param.isArray === undefined || param.arraySz === undefined)
          throw new Error('name, latticeTypeIdx, isArray, and arraySz must be defined for all ABI params.');
        Buffer.from(param.name).slice(0, 20).copy(b, off); off += 20;
      })

Should become:

    if (numParams > 0) {
      // First copy param names (first 20 bytes)
      def.params.forEach((param) => {
        if (param.name === undefined || !param.latticeTypeIdx || param.isArray === undefined || param.arraySz === undefined)
          throw new Error('name, latticeTypeIdx, isArray, and arraySz must be defined for all ABI params.');
        Buffer.from(param.name).slice(0, 20).copy(b, off); off += 20;
      })

The BIP purpose changes in https://github.com/GridPlus/k8x_firmware_production/pull/1650 have broken all of these SDK tests. We need to assume the following rules:

• If coin === helpers.ETH_COIN, use helpers.BTC_LEGACY_PURPOSE (i.e. 44')
• If coin === helpers.BTC_COIN || coin === helpers.BTC_TESTNET_COIN, use BTC_PURPOSE_P2SH_P2WPKH (i.e. 49')

This restriction on the BTC coin is for v1 only! In the future, we should have the BTC "type" (p2pkh, p2sh-pwpkh, or p2wpkh) as a test flag. This would give the tester flexibility on which coin to test.

NOTE: These tests broke because the Lattice now only syncs addresses BTC_PURPOSE_P2SH_P2WPKH paths. In the future, we will give users the ability to also sync legacy addresses (44') or native segwit (84') addresses as well.

The current request handling is based on a v1 spec, which will not be used in production. While the outward-facing API should remain the same, much of the backend logic needs to change.

• Remove second-layer encryption. We will either be querying an https server or using wss-based mqtt messaging to communicate with the lattice. This means we don't need two layers of ECDH encryption. Of course we will keep the inner layer that communicates with the secure enclave

• Create a "request handler" that can either be an https or mqtt client (related to #41 )

• Update each route to use the more generic handler

Once the SDK is fully caught up to production firmware, I need to go through and remove all the legacy code that is no longer being used.

This thing blows up if you try to query the agent multiple times concurrently. This is something we're blocking in the mobile wallet, but the functionality should really derive from the SDK.

I think the easiest way to do this is to have a global flag in the SDK class that gets set to true whenever a function is called and then set to false when the response is called back.

Corresponds to commit 1f0cc840a3e7b4395382478350ebdb1b2f072d5b:

~/Projects/gridplus/lib/gridplus-sdk   add-more-wallet-tests ● ⍟12  env DEVICE_ID='3RkHgk' npm run test-wallet-jobs

> [email protected] test-wallet-jobs /Users/alexmiller/Projects/gridplus/lib/gridplus-sdk
> mocha --timeout 300000 -R spec test/testWalletJobs.js --recursive --exit



  Test Wallet Jobs
    ✓ Should connect to a Lattice and make sure it is already paired. (4891ms)

  exportSeed
    ✓ Should get GP_SUCCESS for a known, connected wallet (5675ms)
    ✓ Should get GP_ENODEV for unknown (random) wallet (2678ms)

  getAddresses
    ✓ Should get GP_SUCCESS for active wallet (3233ms)
    ✓ Should get GP_EINVAL when `pathDepth` is not 4 (7619ms)
    ✓ Should get GP_ENODATA for unknown (random) wallet (2185ms)
    ✓ Should get GP_ENODATA if `first` < num cached, but the `first` + `count` - 1 address is > num cached (2170ms)
    ✓ Should get GP_ENODATA if `first` is > num cached (2184ms)
    ✓ Should get GP_EOVERFLOW if `count` exceeds the max request size (2160ms)
    ✓ Should validate first ETH (3044ms)
    ✓ Should validate BTC address (2674ms)
    ✓ Should validate first BTC change address (3319ms)

  signTx
    ✓ Should get GP_SUCCESS for active wallet (16659ms)
    ✓ Should get GP_ENODEV for unknown (random) wallet (2220ms)
    ✓ Should get GP_ENODEV for known wallet that is inactive (2522ms)
    ✓ Should get GP_EINVAL when `numRequests` is 0 (2372ms)
    ✓ Should get GP_EINVAL when `numRequests` exceeds the max allowed (7313ms)
    ✓ Should return GP_EINVAL when a signer `pathDepth` is not 5 (full BIP44 depth) (8565ms)

  Get delete permission


The following test will remove your seed.
It should be added back in a later test, but these tests could fail!
Do you want to proceed? (y/n)? y
    ✓ Should get permission to remove seed. (25239ms)
(node:91077) [DEP0106] DeprecationWarning: crypto.createDecipher is deprecated.

  deleteSeed
    ✓ Should get GP_ENODEV for unknown (random) wallet (2183ms)
    ✓ Should get GP_SUCCESS for a known, connected wallet. (19450ms)

  loadSeed
    ✓ Should get GP_EINVAL if `exportability` option is invalid (2187ms)
    ✓ Should get GP_SUCCESS when valid seed is provided to valid interface (6690ms)


Please remove your SafeCard, then re-insert and unlock it.
Wait for addresses to sync!
Press Y when the card is re-inserted and addresses have finished syncing. (y/n)? y
    ✓ Should wait for the user to remove and re-insert the card (triggering SafeCard wallet sync) (117560ms)
    ✓ Should ensure export seed matches the seed we just loaded (5791ms)
    ✓ Should get GP_EOVERFLOW if interface already has a seed (6555ms)
Please remove your SafeCard to run this test. Press Y when you have done so. (y/n)? y
    ✓ Should get GP_EAGAIN when trying to load seed into SafeCard when none exists (17050ms)


Please re-insert and unlock your SafeCard to continue.
Wait for wallet to sync.
Press Y when you have done so. (y/n)? y
    ✓ Should wait for the card to be re-inserted (32513ms)

  Wallet sync tests (starting with newly synced wallet)
    ✓ Should fail to fetch ETH address 1 (2181ms)
    ✓ Should fail to fetch BTC address 17-23 (2292ms)
    ✓ Should fail to fetch BTC address 20 (2541ms)
    ✓ Should fail to fetch BTC change address 1 (4697ms)
    ✓ Should fail to fetch BTC_TESTNET address 17-23 (2167ms)
    ✓ Should fail to fetch BTC_TESTNET address 20 (2171ms)
    ✓ Should fail to fetch BTC_TESTNET change address 1 (3289ms)
    ✓ Should fetch and validate ETH address 0 (2667ms)
    ✓ Should fetch and validate BTC addresses 10-19 (3805ms)
    ✓ Should wait while the wallet syncs new addresses (BTC->39) (40002ms)
    ✓ Should fetch and validate BTC change address 0 (2225ms)
    ✓ Should wait while the wallet syncs new addresses (BTC(change)->1) (2004ms)
    ✓ Should fetch and validate BTC_TESTNET addresses 10-19 (10410ms)
    ✓ Should wait while the wallet syncs new addresses (BTC_TESTNET->39) (40004ms)
    ✓ Should fetch and validate BTC_TESTNET change address 0 (2304ms)
    ✓ Should wait while the wallet syncs new addresses (BTC_TESTNET(change)->1) (2002ms)
    ✓ Should still fail to fetch ETH address 1 (because GAP_LIMIT=0) (4816ms)
    ✓ Should fail to fetch BTC address 40 (1800ms)
    ✓ Should fail to fetch BTC_TESTNET address 40 (1681ms)
    ✓ Should fetch and validate BTC addresses 17-23 (2723ms)
    ✓ Should wait while the wallet syncs new addresses (BTC->43) (8005ms)
    ✓ Should fail to fetch BTC address 44 (5317ms)
    ✓ Should fetch and validate BTC address 30-38 (3184ms)
    ✓ Should wait while the wallet syncs new addresses (BTC->58) (30006ms)
    ✓ Should fetch and validate BTC change address 1 (4344ms)
    ✓ Should wait while the wallet syncs new addresses (BTC(change)->2) (2003ms)
    ✓ Should fail to fetch BTC_TESTNET address 40 (5649ms)
    ✓ Should fetch and validate BTC_TESTNET addresses 17-23 (4667ms)
    ✓ Should wait while the wallet syncs new addresses (BTC_TESTNET->43) (8002ms)
    ✓ Should fail to fetch BTC_TESTNET address 44 (2678ms)
    ✓ Should fetch and validate BTC_TESTNET address 30-38 (3167ms)
    ✓ Should wait while the wallet syncs new addresses (BTC_TESTNET->58) (30004ms)
    ✓ Should fetch and validate BTC_TESTNET change address 1 (7394ms)
    ✓ Should wait while the wallet syncs new addresses (BTC_TESTNET(change)->2) (2004ms)
    ✓ Should fail to fetch BTC addresses 59-61 (2984ms)
    ✓ Should fail to fetch BTC address 59 (3299ms)
    ✓ Should fail to fetch BTC change address 3 (2544ms)
    ✓ Should fail to fetch BTC_TESTNET addresses 59-61 (3434ms)
    ✓ Should fail to fetch BTC_TESTNET address 59 (2176ms)
    ✓ Should fail to fetch BTC_TESTNET change address 3 (2620ms)
    ✓ Should fetch and validate BTC address 58 (2326ms)
    ✓ Should wait while the wallet syncs new addresses (BTC->78) (40003ms)
    ✓ Should fetch and validate BTC_TESTNET address 58 (3279ms)
    ✓ Should wait while the wallet syncs new addresses (BTC_TESTNET->78) (40004ms)
    ✓ Should fetch and validate BTC address 36-45 (4512ms)
    ✓ Should fail to fetch BTC address 70-79 (2157ms)
    ✓ Should fetch and validate BTC address 70-78 (3393ms)
    ✓ Should wait while the wallet syncs new addresses (BTC->99) (40003ms)
    ✓ Should fetch and validate BTC_TESTNET address 36-45 (3427ms)
    ✓ Should fail to fetch BTC_TESTNET address 70-79 (2170ms)
    ✓ Should fetch and validate BTC_TESTNET address 70-78 (3353ms)
    ✓ Should wait while the wallet syncs new addresses (BTC_TESTNET->99) (40002ms)
    ✓ Should fail to fetch BTC change address 3 (2276ms)
    ✓ Should fail to fetch BTC_TESTNET change address 3 (2221ms)
    ✓ Should fetch and validate BTC change address 2 (2156ms)
    ✓ Should wait while the wallet syncs new addresses (BTC(change)->3) (2004ms)
    ✓ Should fetch and validate BTC change address 3 (4168ms)
    ✓ Should wait while the wallet syncs new addresses (BTC(change)->4) (2002ms)
    ✓ Should fetch and validate BTC_TESTNET change address 2 (4988ms)
    ✓ Should wait while the wallet syncs new addresses (BTC_TESTNET(change)->3) (2004ms)
    ✓ Should fetch and validate BTC_TESTNET change address 3 (4311ms)
    ✓ Should wait while the wallet syncs new addresses (BTC_TESTNET(change)->3) (2003ms)
    ✓ Should fetch and validate BTC address 0-9 (6951ms)
    ✓ Should fetch and validate BTC change address 0 (2874ms)
    ✓ Should fetch and validate BTC_TESTNET address 0-9 (3330ms)
    ✓ Should fetch and validate BTC_TESTNET address 0 (2887ms)


  94 passing (16m)

The SDK should fail and return an error instead of passing null values to the device for signing.

The most obvious case of this is in building an Ethereum transaction - the user should pass null for the nonce value in the transaction and the SDK should be able to fill that value in based on the sender. However, if no sender is present, the SDK continues with nonce=null. This should not happen and an error should be triggered instead.

There is a similar need in the SDK as there is in the firmware described by this issue:
https://github.com/GridPlus/k8x_firmware_production/issues/1691

In #213 I got rid of the full, bloaty ethers module in favor of smaller packages that we actually use. Unfortunately, I have found that ethers-eip712, which we need for TypeData messages, requires the full ethers module as a dependency, so this SDK will break if that is not included.

I have looked at several EIP712 libs but none of them work for the test payloads so I can't use any of them right now. It isn't worth the trouble of fixing those other modules, so I will include ethers back in for now and come back to this.

The best option I found is a module called eip-712. It failed on several payloads, but it looked like only two types of failures:

• 'Should test a large 1inch transaction' threw some error internal to eip-712: AssertionError: expected 'Failed to make request to device: data.startsWith is not a function' to equal null
• 'Should test canonical EIP712 example with 3rd level nesting' simply calculates the wrong hash. I opened an issue pointing this out. I looked at the code and it is not obvious what the issue is except to say that it is failing to build the final nested object into the definition. The encodeType function in ethers-eip712 (here) is much more complex than the one in eip-712 (here) so I think that code would probably just need to get replicated. Unfortunately that isn't the only problem so I can't just fork it and merge that in.

We should aim to get rid of ethers-eip712 once we find a slimmer module that works with all the payloads we need.

I have started a branch to show what this swap could look like: https://github.com/GridPlus/gridplus-sdk/tree/fix-ethers-again

The SDK should allow the user to not specify a secret for pairing and should generate one on the backend.

We will use a subset of the dictionary, so it's better not to push that logic onto the user.

It'd be nice to be able to fetch (and perhaps update) the list of third-party connections, like those displayed on the Permissions screen on the Lattice.

It seems that when pinging the Lattice repeatedly, people are sometimes seeing isPaired = undefined. The only place where isPaired is getting set to a variable value is in connect:

this.isPaired = this._handleConnect(res);

I'm honestly not sure how _handleConnect could be returning undefined:

  _handleConnect(res) {
    let off = 0;
    const pairingStatus = res.readUInt8(off); off++;
    // If we are already paired, we get the next ephemeral key
    const pub = res.slice(off, off + 65).toString('hex'); off += 65;
    // Grab the firmware version (will be 0-length for older fw versions)
    // It is of format |fix|minor|major|reserved|
    this.fwVersion = res.slice(off, off + 4);
    // Set the public key
    this.ephemeralPub = getP256KeyPairFromPub(pub);
    // return the state of our pairing
    return (pairingStatus === messageConstants.PAIRED);
  }

The easy fix here is to just change the first line to:

this.isPaired = this._handleConnect(res) || false;

But I'd really like to know how isPaired could be getting set to undefined here...

Per security warning:

Upgrade elliptic to version 6.5.4 or later.

We should tell people how to load a custom set of ABI definitions since we restrict what can be added on the web wallet side.

I get failed to make request to device error in the console when trying to create a wallet on https://gnosis-safe.io/app/#/welcome

Steps to reproduce:

1. Connect lattice wallet
2. Click "create new safe"
3. Follow the flow
4. Approve transaction on the device

Expected outcome:
The transaction goes through, a wallet is created

Obtained outcome:
Transaction fails

Software versions:
GCE: 0.48.12
bootloader: 0.3.1
HSM: 0.11.0

It seems we reject pairing requests where the requester name is <5 characters in firmware. If that is the case, we should put an appropriate error message in the SDK to block the request entirely

When evaluating a signPersonal request, we currently check if the message is valid ASCII. This means that if someone tries to send a special UTF8 character, we evaluate that it is not ASCII and instead display a hex string representation.

We should instead look for non-ASCII UTF8 characters and do one of the following:

• Reject the payload at the SDK level with the error indicating that we cannot display non-ASCII characters right now (e.g. no emojis)
• Accept the payload and display an unsupported character on the Lattice side. This would involve firmware work, so I am leaning against doing it this way.

Either way, there will likely be requests with emojis or other UTF8 characters and we should not be displaying those as hex.

I testnet was not enabled by the user under Settings->Currencies->Bitcoin, the main test will fail at the very beginning in the "Should get addresses" step (and on all BTC tests as well). There is no need to run all tests if this situation is detected, so the test should abort early with a message asking to enable testnet.

I tried to make a limit order on Opyn but got an error message saying "invalid signature length". This was an ETH_MSG request which went through to my Lattice and was signed. I'll need to look at the integration.

With every request, there seems to be a small probability that we will encounter an error in elliptic when trying to decrypt a Lattice response with the shared secret (I believe the error is actually deriving the new shared secret using the new ephemeral pubkey).

This error gets bubbled up to the user and should really be handled by the SDK. This is the proposed mechanism:

1. Cache the request when it is made (specifically for getAddresses, getWallets, and sign). This can be made into a state variable in the SDK session object. We should cache the raw payload and message type so we can dispatch the message again when we get a new ephem pubkey.
2. If we catch an error when deriving the new shared secret, call connect to get a new ephemeral pubkey. If for some reason we can't handle that, keep calling connect (it is very unlikely the same problem will occur twice in a row).
3. After getting a new ephemeral pub, resubmit the request using the data from 1.

See: https://github.com/GridPlus/k8x_firmware_production/issues/2247

Cannot use lattice wallet on rinkeby dapp: https://rinkeby.gnosis-safe.io/app/#/welcome

It opens a dev Heroku lattice wallet which timeouts when trying to connect to the wallet, the same dapp deployed on mainnet works fine

Per the new spec, the SDK now has to request a walletUID from the device. This will return an array of form:

[
  [ walletUID (u32), isPresent (bool), name (char[20]) ],
   ...
]

In order to communicate with the device (getAddresses, sign) in the established session, we need to include the walletUID of the wallet where isPresent=true.

Getting the UID when starting the SDK session

The easiest way to do this is to make the getWallets request if:

1. We connect and find we are paired
2. We finalize a pairing

At this point, the walletUID can be saved to state.

Switching active UID when the user inserts/removes a card

If the user inserts/removes a card, the isPresent flag will change and we will have to do getWallets again. Note that we can detect this condition if the error response code is 0x90.

This condition should be caught when we make any request and find the response code = 0x90.

In #111 we introduced a mechanism to default to UTF8 character display for personalSign messages (rather than defaulting to hex strings). Unfortunately, the Lattice cannot currently display non-ASCII UTF8 characters and the display simply skips those characters, which is no bueno for a "secure" screen.

Unfortunately, I think the best strategy here is to throw an error for non-ascii UTF8 strings. This is not a full reversion of #111 -- it simply adds a restriction on messages.

When calling refreshWallets after a disconnect, it continues to throw an error (Error getting active wallet.). Similarly, it does not appear to detect when the device has disconnected. The underlying functionality seems entirely broken.

The MetaMask fork allows users to specify different derivation path indexes (standard, Ledger Live, Ledger Legacy). As it were, the getAddresses call instructs the hardware to increment n number of addresses at the last path index for standard derivation only, and any other path index requires a client-specific implementation (see gridplus/eth-lattice-keyring) to enumerate n calls on getAddresses.

As the number of clients integrating with Lattice1 grows, it's worth considering allowing getAddresses to increment a non-standard path index, making it easier for feature-parity across clients.

When running a test, it seems that when a timeout is reached, it still tries to go to the end running every step even though it will fail all of them. Timeouts should abort the execution early, or otherwise they should be "huge" to avoid these issues under slow connections, etc.

We should use the WalletConnect API to make our connectivity more standardized. Not quite sure how this should work given our interactive, out-of-band secret exchange - will need to investigate once v1 is released

All wallet_jobs passing as of commit 89ade0f6c0f9fac781623a39ad8c13d8158c1d55:

 ~/Projects/gridplus/lib/gridplus-sdk   add-address-tests ● ⍟12  env DEVICE_ID='3RkHgk' npm run test-wallet-jobs

> [email protected] test-wallet-jobs /Users/alexmiller/Projects/gridplus/lib/gridplus-sdk
> mocha --timeout 180000 -R spec test/testWalletJobs.js --recursive --exit



  Test Wallet Jobs
    ✓ Should connect to a Lattice and make sure it is already paired. (5221ms)

  exportSeed
    ✓ Should get GP_SUCCESS for a known, connected wallet (11761ms)
    ✓ Should get GP_ENODEV for unknown (random) wallet (2670ms)

  getAddresses
    ✓ Should get GP_SUCCESS for active wallet (3178ms)
    ✓ Should get GP_EINVAL when `pathDepth` is not 4 (8181ms)
    ✓ Should get GP_ENODATA for unknown (random) wallet (4148ms)
    ✓ Should get GP_ENODATA if `first` < num cached, but the `first` + `count` - 1 address is > num cached (2150ms)
    ✓ Should get GP_ENODATA if `first` is > num cached (8411ms)
    ✓ Should get GP_EOVERFLOW if `count` exceeds the max request size (4836ms)
    ✓ Should validate first ETH (4835ms)
    ✓ Should validate BTC addresses (4931ms)
    ✓ Should validate first BTC_CHANGE address (16034ms)

  signTx
    ✓ Should get GP_SUCCESS for active wallet (8231ms)
    ✓ Should get GP_ENODEV for unknown (random) wallet (2699ms)
    ✓ Should get GP_ENODEV for known wallet that is inactive (5667ms)
    ✓ Should get GP_EINVAL when `numRequests` is 0 (2658ms)
    ✓ Should get GP_EINVAL when `numRequests` exceeds the max allowed (8159ms)
    ✓ Should return GP_EINVAL when a signer `pathDepth` is not 5 (full BIP44 depth) (10978ms)

  Get delete permission


The following test will remove your seed.
It should be added back in a later test, but these tests could fail!
Do you want to proceed? (y/n)? y
    ✓ Should get permission to remove seed. (5972ms)
(node:26063) [DEP0106] DeprecationWarning: crypto.createDecipher is deprecated.

  deleteSeed
    ✓ Should get GP_ENODEV for unknown (random) wallet (3996ms)
    ✓ Should get GP_SUCCESS for a known, connected wallet. (17702ms)

  loadSeed
    ✓ Should get GP_EINVAL if `exportability` option is invalid (4823ms)
    ✓ Should get GP_SUCCESS when valid seed is provided to valid interface (8268ms)


Please remove your SafeCard, then re-insert and unlock it.
Wait for addresses to sync!
Press Y when the card is re-inserted and addresses have finished syncing. (y/n)? y
    ✓ Should wait for the user to remove and re-insert the card (120728ms)
    ✓ Should ensure export seed matches the seed we just loaded (6534ms)
    ✓ Should get GP_EOVERFLOW if interface already has a seed (6189ms)
Please remove your SafeCard to run this last test. Press Y when you have done so. (y/n)? y
    ✓ Should get GP_EAGAIN when trying to load seed into SafeCard when none exists (12432ms)


  27 passing (5m)

All wallet_jobs passing as of commit dfaed269584d5d46a73d6487291728bc797b164c

 ~/Projects/gridplus/lib/gridplus-sdk   master ⍟12  env DEVICE_ID='3RkHgk' npm run test-wallet-jobs

> [email protected] test-wallet-jobs /Users/alexmiller/Projects/gridplus/lib/gridplus-sdk
> mocha --timeout 180000 -R spec test/testWalletJobs.js --recursive --exit



  Test Wallet Jobs
    ✓ Should connect to a Lattice and make sure it is already paired. (4443ms)

  exportSeed
    ✓ Should get GP_SUCCESS for a known, connected wallet (6179ms)
    ✓ Should get GP_ENODEV for unknown (random) wallet (2203ms)

  getAddresses
    ✓ Should get GP_SUCCESS for active wallet (2750ms)
    ✓ Should get GP_EINVAL when `pathDepth` is not 4 (8379ms)
    ✓ Should get GP_ENODATA for unknown (random) wallet (11007ms)
    ✓ Should get GP_ENODATA if `first` < num cached, but the `first` + `count` - 1 address is > num cached (2157ms)
    ✓ Should get GP_ENODATA if `first` is > num cached (2669ms)
    ✓ Should get GP_EOVERFLOW if `count` exceeds the max request size (2667ms)

  signTx
    ✓ Should get GP_SUCCESS for active wallet (8194ms)
    ✓ Should get GP_ENODEV for unknown (random) wallet (2678ms)
    ✓ Should get GP_ENODEV for known wallet that is inactive (2663ms)
    ✓ Should get GP_EINVAL when `numRequests` is 0 (2677ms)
    ✓ Should get GP_EINVAL when `numRequests` exceeds the max allowed (8563ms)
    ✓ Should return GP_EINVAL when a signer `pathDepth` is not 5 (full BIP44 depth) (10689ms)

  Get delete permission


The following test will remove your seed.
It should be added back in a later test, but these tests could fail!
Do you want to proceed? (y/n)? y
    ✓ Should get permission to remove seed. (5203ms)
(node:43207) [DEP0106] DeprecationWarning: crypto.createDecipher is deprecated.

  deleteSeed
    ✓ Should get GP_ENODEV for unknown (random) wallet (3191ms)
    ✓ Should get GP_SUCCESS for a known, connected wallet. (13186ms)

  loadSeed
    ✓ Should get GP_EINVAL if `exportability` option is invalid (2890ms)
    ✓ Should get GP_SUCCESS when valid seed is provided to valid interface (7185ms)


Please remove your SafeCard, then re-insert and unlock it.
Wait for addresses to sync!
Press Y when the card is re-inserted and addresses have finished syncing. (y/n)? y
    ✓ Should wait for the user to remove and re-insert the card (107979ms)
    ✓ Should ensure export seed matches the seed we just loaded (6236ms)
    ✓ Should get GP_EOVERFLOW if interface already has a seed (6810ms)
Please remove your SafeCard to run this last test. Press Y when you have done so. (y/n)? y
    ✓ Should get GP_EAGAIN when trying to load seed into SafeCard when none exists (9627ms)


  24 passing (4m)

I am trying to sign the following personal_sign message from GodsUnchained:

Message:
Only sign this request if you’ve initiated an action with Immutable X.

Obviously this is an ASCII string and should be displayed as such on the Lattice, but it is displaying hex. Here is the logic to build that in the SDK:

 let displayHex = false;
    if (typeof input.payload === 'string') {
      if (input.payload.slice(0, 2) === '0x') {
        payload = ensureHexBuffer(input.payload)
        displayHex = true === isHexStr(input.payload.slice(2));
      } else {
        if (false === latticeCanDisplayStr(input.payload))
          throw new Error('Currently, the Lattice can only display ASCII strings.');
        payload = Buffer.from(input.payload)
      }
    } else if (typeof input.displayHex === 'boolean') {
      // If this is a buffer and the user has specified whether or not this
      // is a hex buffer with the optional argument, write that
      displayHex = input.displayHex
    }

I would need to validate this exact payload but my guess is that it's coming in as a buffer. We should add a mechanism to convert the buffer to a string and validate if that string is ASCII, then determine displayHex from that.

Currently, connect takes a serial string and uses that to query our routing server.

Users should be able to establish a direct connection either over LAN or with a port open on the device.

connect should be updated to accept either a serial string or a direct IP/port. If the argument contains periods, an mqtt client would be created to query the device directly. If the argument does not contain periods, we would use the proxy server. This, obviously, means serial strings should never have periods.

This code was introduced in Lattice firmware v0.13.0 and was intended to facilitate a more intelligent retry mechanism. However, the current retry logic is incorrect:

RESP_ERR_INVALID_EPHEM_ID gets thrown in firmware when the ephemeral ID (a rolling, random key used to encrypt individual requests) cannot be used to find a requester: https://github.com/GridPlus/k8x_firmware_production/blob/dev/lattice_firmware/src/signing/pairing_manager.c#L1465

We should be able to simply call connect and the response data should contain a new ephemeral ID (it’s a pubkey), which gets set for the next request in the connect callback: https://github.com/GridPlus/gridplus-sdk/blob/dev/src/client.js#L698

But I just opened my MM (running all latest software) and tried to send a transaction request to my Lattice (running v0.13.1), and I got this error (which maps to the error code I am talking about here):

Error from device: Could not find requester. Please reconnect.

So what’s the issue? Well, we are using the original data, which is an encrypted payload containing the original ephemeral ID… so we need to decode it and swap the key. Or else we should pass in unencrypted data to _request

The provider-related functionality is not really necessary for this SDK, as it is meant to interface with the Lattice. Removing it would lead to a much simpler design. Users can still use whatever providers they want in their application without having those APIs integrated into the Lattice's SDK.

Refactor the Client's public functions to return Promises so the frontend can do better error handling.

I've had problems in the past with elliptic and React Native, especially around signing, though a solution appears to exist.

We should make sure all elliptic functionality existing in the SDK works with RN.

We now get superagent errors thrown specifically for ETH requests using node v14. This was fine for v13 and previous.

  Connect and Pair
Please enter the ID of your test device: XXXXX
    ✓ Should connect to a Lattice (8005ms)
Please enter the pairing secret: Q191HN8V
    ✓ Should attempt to pair with pairing secret (10971ms)
    ✓ Should try to connect again but recognize the pairing already exists (3856ms)
    ✓ Should get addresses (20718ms)
    1) Should sign Ethereum transactions
    ✓ Should sign legacy Bitcoin inputs (10260ms)
    ✓ Should sign segwit Bitcoin inputs (12247ms)


  6 passing (1m)
  1 failing

  1) Connect and Pair
       Should sign Ethereum transactions:
     Error: the string "Failed to make request to device." was thrown, throw an Error :)
      at processTicksAndRejections (internal/process/task_queues.js:93:5)

We are currently returning a single response object in all routes. This means that if there is an error, the response object looks like { err: "some error" }.

It's probably better to return an error as the first argument, followed by response data (if applicable). This way the user doesn't need to fish into the response object for an error. It is also more standard across node.js APIs

this._getEphemId() sometimes returns null. This should not be possible.

The issue is inside _buildEncRequest.

I am somehow able to call this function, which should not be the case if this._getEphemId() returns null.

If we return the added records with ids, then the consuming project can combine the added records to the existing list without needing to refetch.

Without ids, it's not possible to use the other functions of the client on those records because they require the id.

I'd never seen signTypedData_v1 used in the wild until today: https://lostpoets.xyz/

Here is the message:

[
  {"type":"string","name":"banner","value":"Please sign this transaction to log in"}, 
  {"type":"string","name":"challenge","value":"6b7c02a376bfb77fe1f55e1671083c0e"}
]

We should look into supporting this. It will require changes at both the SDK and firmware levels.

Making a list of problems with documentation that need to be updated:

• test3 needs to be replaced with testnet. Verify that the whole section of network options is correct.
• sign() - make sure it is clear that Ethereum sender and accountIndex must be a string and int, respectively (NOT arrays). We should also probably rename accountIndex -> senderIndices
• For Bitcoin transactions, the user currently needs to include { addresses: <array of addrs> } as the third argument to sdk.getTx or it will not return the right formatted txs. Need to fix this in both the docs (mention opts argument) and the SDK itself (format all transactions properly)

When transaction data is too large it fails to bubble the error up to the user and instead the user sees Object object. This is an easy fix which is already done in a working branch (for tuple support). Just tracking it here so it doesn't get lost.