Improved Tests Phase 1: Housekeeping #250

mrnerdhair · 2021-03-23T21:09:42Z

This is the first (edit: yeah, that aged well) of a series of PRs targeting the hdwallet-native package. My end goal is to land net-new isolated key storage machinery; first, though, the tests need to be improved so that there's a baseline for evaluating whether the new stuff will break anything.

I'm planning a three-phase endeavor to achieve this:

Housekeeping: a few changes to existing code, small and manually-inspectable (this PR)
Test Improvements: new and improved tests, with no changes to non-test code or behavior
Bugfixes: changes to non-test code to fix the bugs exposed by the new tests

Some of the new tests added in phase 2 will fail because they expose underlying bugs. An alternative approach would be to introduce bugfixes and tests at the same time, so that the tests always pass; however, because of the sheer volume of new tests being added it's also valuable to be able to look at them as a unit and conclude that they can't break anything because they don't change any non-test behavior.

This first PR covers some simple, non-controversial housekeeping items (formatting, typos, a couple of logic bugs, and environment setup) in order to cleanly separate these concerns from the tests themselves.

vercel · 2021-03-23T21:09:46Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/shapeshift/hdwallet/BKRtQWFMCofFr2csZvz5zPSdAD35
✅ Preview: https://hdwallet-git-fork-reidrankin-improved-tests-housek-1219c9.vercel.app

mrnerdhair · 2021-03-23T21:11:06Z

@millsmcilroy @natven

mrnerdhair · 2021-03-23T21:12:56Z

packages/hdwallet-native/jest.config.js

+  preset: "ts-jest",
+//   testEnvironment: "node",
+  testPathIgnorePatterns: ["dist"],
+  globals: {


Using a jest.config.js instead of embedding the configuration into package.json is required to pass through Uint8Array and ArrayBuffer globals, which we need to work around jestjs/jest#4422.

mrnerdhair · 2021-03-23T21:14:38Z

packages/hdwallet-native/src/native.test.ts

@@ -1,10 +1,11 @@
-import { NativeHDWallet } from "./native";
+import * as NativeHDWallet from "./native";


Using .create() instead of the constructor 1) lets us make this import change, which will make things prettier later, and 2) tests a slightly-longer code path.

mrnerdhair · 2021-03-23T21:24:04Z

(Oh yeah, just in case I wasn't clear: all this code is ready to go. The phased PR approach is just a process thing.)

mrnerdhair · 2021-03-23T22:04:54Z

Converted to draft for a bit; want to make one more pass and make sure all the stuff that should be in here is in here

mrnerdhair · 2021-03-23T23:05:35Z

packages/hdwallet-native/src/binance.ts

        await client.initChain();

-        const addressFrom = msg.tx?.msgs?.[0]?.inputs?.[0]?.address;
-        const addressFromVerify = client.getClientKeyAddress()
+        const addressFrom = msg.tx.msgs[0].inputs[0].address;


The type specification on msg requires that this path exist, and if it doesn't a hard-fail is preferable to feeding undefined to the Binance SDK.

mrnerdhair · 2021-03-23T23:08:45Z

packages/hdwallet-native/package.json

@@ -15,6 +15,7 @@
  },
  "dependencies": {
    "@shapeshiftoss/hdwallet-core": "^1.11.1",
+    "@zxing/text-encoding": "^0.9.0",


This is an optional dependency of web-encoding, and provides TextEncoder support when it's not available natively. It's a fork of the now-unmaintained text-encoding package.

mrnerdhair · 2021-03-23T23:09:32Z

packages/hdwallet-native/src/crypto/classes/cipherString.ts

@@ -44,4 +45,13 @@ export class CipherString {
  get encryptedString() {
    return `${this.encryptionType}.${[this.data, this.iv, this.mac || ""].join("|")}`;
  }
+
+  toEncryptedObject(key: SymmetricCryptoKey): EncryptedObject {


This helper makes it easier to actually decrypt a CipherString, and will be used in tests later.

mrnerdhair · 2021-03-24T01:15:26Z

It's apparent that the binance integration tests need a little TLC before this can land. Sorry for the premature tagging; please bear with me.

mrnerdhair · 2021-03-24T03:45:32Z

integration/src/binance/tx02.mainnet.signed.json

@@ -36,7 +36,7 @@
    }
  ],
  "txid": "A640902638C0F08B6BF225D18AE9A15FD0B6DBA6C739BD3F5E390CE737BC8E75",
-  "serialized": "c001f0625dee0a482a2c87fa0a200a14ea5d7ae99a9ce2f9db7c43a907377024b7f343e812080a03424e4210e80712200a14679cd81fab41426840fa60280970513b810eb41e12080a03424e4210e80712700a26eb5ae9872102ddf08b1f51a4132c63c73455cac6569404c78c9e3b5bffd1f64e07a9a2f109b412400e7eb2814f3659e49ff228f39d968e05d1c05894e412bb193e575ca2fcf109a84ec1df6ece4b85586717e53066baa9b3f9da6ccea07f4fd92e1046f86ca64fe818c9e01c2008",
+  "serialized": "c001f0625dee0a482a2c87fa0a200a14ea5d7ae99a9ce2f9db7c43a907377024b7f343e812080a03424e4210e80712200a14679cd81fab41426840fa60280970513b810eb41e12080a03424e4210e80712700a26eb5ae9872102ddf08b1f51a4132c63c73455cac6569404c78c9e3b5bffd1f64e07a9a2f109b412406cca9c5ac3d068d2acb58a8ff3ea68007120921eaecc7f4a2c31cfde9b753d4c158cae5719a03acf6524da57ca8d91634903c6dac183f789f088385f7784c78e18c9e01c2011",


Because tests pass before and after this commit, and the signature value is not changing, this updated value must be correct.

mrnerdhair · 2021-03-24T03:47:20Z

integration/src/binance/binance.ts

@@ -65,18 +68,63 @@ export function binanceTests(get: () => { wallet: HDWallet; info: HDWalletInfo }
          tx: tx02_unsigned,
          addressNList: bip32ToAddressNList("m/44'/714'/0'/0/0"),
          chain_id: "Binance-Chain-Nile",
-          account_number: "24250",
-          sequence: 0,


Note that these values were previously being ignored due to a bug in hdwallet-native.

mrnerdhair · 2021-03-24T03:50:09Z

integration/src/binance/tx02.mainnet.signed.json

-              "denom": "BNB",
-              "amount": "1000"
+              "amount": "0.00001",
+              "denom": "BNB"


The changes in field order and data type bring the test vectors in line with the signBytes value -- the serialized data that's actually signed. (Not to be confused with the serialized transaction, which is different for reasons.)

mrnerdhair · 2021-03-24T03:52:08Z

integration/src/binance/tx02.mainnet.signed.json

    }
  ],
+  "sequence": "17",
+  "source": "0",


These values of sequence and data type match the ones actually signed -- which do not, until 2cb9480, actually come from here, but from an call out to the interwebs to check actual on-chain state of this address.

(or, at least, it's tracked as a separate network in networks.ts)

vercel bot deployed to Preview March 23, 2021 21:09 View deployment

mrnerdhair commented Mar 23, 2021

View reviewed changes

vercel bot deployed to Preview March 23, 2021 22:02 View deployment

mrnerdhair marked this pull request as draft March 23, 2021 22:04

vercel bot deployed to Preview March 23, 2021 22:40 View deployment

vercel bot deployed to Preview March 23, 2021 23:02 View deployment

mrnerdhair commented Mar 23, 2021

View reviewed changes

vercel bot deployed to Preview March 24, 2021 00:39 View deployment

vercel bot deployed to Preview March 24, 2021 03:39 View deployment

mrnerdhair commented Mar 24, 2021

View reviewed changes

vercel bot deployed to Preview March 24, 2021 03:56 View deployment

vercel bot deployed to Preview March 24, 2021 04:12 View deployment

vercel bot deployed to Preview March 24, 2021 14:57 View deployment

vercel bot deployed to Preview March 24, 2021 16:00 View deployment

vercel bot deployed to Preview March 24, 2021 16:10 View deployment

vercel bot deployed to Preview March 24, 2021 16:17 View deployment

vercel bot deployed to Preview March 24, 2021 16:28 View deployment

vercel bot deployed to Preview March 24, 2021 16:47 View deployment

vercel bot deployed to Preview March 24, 2021 16:58 View deployment

vercel bot had a problem deploying to Preview April 1, 2021 22:11 Failure

vercel bot deployed to Preview April 2, 2021 16:11 View deployment

vercel bot deployed to Preview April 2, 2021 17:05 View deployment

vercel bot deployed to Preview April 2, 2021 18:25 View deployment

elmutt approved these changes Apr 2, 2021

View reviewed changes

mrnerdhair added 13 commits April 2, 2021 14:36

binance is not cosmos

441e956

(or, at least, it's tracked as a separate network in networks.ts)

wipe FIO wallet during native wallet wipe

42423be

add .nvmrc

22f34c9

make jest work

897c656

CryptoHelper: fix test filename

8ae33b7

CryptoHelper: typo fix

aab4ff5

NativeBinanceWallet: prettier

a2d0076

NativeHDWallet: use create() when testing

35320f1

add line break

c05c39b

NativeFioWallet: enable deterministic encryption

874fd64

CipherString: add toEncryptedObject helper

c314afe

NativeFioWallet: use a prebaked polyfill for text encoding

481fb2a

version bump

0d08f15

vercel bot deployed to Preview April 2, 2021 18:37 View deployment

mrnerdhair marked this pull request as ready for review April 2, 2021 18:37

elmutt merged commit bab11e1 into shapeshift:master Apr 2, 2021

mrnerdhair deleted the improved-tests-housekeeping branch April 2, 2021 18:45

This was referenced Apr 2, 2021

Fix typo in BNB init #262

Closed

Improved Tests Phase 2: Improved Tests #271

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved Tests Phase 1: Housekeeping #250

Improved Tests Phase 1: Housekeeping #250

mrnerdhair commented Mar 23, 2021 •

edited

Loading

vercel bot commented Mar 23, 2021 •

edited

Loading

mrnerdhair commented Mar 23, 2021

mrnerdhair Mar 23, 2021

mrnerdhair Mar 23, 2021

mrnerdhair commented Mar 23, 2021

mrnerdhair commented Mar 23, 2021

mrnerdhair Mar 23, 2021

mrnerdhair Mar 23, 2021

mrnerdhair Mar 23, 2021

mrnerdhair commented Mar 24, 2021

mrnerdhair Mar 24, 2021

mrnerdhair Mar 24, 2021

mrnerdhair Mar 24, 2021

mrnerdhair Mar 24, 2021

		@@ -1,10 +1,11 @@
		import { NativeHDWallet } from "./native";
		import * as NativeHDWallet from "./native";

Improved Tests Phase 1: Housekeeping #250

Improved Tests Phase 1: Housekeeping #250

Conversation

mrnerdhair commented Mar 23, 2021 • edited Loading

vercel bot commented Mar 23, 2021 • edited Loading

mrnerdhair commented Mar 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrnerdhair commented Mar 23, 2021

mrnerdhair commented Mar 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrnerdhair commented Mar 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrnerdhair commented Mar 23, 2021 •

edited

Loading

vercel bot commented Mar 23, 2021 •

edited

Loading