Skip to content
This repository has been archived by the owner on Jun 17, 2021. It is now read-only.

Removing native js dependency #257

Merged
merged 8 commits into from
Jul 6, 2020

Conversation

nebojsa94
Copy link
Contributor

This PR removes keccak and secp256k1 dependencies and instead uses ethereum-cryptography package that doesn't require native dependency compiling

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 98.854% when pulling 6a6af8e on Tenderly:removing-native-js-dependency into 58c2476 on ethereumjs:master.

@coveralls
Copy link

coveralls commented Jun 10, 2020

Coverage Status

Coverage increased (+0.008%) to 99.711% when pulling ae77d7a on Tenderly:removing-native-js-dependency into d485939 on ethereumjs:master.

@nebojsa94 nebojsa94 marked this pull request as ready for review June 16, 2020 17:41
src/account.ts Outdated
publicKeyCreate,
publicKeyVerify,
publicKeyConvert,
} = require('ethereum-cryptography/shims/hdkey-secp256k1v3')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return keccak512(a)
}
default: {
throw new Error(`Invald algorithm: keccak${bits}`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/signature.ts Outdated
@@ -1,4 +1,5 @@
import * as secp256k1 from 'secp256k1'
const { sign, publicKeyConvert } = require('ethereum-cryptography/shims/hdkey-secp256k1v3')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lgtm-com
Copy link

lgtm-com bot commented Jun 19, 2020

This pull request introduces 1 alert when merging eda4d21 into 58c2476 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

alcuadrado
alcuadrado previously approved these changes Jun 22, 2020
Copy link
Member

@alcuadrado alcuadrado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good now.

For the rest of the team, this is the first PR for implementing ethereum-cryprography here. The other PRs are for older versions, and are a bit more complex, but will also have a bigger impact in the ecosystem, as those versions are used more frequently.

@holgerd77
Copy link
Member

@nebojsa94 oh sorry, I merged in another PR #248, didn't expect this to conflict with this one. 😕 Could you give this a final update, I will directly merge this in afterwards.

@alcuadrado thanks for the review and the additional context. Should we directly publish this as v7.0.3 then I assume?

Copy link
Member

@alcuadrado alcuadrado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good now, thanks @nebojsa94

@alcuadrado
Copy link
Member

Should we directly publish this as v7.0.3 then I assume?

Yep, this is the v7 version of these changes, so v7.0.3 should do.

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this looks good, will merge this in now. Thanks @nebojsa94 and @alcuadrado for the continued effort on this! 😄

@holgerd77 holgerd77 merged commit 584cc82 into ethereumjs:master Jul 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants