Deprecate account management by tomusdrw · Pull Request #10213 · openethereum/parity-ethereum

tomusdrw · 2019-01-18T12:54:52Z

Move account management to a single crate (ethcore-accounts)
Avoid depenending on accounts directly (only main binary, parity-rpc and secret-store (one struct) depends on it currently)
Rely on raw secrets wherever possible (engine-signer and secret-store)
Add compilation feature to allow easier removal in the future (diff in binary size is 5MB (61 vs 56 on my machine))
Feature-gate RPCs that deal with accounts, add deprecation notice to others.

Related: #9997

Future work:

Consider extracting accounts to a jsonrpc-proxy
Integrate with CLEF (https://github.com/ethereum/go-ethereum/tree/master/cmd/clef)

niklasad1 · 2019-01-21T10:06:48Z

hrm, I don't understand why the android build fails

svyatonik · 2019-01-22T11:49:30Z


 [features]
+default = ["accounts"]
+accounts = ["ethcore-accounts", "ethcore-secretstore/accounts", "parity-rpc/accounts"]


This causes Parity to be built with "secretstore" feature enabled by default (since default features are "accounts" and it implies "ethcore-secretstore/accounts"). I guess we need something like accounts AND secretstore = ["ethcore-secretstore/accounts"], but I'm not sure it could be defined within Cargo.toml. Probably replace accounts => [..., "ethcore-secretstore/accounts", ...] with secretstore => [""ethcore-secretstore/accounts", "ethcore-accounts"]?

or even better - just remove "ethcore-secretstore/accounts" from "accounts" - so that there could be --features "", --features "accounts", --features "secretstore" and --features "secretstore accounts".

I don't think that would work, we need to enable ethcore-secretstore/accounts if both secretstore and accounts features are enabled. I changed it so that we require accounts in case we compile with secretstore

svyatonik · 2019-01-22T11:52:39Z

 use crypto::DEFAULT_MAC;
 use ethkey::Secret;
-use ethcore::account_provider::AccountProvider;
+use accounts::AccountProvider;


Just curios - are we going to leave account management under accounts feature forever (i.e. for some long period), or it'll be totally purged from the codebase in the nearest future? In the latter case, we'll need to replace all address: H160, password: Password in SS RPCs with something like secret: H256 or do something else.

It's not defined yet, perhaps we are going to move some functionality to the proxy, but if we decide to remove accounts completely we need to provide some non-account-based alternatives like using secret directly. Don't know the definitive answer yet though.

insipx

Looks good!

Co-Authored-By: tomusdrw <tomusdrw@users.noreply.github.com>

niklasad1 · 2019-01-30T14:44:17Z

+pub(crate) mod light;
+mod full;
+mod prospective_signer;
+


This is a good change i.e., to break-up dispatcher into different mods however I haven't reviewed that code. I assume it is just moved?

Yeah, mostly moved, but I had to do some small changes to support the new Accounts trait instead of using AccountProvider directly.

all right, we should probably run some manual tests on the light-client before merging because rpc integration tests are still missing

niklasad1

Really good work and LGTM.

However we need to run some manual tests on the light-client before merging.

5chdn · 2019-02-07T11:46:50Z

could you sign this off @ordian @svyatonik @insipx ?

svyatonik

SS changes are OK. The rest also seems fine, though I haven't explored it in details.

5chdn · 2019-02-07T12:13:10Z

Does not compile @tomusdrw

* master: fix: parity-clib/examples/cpp/CMakeLists.txt (#10313) CI optimizations (#10297) Increase number of requested block bodies in chain sync (#10247) Deprecate account management (#10213) Properly handle check_epoch_end_signal errors (#10015) fix(osx and windows builds): bump parity-daemonize (#10291) Add missing step for Using `systemd` service file (#10175) Call private contract methods from another private contract (read-only) (#10086) update ring to 0.14 (#10262) fix(secret-store): deprecation warning (#10301) Update to jsonrpc-derive 10.0.2, fixes aliases bug (#10300) Convert to jsonrpc-derive, use jsonrpc-* from crates.io (#10298)

tomusdrw added 19 commits December 31, 2018 12:13

Extract accounts from ethcore.

644b208

Fix ethcore.

b44fd8d

Get rid of AccountProvider in test_helpers

3f47857

Fix rest of the code.

52741b0

Re-use EngineSigner, fix tests.

68f927b

Simplify EngineSigner to always have an Address.

b98a96d

Fix RPC tests.

7a26457

Add deprecation notice to RPCs.

81ff2e5

Feature to disable accounts.

42899bd

extract accounts in RPC

ae1122b

Run with accounts in tests.

dc44a7d

Merge branch 'master' into td-accounts

44abb4b

Fix RPC compilation and tests.

623bb31

Fix compilation of the binary.

858c5d3

Fix compilation of the binary.

166fa46

Fix compilation with accounts enabled.

2a2aedc

Merge remote-tracking branch 'origin/td-accounts' into td-accounts

b8709d7

Merge branch 'master' into td-accounts

78511ee

Fix tests.

f980762

tomusdrw added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jan 18, 2019

Update submodule.

7f77577

ordian reviewed Jan 18, 2019

View reviewed changes

Comment thread accounts/Cargo.toml Outdated

Remove android.

22a76c5

5chdn added this to the 2.4 milestone Jan 20, 2019

5chdn added the B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. label Jan 20, 2019

niklasad1 reviewed Jan 21, 2019

View reviewed changes

Comment thread accounts/src/lib.rs Outdated

Use derive for Default

0729133

niklasad1 reviewed Jan 22, 2019

View reviewed changes

Comment thread ethcore/src/engines/authority_round/mod.rs Outdated

niklasad1 reviewed Jan 22, 2019

View reviewed changes

Comment thread parity/account.rs Outdated

svyatonik reviewed Jan 22, 2019

View reviewed changes

tomusdrw added 5 commits January 22, 2019 13:04

Merge branch 'master' into td-accounts

e8a07d8

Don't build secretstore by default.

ad77cdf

Add link to issue.

e9543e5

Merge branch 'master' into td-accounts

121e41b

Merge branch 'master' into td-accounts

f4283d0

niklasad1 reviewed Jan 29, 2019

View reviewed changes

Comment thread Cargo.lock

tomusdrw added 3 commits January 29, 2019 10:36

Merge branch 'master' into td-accounts

c03d88b

Refresh Cargo.lock.

ddc1e5f

Fix miner tests.

9e198e0

insipx reviewed Jan 30, 2019

View reviewed changes

niklasad1 reviewed Jan 30, 2019

View reviewed changes

Comment thread rpc/Cargo.toml Outdated

Update rpc/Cargo.toml

fb57ec7

Co-Authored-By: tomusdrw <tomusdrw@users.noreply.github.com>

niklasad1 reviewed Jan 30, 2019

View reviewed changes

niklasad1 approved these changes Jan 30, 2019

View reviewed changes

Merge branch 'master' into td-accounts

de59679

niklasad1 added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 4, 2019

Merge branch 'master' into td-accounts

99c0fcf

Merge branch 'master' into td-accounts

c770172

svyatonik approved these changes Feb 7, 2019

View reviewed changes

Fix private tests.

bf127c0

5chdn merged commit d5c19f8 into master Feb 7, 2019

5chdn deleted the td-accounts branch February 7, 2019 13:34

grbIzl mentioned this pull request Jul 2, 2019

Remove excessive warning #10831

Merged

Conversation

tomusdrw commented Jan 18, 2019

Uh oh!

Uh oh!

niklasad1 commented Jan 21, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

insipx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niklasad1 left a comment

Choose a reason for hiding this comment

Uh oh!

5chdn commented Feb 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

svyatonik left a comment

Choose a reason for hiding this comment

Uh oh!

5chdn commented Feb 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

5chdn commented Feb 7, 2019 •

edited

Loading