Extract genesis config and improve adding builtin services [ECR-3750] #1541

skletsun · 2019-11-14T08:59:38Z

Overview

Genesis config is extracted into separated entity.
Improved adding builtin services.

UPD: Current status (since [c7abbea]):

Add ability to specify default instances for ServiceFactory.
Remove InstanceConfig.
Fix majourity of relatively small comments.
Refactor Node::new().
WellKnownRuntime and the rest of tasks from ECR-3744.
Get rid of InstanceCollection.

See: https://jira.bf.local/browse/ECR-3750, https://jira.bf.local/browse/ECR-3690.

Definition of Done

There are no TODOs left in the merged code
Change is covered by automated tests
Benchmark results are attached (if applicable)
The coding guidelines are followed
Public API has proper documentation
Changelog is updated if needed (in case of notable or breaking changes)
The continuous integration build passes

exonum/src/blockchain/config.rs

alekseysidorov · 2019-11-14T09:15:16Z

exonum/src/blockchain/config.rs

+        self
+    }
+
+    pub fn from_spec(instance_spec: InstanceSpec, constructor: impl BinaryValue) -> Self {


By this method there is no way to set deploy args, is this a correct approach?

Yes. The main intention of this method was using in tests for converting in tests. I wanted to reflect this in the documentation, but forgot. Now I see that this method is now such useful, so probably it will be better to get rid of it.

exonum/src/blockchain/builder.rs

skletsun · 2019-11-14T09:47:23Z

There are some minor tasks left (like write docs, rename structs), but the PR, in general, is ready for review.

codecov · 2019-11-14T13:34:30Z

Codecov Report

Merging #1541 into master will decrease coverage by 0.02%.
The diff coverage is 94.1%.

@@            Coverage Diff             @@
##           master    #1541      +/-   ##
==========================================
- Coverage   93.81%   93.79%   -0.03%     
==========================================
  Files         204      204              
  Lines       30047    30302     +255     
==========================================
+ Hits        28188    28421     +233     
- Misses       1859     1881      +22

Impacted Files	Coverage Δ
test-suite/testkit/tests/counter/main.rs	`99.24% <ø> (-0.01%)`	⬇️
...sts/inflating_currency/inflating_cryptocurrency.rs	`97.4% <ø> (ø)`	⬆️
test-suite/testkit/tests/interfaces/services.rs	`98.43% <ø> (ø)`	⬆️
services/supervisor/src/lib.rs	`85.83% <ø> (-0.57%)`	⬇️
test-suite/testkit/tests/service_hooks/hooks.rs	`100% <ø> (ø)`	⬆️
...est-suite/testkit/tests/inflating_currency/main.rs	`100% <ø> (ø)`	⬆️
cli/src/lib.rs	`0% <ø> (ø)`	⬆️
services/supervisor/tests/supervisor/inc.rs	`96.07% <ø> (-0.22%)`	⬇️
test-suite/testkit/tests/counter/counter.rs	`85.71% <ø> (ø)`	⬆️
exonum/src/proto/mod.rs	`95.23% <ø> (ø)`	⬆️
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97d2c71...b94ae16. Read the comment docs.

exonum/src/helpers/mod.rs

exonum/src/blockchain/mod.rs

exonum/src/blockchain/config.rs

exonum/src/blockchain/builder.rs

slowli

Comments as of 53f362a. I'd suggest to remove InstanceConfig within this PR; if possible, refactor Node::new() as well. (Is there a task to make genesis config optional for node startup? If I understand correctly, it is not used once the network is launched.)

examples/sample_runtime/src/main.rs

exonum/benches/criterion/block.rs

exonum/src/blockchain/builder.rs

exonum/src/blockchain/config.rs

exonum/src/blockchain/mod.rs

slowli · 2019-11-15T15:31:38Z

exonum/src/node/mod.rs

@@ -950,8 +951,14 @@ impl Node {
            node_cfg.service_keypair(),
            ApiSender::new(channel.api_requests.0.clone()),
        );
-        let blockchain = BlockchainBuilder::new(blockchain, node_cfg.consensus.clone())
-            .with_rust_runtime(channel.endpoints.0.clone(), services)
+        let (rust_runtime, genesis_config) = create_rust_runtime_and_genesis_config(


I'd suggest to refactor Node::new for clarity. Ideally, it should accept GenesisConfig directly or via a field in NodeConfig (since the consensus config is already there). Likewise, service factories should be accepted instead of InstanceCollections.

Should we even pass service factories here? Wouldn't it be better to pass RustRuntime populated with factories along with other runtimes instead?

exonum/src/runtime/types.rs

exonum/tests/explorer/blockchain/mod.rs

alekseysidorov · 2019-11-18T14:33:52Z

exonum/src/node/mod.rs

+        );
+
+        let blockchain = BlockchainBuilder::new(blockchain, genesis_config)
+            .with_runtime(rust_runtime)
            .with_external_runtimes(external_runtimes)


This method looks like an redundant.

Going to get rid of it during the Node::new() refactoring.

…untime()`.

aleksuss · 2019-12-02T06:30:26Z

examples/sample_runtime/src/main.rs

-        )
-        .with_additional_runtime(SampleRuntime::default())
+    let supervisor_service = SimpleSupervisor::new();
+    let genesis_config = GenesisConfigBuilder::with_consensus_config(consensus_config)


I suppose GenesisConfigBuilder::new().with_consensus_config(consensus_config) would be more idiomatic ?

Is there any condition when we don't want to call with_consensus_config? I suggest to just rename this method to new, so it'll be GensisConfigBuilder::new(consensus_config).

However maybe it can be done later.

Well, perhaps it makes sense for this. But I'd also suggest to do this later.

I think that since it's not a common scenario, it's better to have a separate constructor like without_consensus_config.

slowli

Couple of comments as of 2e803d0.

slowli · 2019-12-02T11:36:34Z

test-suite/testkit/src/builder.rs

    instances: Vec<InstanceInitParams>,
-    artifacts: HashMap<ArtifactId, Vec<u8>>,
+    artifacts: HashSet<ArtifactSpec>,


There is a difference between HashMap<ArtifactId, Vec<u8>> and HashSet<ArtifactSpec>; the first overrides different specs for the same ArtifactId, while the second admits several ArtifactIds with different specs.

Thanks for pointing out, corrected.

slowli · 2019-12-02T11:37:47Z

test-suite/testkit/src/builder.rs

-        let (id, runtime) = runtime.into();
-        if id == RustRuntime::ID as u32 || self.additional_runtimes.contains_key(&id) {
-            panic!("TestkitBuilder already contains runtime with id {}", id);
+    pub fn with_additional_runtime(mut self, runtime: impl Into<RuntimeInstance>) -> Self {


Why not impl WellKnownRuntime?

Good point. I implemented it in analogue to the BlockchainBuilder.with_runtime() which has to accept RuntimeInstance as well, but for this case impl WellKnownRuntime looks better.

slowli · 2019-12-02T11:38:32Z

test-suite/testkit/src/builder.rs

-            .or_insert_with(|| deploy_spec.into_bytes());
+    pub fn with_artifact(mut self, artifact: impl Into<ArtifactSpec>) -> Self {
+        let artifact = artifact.into();
+        if !self.artifacts.contains(&artifact) {


Nit: This check seems redundant.

Re-wrote it to be actual again.

slowli · 2019-12-02T11:43:02Z

test-suite/testkit/src/lib.rs

-        self,
-        runtimes: impl IntoIterator<Item = impl Into<(u32, Box<dyn Runtime>)>>,
-    ) -> TestKit {
+    pub fn resume(self, runtimes: impl IntoIterator<Item = impl Into<RuntimeInstance>>) -> TestKit {


Nit: I'm almost sure this argument won't work with multiple runtimes of different types since the iterator items need to have the same type. Oh well, the old definition had the same problem.

Well, in fact, it is used exactly for this situation. More presize - in tests there are a few places where we pass RustRuntime this way.

test-suite/testkit/tests/interfaces/services.rs

exonum/src/runtime/mod.rs

CHANGELOG.md

popzxc

I've avoided all the nitpicks for this to get merged, but there is still one moment I can't agree with.

Once fixed, I'll approve.

popzxc · 2019-12-03T09:46:14Z

exonum/src/runtime/types.rs

+    }
+}
+
+impl<T: Into<ArtifactId>> From<T> for ArtifactSpec {


This conversion seems to be very error-prone, since it hides the fact of setting payload to empty value.
Service having no payload is not an invariant, but From conversion makes it feel like it is.

I suggest it to be a method with the name explaining it unambiguously, e.g. new_without_spec (and @slowli originally suggested it to be a method, and his suggestion still makes sense).

popzxc · 2019-12-03T09:46:26Z

exonum/src/runtime/types.rs

@@ -324,6 +371,15 @@ impl Display for InstanceSpec {
    }
 }

+impl From<InstanceSpec> for InstanceInitParams {
+    fn from(spec: InstanceSpec) -> Self {


…/exonum into ecr-3750-extract-genesis-config

alekseysidorov · 2019-12-03T14:59:49Z

cli/src/lib.rs

    /// Adds a new Runtime to the list of available runtimes.
    ///
    /// Note that you don't have to add a Rust Runtime, since it's included by default.
-    pub fn with_external_runtime(mut self, runtime: impl Into<(u32, Box<dyn Runtime>)>) -> Self {
+    pub fn with_external_runtime(mut self, runtime: impl WellKnownRuntime) -> Self {


One small nit: It turns that all possible runtimes are well known. :)

I think that we should think about trait name after that this PR will be merged.

exonum/src/blockchain/mod.rs

skletsun requested review from alekseysidorov, aleksuss, popzxc and slowli as code owners November 14, 2019 08:59

skletsun changed the title ~~Extract genesis config [ECR-3750]~~ Extract genesis config [ECR-3750][ECR-3609] Nov 14, 2019

skletsun changed the title ~~Extract genesis config [ECR-3750][ECR-3609]~~ Extract genesis config and improve adding builtin services [ECR-3750] [ECR-3609] Nov 14, 2019

alekseysidorov reviewed Nov 14, 2019

View reviewed changes

aleksuss added this to the Release 0.13 milestone Nov 14, 2019

skletsun commented Nov 14, 2019

View reviewed changes

exonum/src/blockchain/builder.rs Show resolved Hide resolved

skletsun requested a review from dmitry-timofeev November 14, 2019 09:47

popzxc added core Changes affecting exonum core update changelog labels Nov 14, 2019

popzxc reviewed Nov 14, 2019

View reviewed changes

dmitry-timofeev removed their request for review November 14, 2019 16:45

slowli reviewed Nov 15, 2019

View reviewed changes

alekseysidorov reviewed Nov 18, 2019

View reviewed changes

aleksuss changed the base branch from dynamic_services to master November 22, 2019 12:25

skletsun added 11 commits November 25, 2019 11:25

Added GenesisConfig & related.

18592c6

Updated tests.

ca865e9

Update Node.

5c66936

Update benches.

dc05731

Update sandbox and explorer tests.

0de7f76

Updated runtime tests.

3ee4bc2

Updated TestKit.

9ae26ea

Updated SampleRuntime.

6cf6ab3

Improve adding builtin services [ECR-3690].

5251193

Removed the `InstanceConfig::from_spec' method. [ci skip]

8f1baee

Rename BlockchainBuilder-s .with_additional_runtime() to `.with_r…

fd1d1b9

…untime()`.

skletsun added 3 commits December 2, 2019 02:33

Adapt code for recent changes.

9c6aa86

Rename DefaultInstance#DEFAULT_XXX constants to DefaultInstance#XXX.

9bbb5f7

Rename DefaultXXX to BuiltinXXX.

d7bc758

aleksuss reviewed Dec 2, 2019

View reviewed changes

skletsun added 3 commits December 2, 2019 09:27

Remove needless InstanceCollection for Supervisor.

7d1e662

Remove InstanceCollection from tests.

6b017de

Merge branch 'master' into ecr-3750-extract-genesis-config

2e803d0

alekseysidorov approved these changes Dec 2, 2019

View reviewed changes

skletsun changed the title ~~Extract genesis config and improve adding builtin services [ECR-3750] [ECR-3609]~~ Extract genesis config and improve adding builtin services [ECR-3750] Dec 2, 2019

Update CHANGELOG.md.

2300f76

slowli reviewed Dec 2, 2019

View reviewed changes

skletsun added 2 commits December 2, 2019 15:20

Merge branch 'master' into ecr-3750-extract-genesis-config

a0ddd61

Fix issues after merging with master.

031fa11

popzxc reviewed Dec 2, 2019

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

skletsun added 5 commits December 2, 2019 16:11

Fix changelog placeholder line.

5da3eef

Add runtime as WellKnownRuntime for TestKit and Node builders.

f85083f

Fix issue with artifacts adding.

a948d9d

Rename back to DefaultInstance.

f771694

Mention unfinished tasks in TODO:

f3dc086

aleksuss approved these changes Dec 3, 2019

View reviewed changes

popzxc suggested changes Dec 3, 2019

View reviewed changes

Oleksandr Anyshchenko and others added 4 commits December 3, 2019 13:30

Merge branch 'master' into ecr-3750-extract-genesis-config

a35eaec

Rewrite adding artifacts as suggested.

e624512

Rewrite adding instances as suggested during review.

f9d352a

Merge branch 'ecr-3750-extract-genesis-config' of github.com:skletsun…

3b5c725

…/exonum into ecr-3750-extract-genesis-config

alekseysidorov approved these changes Dec 3, 2019

View reviewed changes

popzxc approved these changes Dec 3, 2019

View reviewed changes

skletsun added 2 commits December 3, 2019 17:13

Remove redundant checks for artifact being already deployed.

d3eaa20

Fix spelling [ci skip].

b94ae16

aleksuss merged commit 0729abf into exonum:master Dec 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract genesis config and improve adding builtin services [ECR-3750] #1541

Extract genesis config and improve adding builtin services [ECR-3750] #1541

skletsun commented Nov 14, 2019 •

edited

Loading

alekseysidorov Nov 14, 2019

skletsun Nov 14, 2019

skletsun commented Nov 14, 2019

codecov bot commented Nov 14, 2019 •

edited

Loading

slowli left a comment

slowli Nov 15, 2019

skletsun Nov 25, 2019

alekseysidorov Nov 18, 2019

skletsun Nov 25, 2019

aleksuss Dec 2, 2019

popzxc Dec 2, 2019

popzxc Dec 2, 2019 •

edited

Loading

skletsun Dec 2, 2019 •

edited

Loading

popzxc Dec 2, 2019

slowli left a comment

slowli Dec 2, 2019

skletsun Dec 3, 2019

slowli Dec 2, 2019

skletsun Dec 3, 2019

slowli Dec 2, 2019

skletsun Dec 3, 2019

slowli Dec 2, 2019

skletsun Dec 3, 2019

popzxc left a comment

popzxc Dec 3, 2019

popzxc Dec 3, 2019

alekseysidorov Dec 3, 2019

Extract genesis config and improve adding builtin services [ECR-3750] #1541

Extract genesis config and improve adding builtin services [ECR-3750] #1541

Conversation

skletsun commented Nov 14, 2019 • edited Loading

Overview

Definition of Done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skletsun commented Nov 14, 2019

codecov bot commented Nov 14, 2019 • edited Loading

Codecov Report

slowli left a 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

popzxc Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

skletsun Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slowli left a 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

popzxc left a 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

skletsun commented Nov 14, 2019 •

edited

Loading

codecov bot commented Nov 14, 2019 •

edited

Loading

popzxc Dec 2, 2019 •

edited

Loading

skletsun Dec 2, 2019 •

edited

Loading