[ECR-1627] TODO cleanup pt.4 #869

ponimas · 2018-08-13T12:54:39Z

Dead code removed from the sandbox.
Implemented proposed test scenarios.
Outdated TODOs were removed.

* Update rand to 0.5.4 - Bump version in all dependent crates - Replace removed next_u32() with gen::<u32>() (ditto for u64) - Seed of XorShiftRng is now [u8; 16] instead of [u32; 4], fix the types and generate some new seed values - Rng trait got split into Rng and RngCore, add latter where necessary to get access to methods like fill_bytes() - Replace deprecated gen_ascii_chars() with recommended usage of Alphanumeric distribution with sample_iter() - Replace deprecated gen_iter() usage with fill_bytes() and sample_iter() as approriate - Replace deprecated Rand usage with Standard distribution - Replace initialization on XorShiftRng from implicitly converted rand::random() into an explicit initialization from seed as From/Into conversion has been removed * Update snow to 0.3.1 - Resolvers are not reexported now, import them explicitly - NoiseBuilder is now called just Builder - Some functions now return SnowError instead of failure::Error, implement a conversion to NoiseError for this type - CryptoResolver now does not have Send requirement for returned types - Dh trait now expects a Result to be returned - Fix the implementation to return Ok or Err as appropriate, this allows us to get rid of a TODO - Update the tests as well to unwrap the result, this is needed to avoid compiler warnings about unused Results - Returning an Err from Dh::dh() produces a different error so update test expectations to match that * Update proptest to 0.8.4 - Strategy::Value has been changed in 0.8.0 to contain the actual value produced by the strategy instead of strategy value tree (which is now stored in Stragery::Tree). Fix the associate type usage and drop now unnecessary import of U8BinarySearch. * Update tokio-io to 0.1.7 - tokio_io::AsyncRead::framed is deprecated since 0.1.7, use Decoder from tokio_codec instead * Update tokio-retry to 0.2.0 - Retry::spawn() no longer needs event loop handle * Update cspell to 3.1.3 - Use "npm install" to install dependencies from package.json file instead of specifying version in two places * Mention updates in CHANGELOG

vldm · 2018-08-15T08:14:51Z

Can you append a brief summary of this pull request, and update title.
It's a bit weird when git history contains many entries that don't describe changes.

ponimas · 2018-08-15T08:28:11Z

@vldm But... The title is the brief summary of this pull request already.

ilammy

The changes look technically okay to me.

But I agree with @vldm that the pull request title could get some more love. For example, it's way more common to put [ECR-1234] references at the end of the title (119 vs 4 in git history). It also could be a bit more imperative and detailed like Cleanup outdated TODOs. But I'd keep it as is this time because there are three already merged PRs with the same title pattern.

ilammy · 2018-08-15T12:22:27Z

exonum-dictionary.txt

@@ -86,8 +86,10 @@ postvote
 precheck
 precommit
 precommits
+precommiters


<span class="grammarnazi">It is precommitters, with double t.</span>

ilammy · 2018-08-15T12:24:27Z

exonum/src/sandbox/consensus/invalid_message.rs

@@ -76,8 +75,7 @@ fn ignore_propose_from_non_leader() {

    let propose = ProposeBuilder::new(&sandbox)
        .with_validator(ValidatorId(3))    //without this line Prevote would have been broadcast
-        .with_duration_since_sandbox_time(PROPOSE_TIMEOUT)
-        .build();
+                .build();


There's some weird extra indentation here and there. Shouldn't rustfmt complain about this?..

looks like bug in rustfmt

ilammy · 2018-08-15T12:31:38Z

exonum/src/sandbox/consensus/round_details.rs

@@ -1230,8 +1197,7 @@ fn handle_full_propose_wrong_state_hash() {

    sandbox.assert_state(Height(1), Round(1));
    //    let tmp_propose = ProposeBuilder::new(&sandbox)
-    //        .with_duration_since_sandbox_time(PROPOSE_TIMEOUT)
-    //        .build();
+    //            //        .build();


Duplicate // here looks weird.

And... do we need this code? Keeping commented out code is not a good practice.

ilammy · 2018-08-15T12:32:56Z

exonum/src/sandbox/consensus/round_details.rs

@@ -1561,8 +1516,7 @@ fn commit_as_leader_send_propose_round_timeout() {
    sandbox.check_broadcast_status(new_height, &block.hash());

    let propose = ProposeBuilder::new(&sandbox)
-        .with_duration_since_sandbox_time(PROPOSE_TIMEOUT)
-        //        .with_tx_hashes(&[tx.hash()]) //ordinary propose, but with this unreceived tx
+                //        .with_tx_hashes(&[tx.hash()]) //ordinary propose, but with this unreceived tx


I'd remove this commented out line entirely then.

ilammy · 2018-08-15T12:34:44Z

exonum/src/sandbox/sandbox_tests_helper.rs

-    ) -> Self {
-        self.duration_since_sandbox_time = Some(duration_since_sandbox_time);
-        self
-    }


Was with_duration_since_sandbox_time() removed because this field was set but never actually used or verified anywhere?

commit message for 7ba7306 will explain this

ponimas · 2018-08-15T13:02:09Z

@ilammy

For example, it's way more common to put [ECR-1234] references at the end of the title (119 vs 4 in git history).

Are there any rules? Cause arguments like "it's way more common..." are not arguments at all.

aleksuss · 2018-08-16T07:05:17Z

@ponimas fix typo pls ./exonum/src/sandbox/consensus/invalid_message.rs:103:24 - Unknown word (commited)

ponimas and others added 18 commits August 8, 2018 20:22

[ECR-1627] [ECR-171] is blocking this TODO

5b8fb43

[ECR-1672] There is no RequestPrecommit anymore. See 15dba13

da7234c

[ECR-1627] Dead code removal

d2d7c24

[ECR-1627] single validator network test

4b3f1f6

[ECR-1627] Review fix

ec18e8c

[ECR-1627] Review fix

eb6c3b8

[ECR-1627] test is passing so we don't need logger

b2846d2

[ECR-1627] allowing dead code

ff7c415

[ECR-1627] single validator network test

4d1fee1

[ECR-1627] allowing dead code

ddad15c

[ECR-1627] logger stderr to stdout

08a3c61

Add endpoint for 'rebroadcasting' transactions [ECR-1716] (exonum#859)

cc48c25

[ECR-1627] we don't have time in block since 47c7c09

7ba7306

[ECR-1627] handle_precommit_remove_propose_request sandbox test

9cf0cdd

Merge remote-tracking branch 'exonum/master' into 1627.3

5c87934

[ECR-1627] fmt

e5b482a

[ECR-1627] removed comment about implemented test

4e542d9

ponimas force-pushed the 1627.3 branch from 5aa7138 to e4709aa Compare August 13, 2018 12:55

ponimas added the work in progress 🚧 Incoming changes expected: pull request isn't ready for the final review. label Aug 13, 2018

[ECR-1627] fix

a22d0a7

ponimas force-pushed the 1627.3 branch 2 times, most recently from 2664441 to 3eb2407 Compare August 14, 2018 14:16

[ECR-1627] tests are ready

cf5be77

ponimas force-pushed the 1627.3 branch from 3eb2407 to cf5be77 Compare August 14, 2018 14:34

ponimas removed the work in progress 🚧 Incoming changes expected: pull request isn't ready for the final review. label Aug 14, 2018

ponimas added 2 commits August 14, 2018 17:35

Merge branch 'master' into 1627.3

4403246

[ECR-1627] lints fix

7c9025b

ilammy previously approved these changes Aug 15, 2018

View reviewed changes

[ECR-1627] spelling fix

c307d62

ponimas dismissed ilammy’s stale review via c307d62 August 15, 2018 12:59

[ECR-1627] review fixes

376c85a

ilammy previously approved these changes Aug 16, 2018

View reviewed changes

aleksuss previously approved these changes Aug 16, 2018

View reviewed changes

[ECR-1627] typo fix

67fb44d

ponimas dismissed stale reviews from aleksuss and ilammy via 67fb44d August 16, 2018 07:08

ilammy approved these changes Aug 16, 2018

View reviewed changes

Merge branch 'master' into 1627.3

b97f06a

aleksuss approved these changes Aug 20, 2018

View reviewed changes

aleksuss merged commit c4624cc into exonum:master Aug 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ECR-1627] TODO cleanup pt.4 #869

[ECR-1627] TODO cleanup pt.4 #869

ponimas commented Aug 13, 2018 •

edited

Loading

vldm commented Aug 15, 2018

ponimas commented Aug 15, 2018

ilammy left a comment

ilammy Aug 15, 2018

ilammy Aug 15, 2018

ponimas Aug 15, 2018

ilammy Aug 15, 2018

ilammy Aug 15, 2018

ilammy Aug 15, 2018

ponimas Aug 15, 2018

ponimas commented Aug 15, 2018

aleksuss commented Aug 16, 2018

[ECR-1627] TODO cleanup pt.4 #869

[ECR-1627] TODO cleanup pt.4 #869

Conversation

ponimas commented Aug 13, 2018 • edited Loading

vldm commented Aug 15, 2018

ponimas commented Aug 15, 2018

ilammy left a comment

Choose a reason for hiding this comment

ilammy Aug 15, 2018

Choose a reason for hiding this comment

ilammy Aug 15, 2018

Choose a reason for hiding this comment

ponimas Aug 15, 2018

Choose a reason for hiding this comment

ilammy Aug 15, 2018

Choose a reason for hiding this comment

ilammy Aug 15, 2018

Choose a reason for hiding this comment

ilammy Aug 15, 2018

Choose a reason for hiding this comment

ponimas Aug 15, 2018

Choose a reason for hiding this comment

ponimas commented Aug 15, 2018

aleksuss commented Aug 16, 2018

ponimas commented Aug 13, 2018 •

edited

Loading