-
-
Notifications
You must be signed in to change notification settings - Fork 650
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Web Worker Decrypt #680
Web Worker Decrypt #680
Conversation
…tion. Unblock UI and display spinner during scrypt decryption.
onChange(value: KeystoreValue): void; | ||
onUnlock(): void; | ||
}; | ||
|
||
public render() { | ||
const { file, password } = this.props.value; | ||
const isWalletLoading = this.props.isWalletLoading; |
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.
Why not de-structure?
Impressive work! Going to leave it open a bit more and solicit feedback from @HenryNguyen5 and @wbobeirne who participated in design. |
Here's an encrypted Keystore with no password for testing purposes: |
nonStrict: boolean; | ||
} | ||
|
||
onmessage = event => { |
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.
event could use typing
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.
package.json
Outdated
@@ -110,7 +110,8 @@ | |||
"webpack": "3.10.0", | |||
"webpack-dev-middleware": "2.0.3", | |||
"webpack-hot-middleware": "2.21.0", | |||
"webpack-sources": "1.0.1" | |||
"webpack-sources": "1.0.1", | |||
"worker-loader": "^1.1.0" |
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.
Lock down version # (We try to avoid ^
versions to keep builds consistent.)
@@ -0,0 +1,22 @@ | |||
import { IFullWallet, fromPrivateKey } from 'ethereumjs-wallet'; | |||
import { toBuffer } from 'ethereumjs-util'; | |||
const Worker = require('worker-loader!./workers/scrypt-worker.worker.ts'); |
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 setup typing for this, as outlined here: https://github.com/webpack-contrib/worker-loader#integrating-with-typescript. Right now it's an implicit any
.
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.
Still typed as any, but
import Worker from 'worker-loader!./workers/scrypt-worker.worker.ts';
seems to fix it!
@@ -43,6 +43,13 @@ export function setWallet(value: IWallet): types.SetWalletAction { | |||
}; | |||
} | |||
|
|||
export function setWalletLoading(loadingStatus: boolean): types.SetWalletLoadingAction { |
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.
Existing nomenclature would be setWalletPending
@@ -57,6 +60,7 @@ export default class KeystoreDecrypt extends Component { | |||
type="password" | |||
/> | |||
</div> | |||
{isWalletLoading ? <Spinner /> : ''} |
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.
Minor, but you can simplify these boolean checks to {isWalletLoading && <Spinner />}
@@ -0,0 +1,18 @@ | |||
import { fromV3, IFullWallet } from 'ethereumjs-wallet'; | |||
|
|||
declare var postMessage: any; |
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.
From https://github.com/webpack-contrib/worker-loader#integrating-with-typescript:
const ctx: Worker = self as any;
ctx.postMessage({ foo: "foo" });
for typing
common/sagas/wallet/wallet.ts
Outdated
@@ -141,18 +146,43 @@ export function* unlockPrivateKey(action: UnlockPrivateKeyAction): SagaIterator | |||
yield put(setWallet(wallet)); | |||
} | |||
|
|||
export function* startLoadingSpinner(): SagaIterator { | |||
yield call(delay, 1000); |
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.
I'd probably shorten this to 300-500, a full second before having the site respond is a little long.
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.
User experience feeling much better. I just had one note about typing, if you can get that change in this all looks good to me!
Closes MyEtherWallet/MyEtherWallet#423, MyEtherWallet/MyEtherWallet#219
This PR includes three solutions.