Local signing fixed #2580
Local signing fixed #2580
Conversation
There was a problem hiding this comment.
Coming in with little context, so just some surface-level feedback and clarifying questions:
- ethereumjs-tx tidies up the TransactionSigner nicely 👌
- Is ethereumjs-tx a full replacement for eth-lib? I've wondered if eth-lib is "actively" maintained (last commit 1 yr ago).
- Haven't looked into how you're packaging the lib or what support promises web3.js has made; just making sure this line in the ethereumjs-tx README is on your radar:
Note: this package expects ECMAScript 6 (ES6) as a minimum environment. From browsers lacking ES6 support, please use a shim (like es6-shim) before including any of the builds from this repo.
| @@ -153,7 +169,7 @@ export default class EthSendTransactionMethod extends SendTransactionMethod { | |||
| * @returns {Boolean} | |||
| */ | |||
| hasAccounts() { | |||
| return this.moduleInstance.accounts && this.moduleInstance.accounts.accountsIndex > 0; | |||
| return this.moduleInstance.accounts && this.moduleInstance.accounts.wallet.accountsIndex > 0; | |||
There was a problem hiding this comment.
Is there any opportunity for wallet to return undefined and break here?
| @@ -20,7 +20,7 @@ | |||
| import scryptsy from 'scrypt.js'; | |||
| import isString from 'lodash/isString'; | |||
| import isObject from 'lodash/isObject'; | |||
| import {fromPrivate, create, sign, decodeSignature} from 'eth-lib/lib/account'; // TODO: Remove this dependency | |||
| import * as EthLibAccount from 'eth-lib/lib/account'; // TODO: Remove this dependency | |||
There was a problem hiding this comment.
👌 namespacing. Is this functionality redundant with ethereumjs-tx?
There was a problem hiding this comment.
There is an ethereumjs package I could use to replace the eth-lib here too. But I will do this later because other enhancements have a higher priority. Thanks for the hint. I'll write it down on my checklist.
|
The es6-shim isn't required because of the babel polyfill I'm including. Thanks for your review! @marcgarreau |
Description
ethereumjs-txsigning implemented.Fixes #1021 #1517 #1074 #1169 #2033 #1793 #1126 #1134 #2578 #2569
Type of change
Checklist:
npm run testin the root folder with success and extended the tests if necessary.npm run buildin the root folder and tested it in the browser and with node.npm run dtslintin the root folder and tested that all my types are correct