Skip to content
This repository was archived by the owner on Nov 6, 2020. It is now read-only.

Encapsulate access to the client for secret store#11232

Merged
grbIzl merged 16 commits into
masterfrom
RemoveClientFromSecretStore
Jan 7, 2020
Merged

Encapsulate access to the client for secret store#11232
grbIzl merged 16 commits into
masterfrom
RemoveClientFromSecretStore

Conversation

@grbIzl
Copy link
Copy Markdown
Collaborator

@grbIzl grbIzl commented Nov 1, 2019

Hopefully it's the last step, that will allow to separate SecretStore to its own repository.
Currently Secret Store uses Client in order to retrieve information from the chain, mostly for its service contracts (like check of permissions). This functionality should be hidden behind abstract trait, that should be implemented inside ethcore.
Details about the solution:

Before this change:

  • SecretStore server is started (from parity main) using method with parameters-pointers to Client, Miner etc:
    pub fn start(client: Arc<Client>, sync: Arc<dyn SyncProvider>, miner: Arc<Miner>, self_key_pair: Arc<dyn NodeKeyPair>, mut config: ServiceConfiguration, db: Arc<dyn KeyValueDB>, executor: Executor) -> Result<Box<dyn KeyServer>, Error>
  • SecretStore module has build dependencies to ethcore libraries
  • Parity module has thin layer inside to start secret store (mostly with required parameters)

After this change:

  • SecretStore server is started (from parity main) using method with parameter-pointer to SecretStoreChain structure, declared in SecretStore module:
    pub fn start(trusted_client: Arc<dyn SecretStoreChain>, self_key_pair: Arc<dyn SigningKeyPair>, mut config: ServiceConfiguration, db: Arc<dyn KeyValueDB>, executor: Executor) -> Result<Box<dyn KeyServer>, Error>
  • This newly added structure doesn't have dependencies to ethcore in its API. So, as a result, SecretStore module doesn't have any dependencies to ethcore or any other internal Parity libraries. It has only dependencies to public crates from crates.io
  • Parity module has thicker layer inside to start secret store, that contains implementation of SecretStoreChain (this implementation has dependencies to ethcore, but as it's the internal implementation inside parity, it's ok)

Tasks:

@grbIzl grbIzl added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels Nov 1, 2019
@dvdplm
Copy link
Copy Markdown
Collaborator

dvdplm commented Nov 20, 2019

* Solve parity runtime dependency

This one could be pretty involved I think. Do you have any thoughts on it (yet)?

@grbIzl grbIzl changed the title [WIP] Encapsulate access to the client for secret store Encapsulate access to the client for secret store Nov 25, 2019
@grbIzl grbIzl added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Nov 25, 2019
@grbIzl grbIzl requested a review from svyatonik November 25, 2019 11:18
Copy link
Copy Markdown
Collaborator

@svyatonik svyatonik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM, though most of introduced traits will be replaced by higher-level traits later. Like some new concepts are still ethereum-specific and won't allow us to use SS in other projects. E.g. contracts are still call Blockchain::retrieve_last_logs() and manually parse ethereum logs. But imo this should be merged as is, because it allows us to extract SS to separate repo (which is, again a necessary step to use it in other projects).

Comment thread secret-store/Cargo.toml Outdated
@svyatonik svyatonik added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 26, 2019
Copy link
Copy Markdown
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge conflicts need to be resolved and I thought paritytech/parity-common#271 needs to be merged first?

Comment thread secret-store/Cargo.toml Outdated
@grbIzl
Copy link
Copy Markdown
Collaborator Author

grbIzl commented Dec 10, 2019

I thought paritytech/parity-common#271 needs to be merged first?

We discussed it @dvdplm and decided to try to separate secret store code into the new repository locally, in order to make sure, that we don't need anything except runtime (that is in PR 271). But in order to proceed to this, I need this preparation PR to be merged.

Copy link
Copy Markdown
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments, but I'm too unfamiliar with the secretstore code to do a proper review. I am also a bit unsure of what the path looks like from this to a stand-alone secretstore; maybe you can extend the PR description to help reviewers?

Comment thread parity/secretstore/blockchain.rs
Comment thread parity/secretstore/blockchain.rs
Comment thread parity/secretstore/blockchain.rs Outdated
Comment thread parity/secretstore/nodekeypair.rs Outdated
Comment thread parity/secretstore/nodekeypair.rs Outdated
Comment thread parity/secretstore/nodekeypair.rs Outdated
Comment thread parity/secretstore/nodekeypair.rs Outdated
Comment thread secret-store/src/blockchain.rs Outdated
fn block_number(&self, id: BlockId) -> Option<BlockNumber>;

/// Retrieve last blockchain logs for the filter
fn retrieve_last_logs(&self, filter: Filter) -> Option<Vec<RawLog>>;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To @svyatonik's point about avoiding "ethereum-isms": should we provide a default impl here?

@grbIzl grbIzl force-pushed the RemoveClientFromSecretStore branch from 79bae63 to df7dc81 Compare January 2, 2020 14:22
@dvdplm dvdplm requested a review from svyatonik January 7, 2020 09:28
@dvdplm dvdplm requested review from dvdplm and svyatonik January 7, 2020 09:55
@grbIzl grbIzl merged commit 424b38a into master Jan 7, 2020
@grbIzl grbIzl deleted the RemoveClientFromSecretStore branch January 7, 2020 13:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants