Public node with accounts and signing in Frontend #5304

maciejhirsz · 2017-03-27T18:33:02Z

Disclaimer: The PR includes a copy of the dictionary for the frontend, hence the crazy diff. It's not that big.

Start parity with --public-node (suggestions for a better flag open):

Disables account managing RPCs.
UI account management is replicated in frontend. Accounts are stored in local storage in keythereum dump format.
Creating accounts, recovering accounts from phrases and signing transactions all happens in the browser.

Some concerns and general notes:

Generating keys in pure JS tends to be slow, and although there are avenues for optimization open there, a solution that re-uses Rust codebase via wasm or asm.js could potentially be better (less code to maintain). For the most part the UX feels fine as-is, although that's on a reasonable CPU, wouldn't try it on a phone.
UI currently does a lot of requests, which might be both noticeable over the wire and reduce the amount of traffic the node can handle. Throttling things would be nice.
Missing features, e.g. vaults, should either be hidden or implemented, although there seems to be little point to vaults in local storage?

rphmeier · 2017-03-28T02:44:49Z

While throttling the amount of requests the UI can make to the web node is definitely important, we should also be able to throttle the amount (and intensity) of RPC requests in general. It's fair to assume that any publicly-available RPC will be pummeled mercilessly sometimes by malicious actors.

tomusdrw

Looks good! I love the way you handled unsupported requests with AccountProvider, very clean!

Most important issue is that only a whitelist of APIs should be enabled, listed them in comment. Not sure how well GUI will work without those APIs.

tomusdrw · 2017-03-28T07:39:02Z

js/src/api/local/accounts/accounts.js

+const NULL_ADDRESS = '0x0000000000000000000000000000000000000000';
+const LOCAL_STORAGE_KEY = '_parity::localAccounts';
+
+function fromLocalStorage () {


I think UI code uses store library exactly for that.

tomusdrw · 2017-03-28T07:40:07Z

js/src/api/local/accounts/accounts.js

+
+    const account = this._store.find((account) => account.address === address);
+
+    if (account == null) {


=== null? Should be caught by linter anyway.

I tend to use == null as an existence check, since the only values it can coerce to are null and undefined. if (!account) would also work. Is there a specific way to catch existence of a value that fits the code style?

I think if (!account) { is used everywhere.

tomusdrw · 2017-03-28T07:41:20Z

js/src/api/local/accounts/accounts.js

+      return false;
+    }
+
+    if (address === this._last) {


Is there a guarantee that this._last will be lowercased? I think it would be safer to do: address === this._last.toLowerCase()

It should be guaranteed to be lowercase as any inputs where address is passed are forced to lowercase before anything is done with it. That said, I'll add a setter to make it clearer it's an expected behavior.

tomusdrw · 2017-03-28T07:42:42Z

js/src/api/local/accounts/accounts.js

+    clearTimeout(this._persistTimer);
+
+    // Throttle persisting the accounts
+    this._persistTimer = setTimeout(() => {


_.debounce?

tomusdrw · 2017-03-28T07:44:59Z

js/src/api/local/ethkey/dictionary.js

@@ -0,0 +1,7778 @@
+export default [


Can we push the dictionary as a separate library (JS and Rust?) It's already duplicated in native-signerand parity.
Will prepare a repo.

tomusdrw · 2017-03-28T07:53:10Z

js/src/api/local/middleware.js

+
+// Maps transaction requests to transaction hashes.
+// This allows the locally-signed transactions to emulate the signer.
+const transactionHashes = {};


Why is it not part of the instance? Or at least static fields if we want to share it between instances.

tomusdrw · 2017-03-28T07:56:35Z

js/src/api/api.js

+      const middleware = this.parity
+        .nodeKind()
+        .then((nodeKind) => {
+          if (nodeKind.availability === 'public') {


What if nodeKind is changed during lifetime of the UI (e.g. Parity being restarted without closing tab window)?

tomusdrw · 2017-03-28T07:58:18Z

rpc/src/v1/helpers/accounts.rs

+	match *provider {
+		Some(ref weak) => weak.upgrade().ok_or_else(Error::internal_error),
+		None => Err(Error {
+			code: ErrorCode::InvalidRequest,


I tried to use helpers::errors::codes::UNSUPPORTED_REQUEST for such stuff in the past, might be good to stay consistent with the error codes.

tomusdrw · 2017-03-28T07:59:35Z

rpc/src/v1/helpers/accounts.rs

+use ethcore::account_provider::AccountProvider;
+use jsonrpc_core::{Error, ErrorCode};
+
+pub fn unwrap_provider(provider: &Option<Weak<AccountProvider>>) -> Result<Arc<AccountProvider>, Error> {


That's pretty clever solution! :) Loving it 👍

tomusdrw · 2017-03-28T08:03:40Z

parity/configuration.rs

@@ -128,6 +128,7 @@ impl Configuration {
 			Some(true) if pruning == Pruning::Specific(Algorithm::Archive) => writeln!(&mut stderr(), "Warning: Warp Sync is disabled because pruning mode is set to archive").expect("Error writing to stderr"),
 			_ => {},
 		};
+		let public_node = self.args.flag_public_node;


Public node should have only subset of APIs enabled:
eth,net,parity,rpc,web3

Some of the disabled APIs are already useless because AccountProvider is missing, but stuff like parity_stopNetwork should also be disabled.

Resolved now: https://github.com/paritytech/parity/blob/fc31fdf3041c8b013b6e0a6fd794ff5378e935b0/parity/configuration.rs#L724-L731

tomusdrw

Just a grumble regarding nodeKind being changed during the lifetime of a website.

gavofyork · 2017-03-29T12:37:58Z

tests failing.

maciejhirsz · 2017-03-29T12:45:36Z

Yeah, not sure what's up with the tests:

ERROR: Job failed (system failure): Error response from daemon: invalid header field value "oci runtime error: container_linux.go:247: starting container process caused \"process_linux.go:258: applying cgroup configuration for process caused \\\"failed to write 15099 to cgroup.procs: write /sys/fs/cgroup/cpu,cpuacct/docker/8aa02970dbeed05350837980f150e91c801cdfde378419435f2ece4eb1b7af21/cgroup.procs: invalid argument\\\"\"\n"

@tomusdrw I talked about this with Jaco, will bundle it with network change in the FE, which triggers a full reload. UX shouldn't really be an issue as that's pretty far from standard behavior.

maciejhirsz added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. M7-ui labels Mar 27, 2017

tomusdrw suggested changes Mar 28, 2017

View reviewed changes

tomusdrw approved these changes Mar 29, 2017

View reviewed changes

tomusdrw mentioned this pull request Mar 29, 2017

Shared wordlist for brain wallets #5331

Merged

maciejhirsz force-pushed the mh-webserver branch from fc31fdf to a64cb9c Compare March 29, 2017 13:48

Squashed: Public Node

9bd3f10

maciejhirsz force-pushed the mh-webserver branch from a64cb9c to 9bd3f10 Compare March 29, 2017 15:18

Skip timing out tests

54196a8

maciejhirsz merged commit ab2c346 into master Mar 29, 2017

maciejhirsz deleted the mh-webserver branch March 29, 2017 15:48

maciejhirsz mentioned this pull request Mar 29, 2017

fix rpc tests #5338

Merged

tomusdrw mentioned this pull request May 9, 2017

External accounts support #3243

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Public node with accounts and signing in Frontend #5304

Public node with accounts and signing in Frontend #5304

maciejhirsz commented Mar 27, 2017

rphmeier commented Mar 28, 2017

tomusdrw left a comment

tomusdrw Mar 28, 2017

tomusdrw Mar 28, 2017

maciejhirsz Mar 28, 2017 •

edited

Loading

tomusdrw Mar 29, 2017

tomusdrw Mar 28, 2017

maciejhirsz Mar 28, 2017

tomusdrw Mar 28, 2017

tomusdrw Mar 28, 2017

tomusdrw Mar 28, 2017

tomusdrw Mar 28, 2017

tomusdrw Mar 28, 2017

tomusdrw Mar 28, 2017

tomusdrw Mar 28, 2017

maciejhirsz Mar 28, 2017

tomusdrw left a comment

gavofyork commented Mar 29, 2017

maciejhirsz commented Mar 29, 2017


		const account = this._store.find((account) => account.address === address);

		if (account == null) {

Public node with accounts and signing in Frontend #5304

Public node with accounts and signing in Frontend #5304

Conversation

maciejhirsz commented Mar 27, 2017

rphmeier commented Mar 28, 2017

tomusdrw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maciejhirsz Mar 28, 2017 • edited Loading

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

Choose a reason for hiding this comment

tomusdrw left a comment

Choose a reason for hiding this comment

gavofyork commented Mar 29, 2017

maciejhirsz commented Mar 29, 2017

maciejhirsz Mar 28, 2017 •

edited

Loading