Replaces eosjs-ecc with elliptic #37

bradlhart · 2019-12-17T16:43:22Z

Change Description

Updated to replace eosjs-ecc references with elliptic. Adjusted tests.
Additionally updated to bump eosjs version to edge. Changed usages of eosjs JsonRpc.

API Changes

API Changes

Documentation Additions

Documentation Additions

GreenBusDriver · 2019-12-17T20:40:44Z

package.json

@@ -22,7 +22,8 @@
    "bip38": "^2.0.2",
    "bs58": "^4.0.1",
    "classnames": "^2.2.6",
-    "eosjs": "^20.0.0",
+    "elliptic": "6.5.2",
+    "eosjs": "20.0.4-e2c667e.0",


2 things here

We don't want to be merging any of this to master. Merging to master is only what we do once we want to release the product (which we don't yet). Please change the base of this branch to develop.

We will never reference an edge release of eosjs in a master PR. I think this is just a consequence of point Update README.md #1, but I wanted to acknowledge the issue separately just to ensure the training...

Switched the PR to use develop instead of master. Branch/commit were started from develop locally.

GreenBusDriver · 2019-12-17T22:56:11Z

src/store/auths/authsActions.ts

@@ -81,7 +83,9 @@ export const authAdd = (nickname: string, privateKey: string, passphrase: string
        throw new Error('Invalid Passphrase')
      }

-      if (!isValidPrivate(privateKey)) {
+      try {
+        PrivateKey.fromString(privateKey).toElliptic()


it'll fail on the first part; you can remove .toElliptic()

d8455f0#diff-b6bf3919e763d246ff62d57e2bcbc07b

GreenBusDriver · 2019-12-17T22:59:14Z

src/utils/Api.ts

@@ -17,7 +17,7 @@ export default class Api {
  private chainId: string

  constructor(abis: HexAbi[], publicKeys: string[], chainId: string) {
-    this.rpc = new JsonRpc(null) // provide null chainURL to ensure vault isn't reaching out to chain for info
+    this.rpc = new JsonRpc('') // provide null chainURL to ensure vault isn't reaching out to chain for info


Why must this be empty string? What was changed that required this? Was that change really necessary?
Also, if this is necessary, let's move this into the function as a default arg value to make the function robust against bad input and not burden clients with having to know to add this arg to their calls.

I've made PRs on eosjs to resolve the issue on eosjs side (two options to either allow or error)
EOSIO/eosjs#631
EOSIO/eosjs#632

As for this package, I believe changing it to an empty string is the better option since the JsonRpc constructor is expecting a string for endpoint with TS. Using null instead is not good practice and is circumventing type checking. I'm updating to change "provide null chainURL" to "provide empty chainURL" though.

https://github.com/EOSIO/eosio-reference-chrome-extension-authenticator-app/pull/37/files/ead38bb2420c82514a8d719d5d2bc424a04c564c..d8455f0e00255a5be1781e1643d298daf1572df3#diff-db1f72b58945ed2936a1519c7c18f11c

GreenBusDriver · 2019-12-30T21:31:35Z

src/utils/Api.ts

@@ -17,7 +17,7 @@ export default class Api {
  private chainId: string

  constructor(abis: HexAbi[], publicKeys: string[], chainId: string) {
-    this.rpc = new JsonRpc(null) // provide null chainURL to ensure vault isn't reaching out to chain for info
+    this.rpc = new JsonRpc('') // provide empty chainURL to ensure vault isn't reaching out to chain for info


Can we just remove this value altogether? This still makes me uncomfortable because it looks like you have to pass an empty string where we used to have to pass null. If there's no value, there should just be no value, and we should default the value or deal with no value internally. Using a value as a non-value is not high on the coolness scale... :)

GreenBusDriver · 2019-12-30T21:32:14Z

package.json

@@ -22,7 +22,8 @@
    "bip38": "^2.0.2",
    "bs58": "^4.0.1",
    "classnames": "^2.2.6",


Let's lock these deps

GreenBusDriver · 2019-12-30T21:34:42Z

src/store/auths/authsActions.ts

@@ -96,7 +100,7 @@ export const authAdd = (nickname: string, privateKey: string, passphrase: string
      auths.push({
        nickname,
        encryptedPrivateKey: await encrypt(privateKey, passphrase),
-        publicKey: privateToPublic(privateKey),


BTW, one option for avoiding breaking this API is to move these toElliptic() and fromElliptic() to a separate EcConversions API, and just keep them completely separate from the primarily API. Then the primary API remains pristine...

OK, we've now beaten this horse. I agree that we should just put the PublicKey, PrivateKey, and Signature classes in the jssig file and allow people to grab them directly if they choose to. They're just helper functions, and I think that draws that line clearly enough.

jlamarr22 · 2020-01-10T20:13:48Z

src/store/auths/authsActions.ts

@@ -105,6 +110,19 @@ export const authAdd = (nickname: string, privateKey: string, passphrase: string
    }
  }

+export const recoverPublicKey = (privateKey: string, data: string) => {


This would ideally be a helper in eosjs or somewhere else since it's so convoluted and requires quite a bit of elliptic knowledge, which could be abstracted away from the consumer.

Changes JsonRpc usages to new eosjs version standard Peer review changes and last ecc reference Locking dependencies bump resolutions Fix issues lock dependencies bump Set RPC to null instead of hard-coding Abstract logic in to auth utils Fix tests Add tests

…eferences

bradlhart force-pushed the remove-eosjs-ecc-references branch from ec213cd to ead38bb Compare December 17, 2019 17:17

GreenBusDriver reviewed Dec 17, 2019

View reviewed changes

bradlhart changed the base branch from master to develop December 17, 2019 20:50

GreenBusDriver reviewed Dec 17, 2019

View reviewed changes

GreenBusDriver reviewed Dec 30, 2019

View reviewed changes

jlamarr22 force-pushed the remove-eosjs-ecc-references branch 2 times, most recently from 910bf0e to 4925d4e Compare January 10, 2020 16:02

jlamarr22 mentioned this pull request Jan 10, 2020

Security audit #38

Closed

2 tasks

jlamarr22 force-pushed the remove-eosjs-ecc-references branch 2 times, most recently from 25ce6e5 to ec6262e Compare January 10, 2020 20:11

jlamarr22 reviewed Jan 10, 2020

View reviewed changes

Replaces ecc with elliptic

ba3b852

Changes JsonRpc usages to new eosjs version standard Peer review changes and last ecc reference Locking dependencies bump resolutions Fix issues lock dependencies bump Set RPC to null instead of hard-coding Abstract logic in to auth utils Fix tests Add tests

jlamarr22 force-pushed the remove-eosjs-ecc-references branch from ec6262e to ba3b852 Compare January 10, 2020 20:24

Bradley Hart added 2 commits January 24, 2020 13:59

Merge remote-tracking branch 'origin/develop' into remove-eosjs-ecc-r…

54e0269

…eferences

eosjs@edge getPublicKey helper function

b50cac5

jeffreyssmith2nd approved these changes Jan 24, 2020

View reviewed changes

bradlhart merged commit 5513202 into develop Jan 24, 2020

bradlhart deleted the remove-eosjs-ecc-references branch January 24, 2020 19:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replaces eosjs-ecc with elliptic #37

Replaces eosjs-ecc with elliptic #37

bradlhart commented Dec 17, 2019 •

edited

Loading

GreenBusDriver Dec 17, 2019

bradlhart Dec 17, 2019

GreenBusDriver Dec 17, 2019

bradlhart Dec 18, 2019

GreenBusDriver Dec 17, 2019

bradlhart Dec 18, 2019

bradlhart Dec 18, 2019

GreenBusDriver Dec 30, 2019

GreenBusDriver Dec 30, 2019

GreenBusDriver Dec 30, 2019

GreenBusDriver Jan 3, 2020

jlamarr22 Jan 10, 2020 •

edited

Loading

Replaces eosjs-ecc with elliptic #37

Replaces eosjs-ecc with elliptic #37

Conversation

bradlhart commented Dec 17, 2019 • edited Loading

Change Description

API Changes

Documentation Additions

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlamarr22 Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

bradlhart commented Dec 17, 2019 •

edited

Loading

jlamarr22 Jan 10, 2020 •

edited

Loading