-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Conversation
if (wallet && payload.transaction) { | ||
const { transaction } = payload; | ||
this._api.parity | ||
.nextNonce(transaction.from) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be called unnecessarily if transaction.nonce
is not zero. This would work:
(transaction.nonce.isZero()
? this._api.parity.nextNonce(transaction.from)
: Promise.resolve(transaction.nonce)
).then(nonce => {…})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is Promise.resolve(...).then
executed in next event loop tick? The problem is that wallet derivation takes significant amount of time and UI is not refreshed before this happens. So it needs to be wrapped with setTimeout(..., 0)
. Will try that and see if it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Promises always resolve on nextTick
afaik.
Also, async code can still block the whole UI if it runs for a while. This should be in a web worker.
@@ -82,3 +117,15 @@ export default class SignerMiddleware { | |||
}); | |||
} | |||
} | |||
|
|||
function asHex (val) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep the code DRY, it would be great if this was part of /api/format/input
-> inHex
.
|
||
const { kdf } = json.crypto; | ||
const kdfparams = json.crypto.kdfparams || {}; | ||
const pwd = new Buffer(password); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
|
||
const ciphertext = new Buffer(json.crypto.ciphertext, 'hex'); | ||
const mac = ethUtil.sha3(Buffer.concat([derivedKey.slice(16, 32), ciphertext])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sha3
is available in /api/util/sha3
.
signTransaction (transaction) { | ||
const tx = new Transaction(transaction); | ||
tx.sign(this.seed); | ||
return `0x${tx.serialize().toString('hex')}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use inHex
from /api/format/input
.
import scrypt from 'scryptsy'; | ||
import Transaction from 'ethereumjs-tx'; | ||
import crypto from 'crypto'; | ||
import aes from 'browserify-aes'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like you can import just browserify-aes/decrypter
. Also import { createDecipheriv } from …
seems to be sufficient.
Note: Just had a formal look at the code, didn't check if the UI works for me. |
import scrypt from 'scryptsy'; | ||
import Transaction from 'ethereumjs-tx'; | ||
import crypto from 'crypto'; | ||
import aes from 'browserify-aes'; | ||
import { pbkdf2Sync } from 'crypto'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -41,27 +43,26 @@ export class Wallet { | |||
if (kdfparams.prf !== 'hmac-sha256') { | |||
throw new Error('Unsupported parameters to PBKDF2'); | |||
} | |||
derivedKey = crypto.pbkdf2Sync(pwd, salt, kdfparams.c, kdfparams.dklen, 'sha256'); | |||
derivedKey = pbkdf2Sync(pwd, salt, kdfparams.c, kdfparams.dklen, 'sha256'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is crypto.pbkdf2
(which is async). Have you checked if the UI still blocks with that one? (It could still be one expensive block even if async, but it sounds promising.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Crypto is implemented in JS, so it will run in single a single thread anyway. The API is there for node compatibility: https://github.com/crypto-browserify/pbkdf2/blob/master/browser.js#L13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, ok.
@@ -75,7 +76,7 @@ export class Wallet { | |||
signTransaction (transaction) { | |||
const tx = new Transaction(transaction); | |||
tx.sign(this.seed); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use ethereumjs-util
directly (slightly more code in our codebase, but smaller dependency tree), as Transaction.prototype.sign
just calls two functions.
@@ -176,6 +176,12 @@ export default class Parity { | |||
.then(outAddress); | |||
} | |||
|
|||
nextNonce (account) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add the function under js/jsonrpc/
as well
@@ -27,6 +27,11 @@ export default class Signer { | |||
.execute('signer_confirmRequest', inNumber16(requestId), options, password); | |||
} | |||
|
|||
confirmRequestRaw (requestId, data) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same js/jsonrpc
comment as above
would like an additional tier of "external" accounts; those that are owned but which don't have a key. they should go on the main accounts page, but in a second section. signer should include allowing upload of a json key (ideally only needed once per key and stored in local storage, but that can be a second PR), but also just copy'n'pasting the unsigned hex and providing the signature (or signed hex) into a text input. this latter point is extremely relevant to cold-wallets. |
Created a separate issue for those 3 additional tasks. Let me know if you feel that any of them should be part of this PR. |
@@ -339,7 +340,10 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM> Eth for EthClient<C, SN, S, M, EM> where | |||
|
|||
let store = take_weak!(self.accounts); | |||
let accounts = try!(store.accounts().map_err(|e| errors::internal("Could not fetch accounts.", e))); | |||
Ok(accounts.into_iter().map(Into::into).collect()) | |||
let addresses = try!(store.accounts_info().map_err(|e| errors::internal("Could not fetch accounts.", e))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accounts_info
should always be a superset of accounts
, so there should be no need to merge the two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. It should be addresses_info
- fixed and added test.
rust side looks good aside from a minor qualm. |
Conflicts: rpc/src/v1/impls/signer.rs rpc/src/v1/traits/signer.rs
* Added new Deploy Contract page // Use Brace in React #2276 * Adding Web Wrokers WIP * Compiling Solidity code // Getting mandatory params #2276 * Working editor and deployment #2276 * WIP : displaying source code * Added Solidity hightling, editor component in UI * Re-adding the standard Deploy Modal #2276 * Using MobX in Contract Edition // Save to Localstorage #2276 * User select Solidity version #2276 * Loading Solidity versions and closing worker properly #2276 * Adds export to solidity editor #2276 * Adding Import to Contract Editor #2276 * Persistent Worker => Don't load twice Solidity Code #2276 * UI Fixes * Editor tweaks * Added Details with ABI in Contract view * Adds Save capabilities to contract editor // WIP on Load #3279 * Working Load and Save contracts... #3231 * Adding loader of Snippets // Export with name #3279 * Added snippets / Importing from files and from URL * Fix wrong ID in saved Contract * Fix lint * Fixed Formal errors as warning #3279 * Fixing lint issues * Use NPM Module for valid URL (fixes linting issue too) * Don't clobber tests.
Closes #3167
Altered
eth_accounts
to return also addresses from Address Book.I'm open to suggestion how this could be done in more elegant way. Probably in future when we have Dapps authorization this will be easy.