Public node: perf and fixes #5390
Conversation
maciejhirsz
commented
Apr 3, 2017
maciejhirsz
commented
- Introduced a worker pool for webworkers, which speeds up re-generating of preview avatars (apparently each worker has to JIT the js on it's own, over and over again).
- All keythereum related stuff is now done in workers as well, so the UI never hangs.
- Regenerating the avatars on the new account modal now shows a spinner.
- Restoring accounts from private keys, testing passwords and changing passwords now works as intended.
|
|
||
| const account = new Account(persist, { keyObject }); | ||
| static fromPrivateKey (persist, key, password) { |
There was a problem hiding this comment.
Not sure if you tested this 100%, if not there is a private key we use in tests on the Rust side inside tests (with a known account). If already done, ignore :)
There was a problem hiding this comment.
All the actual logic for this is in Keythereum, and the main issue here is that the tests take a while and can timeout in CI :|.
|
|
||
| this._store.push(account); | ||
| this.lastAddress = account.address; | ||
| return Account.fromPrivateKey(this.persist, privateKey, password) |
There was a problem hiding this comment.
Small nitpick, would prefer
return Account
.fromPrivateKey(...)
.then(...)
Just aligns with what we have in the rest of the codebase.
| return false; | ||
| } | ||
|
|
||
| const account = this._store[index]; | ||
| return account.isValidPassword(password) |
There was a problem hiding this comment.
Same small alignment comment as above.
js/src/api/local/ethkey/index.js
Outdated
| // Local accounts should never be used outside of the browser | ||
| export let keythereum = null; | ||
| export function createKeyObject (key, password) { | ||
| return workerPool.getWorker().action('createKeyObject', { key, password }) |
There was a problem hiding this comment.
As per above, prefer
return ...getWorker()
.action(...)
.then(...)
(Next function has it that way, just align)
js/src/api/local/ethkey/worker.js
Outdated
| } | ||
| } | ||
| }; | ||
|
|
||
| function bytesToHex (bytes) { |
There was a problem hiding this comment.
Not 100% sure why this is "duplicated", i.e. not used from /api/util/format? (I looked at this the previous round as well, could see a slight difference, but non-duplication is obviously preferred)
js/src/api/local/middleware.js
Outdated
| register('parity_changePassword', ([address, oldPassword, newPassword]) => { | ||
| const account = accounts.get(address); | ||
|
|
||
| return account.decryptPrivateKey(oldPassword) |
|
Generally looks good, one testing comment (probably already taken care of), some small style grumbles, just one function duplication where we can re-use. No major grumbles. Generally though, think we should try and invest in tests for the middleware at some time sooner rather than later, just ensuring that what we do is correct. (And that the future holds to that promise) |
jacogr
added
A5-grumble 🔥
| expect(Object.keys(instance.state.accounts).length).to.equal(7); | ||
| it('resets the accounts', () => { | ||
| expect(instance.state.accounts).to.be.null; | ||
| // expect(Object.keys(instance.state.accounts).length).to.equal(7); |
maciejhirsz
added
A0-pleasereview 🤓
tomusdrw
added
A8-looksgood 🦄
