Update dependencies

[paulmillr]

1. Replaced aes-js with ethereum-cryptography/aes, which defers to browser/node.js native implementation. New AES is always async, which changes signature methods
2. Replaced bs58check with @scure/base which is a dependency of ethereum-cryptography
3. Updated ethereumjs-util to @ethereumjs/[email protected]
4. Replaced utf8 with a simple utilify function inside ethereum-cryptography/utils
5. Replaced randombytes with ethereum-cryptography/random
6. Made devDependency versions exact, instead of version ranges: safer for dev environments
7. Replaced scrypt-js with ethereum-cryptography/scrypt. NOTE: tests were failing after that because EC's scrypt conforms to RFC7914 section 2, which prohibits N: 262144, p: 8, r: 1. The old version did not conform to spec. My guess is that someone at some point messed up the tests and passed p: 8, r: 1 instead of r: 8, p: 1, which is standard. I've commented them out with it.skip
8. KryptoKit is a mess: it uses AES-ECB & MD5 which are bad, and are not implemented in ethereum-cryptography. If we can remove it, that'll ditch crypto-browserify and js-md5

Backwards incompatible changes:

• src/index.ts: Wallet.fromEthSale => async Wallet.fromEthSale
• src/thirdparty.ts: Thirdparty.fromEtherWallet => async Thirdparty.fromEtherWallet
• (if we can make it) src/thirdparty.ts: removed KryptoKit.fromKryptoKit
• Bigints requirement

Notes:

• Should the new version of pkg be named @ethereumjs/wallet? Should the old repo be deprecated?
• @ethereumjs/config-typescript is still the old one; I guess if the repo was inside monorepo, that wouldn't happen
• node_modules/.bin/ethereumjs-config-lint: line 26: realpath: command not found - the issue is messy

[acolytec3]

Added a commit to deal with the linting dependencies and such. That should resolve bullets 2-3 of your notes.

[acolytec3]

@holgerd77 in order to merge this, we'll need to revise the required checks. Since Node 12/13 are past EOL and we don't support them in the monorepo, makes sense to drop support here. I've removed them from the CI so it this PR will need to be admin merged.

[paulmillr]

@acolytec3 what about node 18?

[acolytec3]

@acolytec3 what about node 18?

Just added it. Once @holgerd77 is back, he can approve and merge. We'll need to post a v2 anyway and update the changelog given the breaking changes.

[paulmillr]

RE: KryptoKit: i'm strongly in favor of removing it. Reasons:

We'd have to do some more research with downstream library consumers since a cursory search on github indicates a number of teams have it in code

GitHub search for fromKryptoKit displays compiled files based on the repo or copies of this repo. I don't see any usage of it. Also, we could remove with the version bump; then, if anybody complains, we could bring it back. Also, old versions would still support it.

For example, this is a first time i'm hearing of it. And the method was in for a long, long time.

[holgerd77]

When thinking a bit about it and also after the pushes from @acolytec3 I would also be a fan of a) updating the library name to the scoped version with @ethereumjs/wallet and b) also bringing this to the monorepo. Will talk a bit more about this later today in our call. Paul, if you want to (at least partly) join that would be great, we'll start at 5:30pm Berlin time.

[paulmillr]

Yeah, i'll try. Send me a meeting url in discord please

[paulmillr]

Updated.

The decision that needs to be done:

To remove, or to not remove KryptoKit. Pros: it will remove crypto dependency which is huge; but only used for createDecipheriv. Cons: backwards compat, but it's barely used anywhere.

[paulmillr]

Does anyone have any idea why firefox headless is being ran in a web worker? That causes noble-hashes check for instanceof Hash to fail.

To fix this, we either need to patch noble-hashes, or make browser tests run w/o web worker

[holgerd77]

To give you an update here: I guess I won't be able to prioritize Wallet integration before we have the final monorepo releases out, since there is so much (new) discussion going on and we also need to close on this front.

To have Wallet released under a new namespace and monorepo-integrated would be somewhat nice but it also technically not necessary. So we will very likely do this some time after the main monorepo releases.

[holgerd77]

Sorry that this had to wait for so long, will try to pick this up again early next week 🙂, everyone else invited to have a look as well!

[holgerd77]

Maybe some preparatory questions: @paulmillr can you give some additional context on this KryptoKit thing? What is this? What is the role / level of significance here (maybe optimally with some history: when has this been used for what? did something change?)? Do you have some links?

[paulmillr]

What is this? What is the role / level of significance here (maybe optimally with some history: when has this been used for what? did something change?)? Do you have some links?

Google says it's some old obscure company from 2014-2016. I can't find modern references to it. Github search does not give anything also. So my guess is that the vendor added their stuff at some point, but it hasn't been used ever since. Seems safe to remove.

[holgerd77]

What is this? What is the role / level of significance here (maybe optimally with some history: when has this been used for what? did something change?)? Do you have some links?

Google says it's some old obscure company from 2014-2016. I can't find modern references to it. Github search does not give anything also. So my guess is that the vendor added their stuff at some point, but it hasn't been used ever since. Seems safe to remove.

Ah ok, thanks for the research, I would agree, then it seems very well safe to remove. 👍

[paulmillr]

removed kryptokit, which removes dependency on crypto-browserify. js-md5 seems to still be used for other stuff.

[holgerd77]

Update: ah, and I guess I really want to have all the tests working before we merge anything here, no matter on the explanation why tests eventually fail. Then we have to find some other solution (adopt a test, if necessary skip a test,...).

[paulmillr]

How to force the CI run?

[holgerd77]

Ugh. I've now idea why this is not starting? 🤔

[acolytec3]

Looks like browser tests are failing, specifically this one. @paulmillr any ideas?

Aside from that, why are the tests skipped for scrypt with unencrypted wallets? Is that an invalid test at this point?

[paulmillr]

See point 7 for scrypt. The tests are invalid, but should be replaced with inversed r/p.

How do test failures look like, what is the output? A bit hard to see from here.

[acolytec3]

On the browser tests, near as I can tell, the mac value being computed is different than the expected result.

        + { version: 3,
        - { address: 'b14ab53e38da1c172f877dbc6d65e4a1b0474c3c',
            id: 'ffffffff-ffff-4fff-bfff-ffffffffffff',
        +   address: 'b14ab53e38da1c172f877dbc6d65e4a1b0474c3c',
        -   version: 3,
            crypto: 
            crypto: 
        +    { ciphertext: '15e356c67d266d3ca85cff4f6d92d117b636b68ad6172c858348af44ede45ac4',
        -    { cipher: 'aes-128-ctr',
               cipherparams: { iv: 'ffffffffffffffffffffffffffffffff' },
               cipherparams: { iv: 'ffffffffffffffffffffffffffffffff' },
        +      cipher: 'aes-128-ctr',
        -      ciphertext: '15e356c67d266d3ca85cff4f6d92d11720aae32cdd28d5d9885836dacb1d213b',
               kdf: 'scrypt',
               kdf: 'scrypt',
        +      kdfparams: { dklen: 32, salt: '', n: 262144, p: 1, r: 8 },
        +      mac: '6b011abb72c222dfc2d426aa5ec60adb0d4934bab36fe0db171361b0a3a9748c' } }
        -      kdfparams: { salt: '', n: 262144, dklen: 32, p: 1, r: 8 },
        -      mac: 'aef950dfea8548985b4600ca87ff9cd3ae1f34b793864ab7d099f572719cb79a' } }

It was expecting aef950dfea8548985b4600ca87ff9cd3ae1f34b793864ab7d099f572719cb79a and got 86b011abb72c222dfc2d426aa5ec60adb0d4934bab36fe0db171361b0a3a9748c*

[azerella]

Any updates on merging here? This library is my only blocker with upgrading to Node 18

[acolytec3]

On the browser tests, near as I can tell, the mac value being computed is different than the expected result.

        + { version: 3,
        - { address: 'b14ab53e38da1c172f877dbc6d65e4a1b0474c3c',
            id: 'ffffffff-ffff-4fff-bfff-ffffffffffff',
        +   address: 'b14ab53e38da1c172f877dbc6d65e4a1b0474c3c',
        -   version: 3,
            crypto: 
            crypto: 
        +    { ciphertext: '15e356c67d266d3ca85cff4f6d92d117b636b68ad6172c858348af44ede45ac4',
        -    { cipher: 'aes-128-ctr',
               cipherparams: { iv: 'ffffffffffffffffffffffffffffffff' },
               cipherparams: { iv: 'ffffffffffffffffffffffffffffffff' },
        +      cipher: 'aes-128-ctr',
        -      ciphertext: '15e356c67d266d3ca85cff4f6d92d11720aae32cdd28d5d9885836dacb1d213b',
               kdf: 'scrypt',
               kdf: 'scrypt',
        +      kdfparams: { dklen: 32, salt: '', n: 262144, p: 1, r: 8 },
        +      mac: '6b011abb72c222dfc2d426aa5ec60adb0d4934bab36fe0db171361b0a3a9748c' } }
        -      kdfparams: { salt: '', n: 262144, dklen: 32, p: 1, r: 8 },
        -      mac: 'aef950dfea8548985b4600ca87ff9cd3ae1f34b793864ab7d099f572719cb79a' } }

It was expecting aef950dfea8548985b4600ca87ff9cd3ae1f34b793864ab7d099f572719cb79a and got 86b011abb72c222dfc2d426aa5ec60adb0d4934bab36fe0db171361b0a3a9748c*

@paulmillr any thoughts here?

[paulmillr]

Since the error only happens in browsers and not in nodejs, something is wrong with browser env

[azerella]

@paulmillr @acolytec3 Any updates here?

[acolytec3]

I'll take another look at it but this may not be quick to solve. Is this PR the solution to your issue or are you just needing some depending upgrade?

[azerella]

I'll take another look at it but this may not be quick to solve. Is this PR the solution to your issue or are you just needing some depending upgrade?

This PR is the solution to my issue. The library is currently unusable with Node v17+ due to SSL issues

[acolytec3]

Since the error only happens in browsers and not in nodejs, something is wrong with browser env

@paulmillr I've looked at this failing test again and the actual variance comes from the ciphertext property that's derived when we compute the v3String representation of the wallet. The ethers equivalent version produces the same ciphertext as nodejs does but your updated code (which uses ethereum-cryptography/aes) produces a partially different ciphertext.

For reference, this is the correct ciphertext that our nodeJS code and the ethers wallet implementatin produce -> 15e356c67d266d3ca85cff4f6d92d11720aae32cdd28d5d9885836dacb1d213b

This is what the browser version of our code now produces -> 15e356c67d266d3ca85cff4f6d92d117b636b68ad6172c858348af44ede45ac4

so the last 32 bytes of the ciphertext are different. Is it possible there's some obscure variance in ethereum-cryptography/aes for how it sets up the cipher between nodejs and browser versions?

[paulmillr]

Is it possible there's some obscure variance in ethereum-cryptography/aes for how it sets up the cipher between nodejs and browser versions?

Yes.

[acolytec3]

@azerella what is your specific use case here? We're not likely to get this update done quickly and this is a basically deprecated module anyway that should be able to be swapped out for ethereum-cryptography or possibly ethers depending on the specifics of your use case.