Typescript rewrite

[the-jackalope]

Resolves #58

Possible point of discussion:

• I used CommonJS style exports. This maintains the way that the compiled files can be imported into regular JS (const Wallet = require('ethereumjs-wallet')) but will make it slightly clunky for TypeScript users (import Wallet = require('ethereumjs-wallet'). Couldn't find an official ethereumjs style guide on this issue. If you're unfamiliar with TS module resolution this is a good resource: https://www.typescriptlang.org/docs/handbook/modules.html

[holgerd77]

Hi @the-jackalope, this is really great 😄! @s1na can you eventually do a review on this?

[s1na]

@holgerd77 I'll have a look sure, but this is my first interaction with this library and I'd appreciate a second set of eyes.

[s1na]

Thanks for the effort you put in the PR. It already looks quite good. I had a few questions/comments, but I don't really have a strong opinion on all of them and encourage others to also chime in.

About the module style, in the other repos we're using the proper import X from './file' and export function test and export default class... syntax instead of export = and import = . As you mentioned this would mean users would have to change how they import the module, but that's okay. We can do a major release.

Update: I haven't checked the tests, and haven't run it locally. That has to be done after this first review round.

[the-jackalope]

One extra thing that surfaced from @s1na 's review: the JSON that thetoV3 method generates doesn't match the official documentation for V3 keystore. ethereumjs-wallet was including an address key which the documentation says is a security risk.

I'm planning on removing the address key and updating the tests to match the official spec in my next commit, unless there's a good reason to keep it as-is

[holgerd77]

@the-jackalope this is a pretty significant change, can we address this in a separate PR after this one is merged so it won't get lost?

[the-jackalope]

@s1na : I've implemented your PR changes so whenever you have time for another review that would be great - thank you!

I filed an issue for the address key on the V3 JSON here.

[s1na]

Thanks for applying the changes. It looks much better. Some more nitpicks :)

[the-jackalope]

Cleaned up the KDF stuff a little, sorry it's late. Work's been busy

[holgerd77]

@s1na Can you have another look here? All "nitpicks" addressed 😄?

[s1na]

Looks good to me thanks.

I'd like to re-iterate that this is the first time I'm looking at this library, so if someone else could also have a look to make sure I haven't missed anything it'd be appreciated.

[alcuadrado]

Much better than assert :)

[alcuadrado]

Just a note: This function's body can be replaced with:

return {
  ...v3Defaults,
  ...params
}

And given that it is so small and only used once, it could be inlined.

[alcuadrado]

Same with this function

[alcuadrado]

I have really little context about this library, but why are this and the following method private?

[alcuadrado]

Great work @the-jackalope! Thanks! I left a few comments, but they are not blockers.

I don't have any previous experience with this library, but I reviewed the changes very carefully and I believe that this PR maintains the previous behavior of the library.

[holgerd77]

Will merge here, comments from @the-jackalope can also be addressed in separate PRs, @the-jackalope feel free, would be great, generally again thanks for this extensive work here!

[holgerd77]

I am getting the following errors when trying to build this with freshly installed dependencies and the dist folder deleted to avoid side effects:

+ exec tsc -p ./tsconfig.prod.json
src/index.ts:2:26 - error TS7016: Could not find a declaration file for module 'ethereumjs-util'. '/ethereumjs-wallet/node_modules/ethereumjs-util/dist/index.js' implicitly has an 'any' type.
  Try `npm install @types/ethereumjs-util` if it exists or add a new declaration (.d.ts) file containing `declare module 'ethereumjs-util';`

2 import * as ethUtil from 'ethereumjs-util'
                           ~~~~~~~~~~~~~~~~~

src/index.ts:352:5 - error TS2322: Type 'Buffer | undefined' is not assignable to type 'Buffer'.
  Type 'undefined' is not assignable to type 'Buffer'.

352     return this.publicKey
        ~~~~~~~~~~~~~~~~~~~~~

src/thirdparty.ts:2:26 - error TS7016: Could not find a declaration file for module 'ethereumjs-util'. '/ethereumjs-wallet/node_modules/ethereumjs-util/dist/index.js' implicitly has an 'any' type.
  Try `npm install @types/ethereumjs-util` if it exists or add a new declaration (.d.ts) file containing `declare module 'ethereumjs-util';`

2 import * as ethUtil from 'ethereumjs-util'
                           ~~~~~~~~~~~~~~~~~


Found 3 errors.

[s1na]

ethereumjs-util should be updated to v6.1.0. Strange how tests passed if build is failing 🤔

[alcuadrado]

I think I mentioned this in another PR, but I suspect it's because of an outdated package-lock.json.

[holgerd77]

@alcuadrado Yes, that was it actually (likely also in the other PR), thanks for pointing this out, completely didn't have this on the radar as a potential cause of failure.

So think we can do a release here in the upcoming days. 😀

[alcuadrado]

Glat to see you solved it :)

I would be great if we can include #95 in that release.