Add wallet functions

[kantai]

This PR moves the address generation and wallet formatting logic out of blockstack-browser and into a BlockstackWallet class. The documented interface for this class defines the following functions:

class BlockstackWallet {
  static fromBase58(keychain: string): BlockstackWallet
  static fromSeedBuffer(seed: Buffer): BlockstackWallet
  getIdentitySalt(): string
  getBitcoinAddress(addressIndex: number): string
  getBitcoinPrivateKey(addressIndex: number): string
  getBitcoinPublicKeychain(): string
  getIdentityPublicKeychain(): string
  static getAddressFromBitcoinKeychain(keychainBase58: string, addressIndex: number,
                                       chainType: string = EXTERNAL_ADDRESS): string
  static getAppPrivateKey(appsNodeKey: string, salt: string, appDomain: string): string
  static getLegacyAppPrivateKey(appsNodeKey: string, salt: string, appDomain: string): string
  getIdentityKeyPair(addressIndex: number, alwaysUncompressed: ?boolean = false): IdentityKeyPair
}

This interface was designed to match the interface currently being used in the browser. We could instead try to use a slightly more consistent interface, though that would necessitate more changes in the browser.

You can test this in the browser, by linking this branch: https://github.com/blockstack/blockstack-browser/tree/feature/refactor-wallet-functions against it, and using the browser as normal.

Test this using the branch in this browser PR: stacks-archive/blockstack-browser#1496

There are also included unit tests (unitTestsWallet.js) which confirm that it generates the same data as the current version of the browser.

The getLegacyAppPrivateKey function returns an app private key based on the single hash code.
getAppPrivateKey derives a path based on all of the bytes from the sha256 hash. This slices off three bytes at a time from the hash -- resulting in a path with 11 nested children.

[kantai]

I'm going to add support for the longer derivation paths to this PR.

[larrysalibra]

I linked this to the browser branch mentioned. I was able to start the browser and run it without any noticeable issues, however, running the browser's test framework resulted in generation of an unexpected address.

 149 passing (15s)
  1 failing

  1) account-utils getIdentityOwnerAddressNode should generate app key tree:
     AssertionError: expected '17yJKABn3qphFtypDgdPfXazTWUyGSNNCF' to equal '1A9NEhnXq5jDp9BRT4DrwadRP5jbBK896X'
      at Context.<anonymous> (/Users/larry/git/blockstack-browser/tests/utils/account-utils.test.js:44:13)
  ORIGINALSRC:       assert.equal(actualAppNodeAddress, expectedAppNodeAddress)
  -------------------------^
  TRANSPILED :       assert.equal(actualAppNodeAddress, expectedAppNodeAddress);	// line 45,14
  --------------------------^
      at callFn (/Users/larry/git/blockstack-browser/node_modules/mocha/lib/runnable.js:326:21)
      at Test.Runnable.run (/Users/larry/git/blockstack-browser/node_modules/mocha/lib/runnable.js:319:7)
      at Runner.runTest (/Users/larry/git/blockstack-browser/node_modules/mocha/lib/runner.js:422:10)
      at /Users/larry/git/blockstack-browser/node_modules/mocha/lib/runner.js:528:12
      at next (/Users/larry/git/blockstack-browser/node_modules/mocha/lib/runner.js:342:14)
      at /Users/larry/git/blockstack-browser/node_modules/mocha/lib/runner.js:352:7
      at next (/Users/larry/git/blockstack-browser/node_modules/mocha/lib/runner.js:284:14)
      at Immediate.<anonymous> (/Users/larry/git/blockstack-browser/node_modules/mocha/lib/runner.js:320:5)
      at runCallback (timers.js:672:20)

Need to look into this to see what's happening

[larrysalibra]

Just noticed you marked this as a work in progress. Assume the test failure is a result of the key derivation changes that my not be complete. Will hold off on this until I get the go ahead from @kantai that it's ready to review.

[kantai]

Yep, the test failure is due to the key derivation path still being in decision limbo:

See the discussion at the bottom here:
stacks-archive/blockstack-browser#1367

This branch currently has both of the proposed paths in that issue implemented -- once we get consensus on which path we want to use, I'll update the tests and delete the other path.

[larrysalibra]

@kantai where are we on this? From the linked thread above it looks like we were benchmarking the longer derivation path. I recall a discussion from somewhere deciding that we would simply go with the longer derivation path instead of the homegrown method.

Is this something that can make it in to this sprint's release?

[kantai]

Yeah -- I think we concluded that we should (a) strongly prefer to use the longer path, unless (b) the benchmarking was really bad. The benchmarks seemed fine to me. Will update this PR to include just the longer derivation path, and then we should be able to merge this in.

[larrysalibra]

I merged in develop and refactored some of the wallet code to work with bitcoinjs-lib 4 and the now separate bip32 library. I also marked the wallet API as "private" since that removes it from our auto generated documentation which is the API we support for app developers. We'll want to support it publicly in the near future, but should improve it first instead simply transplanting it from the browser.

[larrysalibra]

@wbobeirne It would be awesome if you could take another look at this and try it out with the related browser PR
stacks-archive/blockstack-browser#1496 when you have a chance!

[wbobeirne]

Looks like a lot broke with the switch from HDNode -> BIP32. It's good you marked all of this as private APIs, because I'd really like to see us come up with a better interface than the amount of mucking around that browser will have to do with these primitives. Ideally we have a very developer friendly set of functions that do things like get addresses and such.

I'll try to fix up stacks-archive/blockstack-browser#1496 without making any more changes to this PR though, in the mean time.

[wbobeirne]

As far as I can tell, this method doesn't actually exist.

[kantai]

looks like what you want is .publicKey ?

[wbobeirne]

I've been grinding through changes between bitcoinjs-lib 3 and 4 in blockstack-browser for about an hour now, and I'm really of the mind that the approach of moving the account-utils functions as they are is a useless pursuit. I think we need to identify the things we actually want to do in browser, and define an extremely simple and limited interface from the start to do those things. We do way too many complex things related to derivation and keypairs in the browser right now, and it would be nearly impossible for someone to replicate this behavior easily.

[kantai]

I've been grinding through changes between bitcoinjs-lib 3 and 4 in blockstack-browser for about an hour now, and I'm really of the mind that the approach of moving the account-utils functions as they are is a useless pursuit.

I'm okay with that, but it is still important that we address stacks-archive/blockstack-browser#1367

[wbobeirne]

Maybe the play here is that we roll back all of my work and commit as-is. That doesn't bother me at all, my only issue is I think some of the changes Larry made to bring this up to speed with bitcoinjs-lib are still incompatible with browser. But I think if we're going to clean all of that up, it'd be better with out my added changes.