feat: Refactor notary. Detect when running cargo test to conditionally run thread termination code. Setup logger #32

ltfschoen · 2018-04-14T07:05:34Z

Changes in Commit #97f182a:
- Restored Unit Tests that detects whether all threads finish and that it returns () from call to diamond_drops::run(config) for all CLI modes
- Added setter and getter of a TEST environment variable so we can detect when the user is running cargo test and only run certain snippets of the implementation that terminates threads so the tests pass, but does not terminate the threads when TEST is disabled. This saves us having to explicitly set environment variables at the command line
- All tests passing, including 3x additional tests that verify that this functionality works. If you remove cli::config_env::set_test_env(); from the Unit Tests that are included in main.rs then the tests will not pass
Changes in Commit #c7cba5d:
- Change environment variables to RUST_ENV: TEST or RUST_ENV: DEVELOP for consistency
Changes in Commit #f8cad45:
- Rearranged order of imported files, and struct attributes for consistency
- Add #[derive(Debug)] to enums to allow use with println! with {:?}
- Add meaningful message to panic! calls
- Fix indentation of notary.run();
- Change variable names used in collation/header.rs tests to be more readable and consistent with implementation
- Add id attribute to Notary and Proposer structs for identification when debugging the client, such as knowing what notary id we are storing a collation into
- Add println! for debugging received messages and current thread in SMC listener
- Add generate_notary() function to remove code duplication in notary tests
- Update SMC registration function to be notary or proposer specific, and to return primative boolean value with meaningful success or error logs
- Change get_selected_notaries() function to accept shard_id argument of ethereum_types::U256 instead of usize for consistency withe the rest of the code
- Add register_proposer_address() since there will be both a notary registry and a proposer registry in the SMC Contract according to the Phase 1 Sharding spec
- Change notary test function names to be more explicit

…nation code * Restored Unit Tests that detects whether all threads finish and that it returns `()` from call to `diamond_drops::run(config)` for all CLI modes * Added setter and getter of a TEST environment variable so we can detect when the user is running `cargo test` and only run certain snippets of the implementation that terminates threads so the tests pass, but does not terminate the threads when TEST is disabled. This saves us having to explicitly set environment variables at the command line * All tests passing, including 3x additional tests that verify that this functionality works. If you rem ove `cli::config_env::set_test_env();` from the Unit Tests that are included in main.rs then the tests w ill not pass

…nts" * Rearranged order of imported files, and struct attributes for consistency * Add `#[derive(Debug)]` to enums to allow use with `println!` with `{:?}` * Add meaningful message to `panic!` calls * Fix indentation of `notary.run();` * Change variable names used in collation/header.rs tests to be more readable and consistent with implementation * Add `id` attribute to `Notary` and `Proposer` structs for identification when debugging the client, such as knowing what notary id we are storing a collation into * Add `println!` for debugging received messages and current thread in SMC listener * Add `generate_notary()` function to remove code duplication in notary tests * Update SMC registration function to be notary or proposer specific, and to return primative boolean value with meaningful success or error logs * Change `get_selected_notaries()` function to accept `shard_id` argument of `ethereum_types::U256` instead of `usize` for consistency withe the rest of the code * Add `register_proposer_address()` since there will be both a notary registry and a proposer registry in the SMC Contract according to the Phase 1 Sharding spec * Change notary test function names to be more explicit

ChosunOne

These look really good, the only thing is that I think there are a lot of println! statements which I think we should be conservative with. I think we should add some logging configuration and log those messages to a file instead of printing them to the screen (since there may be a lot of them).

Reference: https://rust-lang-nursery.github.io/rust-cookbook/logging.html#logging

jamesray1 · 2018-04-16T00:12:42Z

src/cli/config_env.rs

@@ -0,0 +1,40 @@
+use std::env;


I think you need to add

#[macro_use] extern crate log;

It's ok, we only have to include #[macro_use] extern crate log; in main.rs and lib.rs

jamesray1 · 2018-04-16T00:30:17Z

src/cli/config_env.rs

+    match env::var(key) {
+        Ok(val) => {
+            debug!("Found environment variable key {}: {:?}", key, val);
+            val.to_string()


We may want to read the debug! log to a target.

https://docs.rs/log/0.4.1/log/macro.debug.html

Nevermind, it's OK to read to the terminal, a developer can use <command> -> logfile.log.

Do we need to have val.to_string()? It doesn't look like this serves any purpose.

jamesray1 · 2018-04-16T00:44:14Z

src/cli/config_log.rs

@@ -0,0 +1,39 @@
+use log;


I think you also need to add this here as well:

#[macro_use] extern crate log;

…minal

ltfschoen · 2018-04-16T02:08:51Z

Changes in commits #f94d253, #e71e58c, #2733ead, #1c44cea

Setup Log and Fern crates to output log to both stdout and log file
Logs include timestamps, module info, and message
Different levels of verbosity may be set for logging
Configure log file to be gitignored
Optional extras later may be to integrate flags from CLI https://github.com/daboross/fern/blob/master/examples/cmd-program.rs

jamesray1 · 2018-04-16T05:23:17Z

src/cli/config_log.rs

+            .level(LevelFilter::Debug)
+            .level_for("overly-verbose-target", LevelFilter::Info),
+        2 => base_config.level(LevelFilter::Debug),
+        _3_or_more => base_config.level(LevelFilter::Trace),


How does _3_or_more work? I mean, how does it know to match this expression if the value is 3 or more?

I got it from this Fern example and assumed it worked the same way as the _ catch all since it compiles. I'll change it to _ which I'm more comfortable with too!

OK, yeah it's not exactly clear from the example why it is that way, so it's probably better to use _.

jamesray1 · 2018-04-16T05:29:42Z

src/collation/header.rs

        sha3.update(&period_bytes[..]);
-        sha3.update(&proposer_bytes[..]);
+        sha3.update(&proposer_address_bytes[..]);


Thanks for renaming the variables.

… instead of u256_to_bytes32

ltfschoen · 2018-04-16T05:51:17Z

@jamesray1 I think you forgot to run cargo test before pushing commit #361dfc3 to this PR, as your changes have broken the tests. They appear to be incomplete (i.e. you haven't replaced the calls to u256_to_bytes32 on Line 41 and 57 in header.rs, but you've commented out the function u256_to_bytes32). Could you create a separate PR from your fork of 'notary' with your commit directly into the 'notary' branch and have @ChosunOne see if he can get it to work?

jamesray1 · 2018-04-16T06:00:54Z

src/main.rs

        process::exit(1);
    });

    diamond_drops::run(config);
 }
+
+#[cfg(test)]
+mod tests {


It seems a bit odd to put these tests in main.rs and not in /tests, but I guess it's not a bit deal. After all, integration tests are meant to test how parts of your library work together.

Yeh it compiles, we can refactor it later

…unction so tests pass

jamesray1 · 2018-04-16T06:55:26Z

I'm looking into it.

Reference: https://github.com/daboross/fern/blob/master/examples/pretty-colored.rs

ltfschoen · 2018-04-16T06:58:14Z

Changes in commits #db0cf58:

Restored use of u256_to_bytes32 as commit #361dfc3 broke the tests

Changes in commits #6ff0d02, #fff7116

Setup Fern library logger so it outputs a different colour for each log level in stdout

jamesray1 · 2018-04-16T08:09:09Z

src/cli/config_log.rs

+        .format(move |out, message, record| {
+            out.finish(format_args!(
+                "{color_line}[{date}][{target}][{level}{color_line}] {message}\x1B[0m",
+                color_line = format_args!("\x1B[{}m", colors_line.get_color(&record.level()).to_fg_str()),


What's \x1B[{}m and to_fg_str()?

to_fg_str(): https://docs.rs/colored/1.6.0/src/colored/color.rs.html#25-44

\x1B[{}m: http://jafrog.com/2013/11/23/colors-in-terminal.html

jamesray1 · 2018-04-16T08:25:51Z

src/notary.rs

                    match msg {
                        client_thread::Command::Terminate => { break }
                    }
                },
-                None => { }
+                None => {
+                     trace!("No more pending messages from other threads to thread {:?} or channel hung up", thread::current());


Can we keep lines to within 100 characters?

https://internals.rust-lang.org/t/99-vs-100-column-limit/47/6

Perhaps even to 79 characters.

Also pinging @ChosunOne.

jamesray1 · 2018-04-16T08:34:57Z

src/notary.rs

        self.collation_vectors.entry(self.shard_id).or_insert(vec![]);
        let vector = self.collation_vectors.get_mut(&self.shard_id).unwrap();
        vector.push(collation);
    }

    fn store_proposal(&mut self, proposal: collation::Collation) {
+        debug!("Storing in notary id {} a new proposal collation mapped to shard id {}", self.id, self.shard_id);


Hmm, maybe we can change this to "a new proposal mapped...". Although this function takes the same argument as store_collation().

Yeah obviously we will need to add more detail to Collation:body, but that isn't defined in a common spec yet, so we can just mark this as TODO.

jamesray1

Reduce line lengths to 100 characters.

jamesray1

Please add documentation comments.

https://rust-lang-nursery.github.io/api-guidelines/documentation.html
https://doc.rust-lang.org/book/first-edition/documentation.html

It will be easier to do this as we go, rather than having to backtrack later on.

Also pinging @ChosunOne to do the same.

jamesray1 · 2018-04-16T09:01:11Z

src/cli/config_env.rs

+    match env::var(key) {
+        Ok(val) => {
+            debug!("Found environment variable key {}: {:?}", key, val);
+            val.to_string()


Do we need to have val.to_string()? It doesn't look like this serves any purpose.

jamesray1 · 2018-04-16T09:01:27Z

src/cli/config_env.rs

+        },
+        Err(e) => {
+            error!("Error interpreting environment variable key {}: {}", key, e);
+            "".to_string()


Do we need to have "".to_string()? It doesn't look like this serves any purpose.

ChosunOne · 2018-04-16T12:45:16Z

I think the changes made here are fine to merge into my notary branch, we can address any remaining issues when it merges into develop.

feat: Refactor notary. Detect when running cargo test to conditionally run thread termination code. Setup logger

ltfschoen requested review from ChosunOne and jamesray1 April 14, 2018 07:05

ltfschoen added 3 commits April 14, 2018 17:08

fix: Convert to standard environment vars naming for dev and test

c7cba5d

ltfschoen changed the title ~~fix: Detect when running cargo test to conditionally run thread termination code~~ feat: Refactor notary. Detect when running cargo test to conditionally run thread termination code. Apr 15, 2018

ChosunOne reviewed Apr 15, 2018

View reviewed changes

ltfschoen added 2 commits April 16, 2018 08:05

docs: Setup Log crate

f94d253

feat: Add Timestamp to Logs

e71e58c

Reference: https://rust-lang-nursery.github.io/rust-cookbook/logging.html#logging

jamesray1 reviewed Apr 16, 2018

View reviewed changes

ltfschoen added 2 commits April 16, 2018 11:29

feat: Add Fern Crate to save log to gitignored file and output to ter…

2733ead

…minal

refactor: Switch entirely to Fern logger to stdout and log file

1c44cea

jamesray1 reviewed Apr 16, 2018

View reviewed changes

ltfschoen changed the title ~~feat: Refactor notary. Detect when running cargo test to conditionally run thread termination code.~~ feat: Refactor notary. Detect when running cargo test to conditionally run thread termination code. Setup logger Apr 16, 2018

Add indentations for address arrays, and ethereum_types::U256::as_u32…

361dfc3

… instead of u256_to_bytes32

jamesray1 reviewed Apr 16, 2018

View reviewed changes

ltfschoen added 2 commits April 16, 2018 16:26

fix: Revert changes in previous commit by restoring u256_to_bytes32 f…

db0cf58

…unction so tests pass

feat: Add Fern colours to the log level text shown in stdout

6ff0d02

feat: Add Fern logger pretty colours to stdout

fff7116

Reference: https://github.com/daboross/fern/blob/master/examples/pretty-colored.rs

ltfschoen mentioned this pull request Apr 16, 2018

Mismatched type returned from log_file daboross/fern#26

Closed

jamesray1 reviewed Apr 16, 2018

View reviewed changes

jamesray1 suggested changes Apr 16, 2018

View reviewed changes

Merge branch 'notary' into notary

b913550

ChosunOne merged commit 929614e into Drops-of-Diamond:notary Apr 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Refactor notary. Detect when running cargo test to conditionally run thread termination code. Setup logger #32

feat: Refactor notary. Detect when running cargo test to conditionally run thread termination code. Setup logger #32

ltfschoen commented Apr 14, 2018 •

edited

Loading

ChosunOne left a comment

jamesray1 Apr 16, 2018

ltfschoen Apr 16, 2018

jamesray1 Apr 16, 2018

jamesray1 Apr 16, 2018

jamesray1 Apr 16, 2018

jamesray1 Apr 16, 2018

jamesray1 Apr 16, 2018

ltfschoen commented Apr 16, 2018

jamesray1 Apr 16, 2018 •

edited

Loading

ltfschoen Apr 16, 2018

jamesray1 Apr 16, 2018 •

edited

Loading

jamesray1 Apr 16, 2018

ltfschoen commented Apr 16, 2018 •

edited

Loading

jamesray1 Apr 16, 2018 •

edited

Loading

ltfschoen Apr 16, 2018

jamesray1 commented Apr 16, 2018

ltfschoen commented Apr 16, 2018

jamesray1 Apr 16, 2018

jamesray1 Apr 16, 2018

jamesray1 Apr 16, 2018 •

edited

Loading

jamesray1 Apr 16, 2018 •

edited

Loading

jamesray1 Apr 16, 2018 •

edited

Loading

jamesray1 Apr 16, 2018

jamesray1 left a comment

jamesray1 left a comment

jamesray1 Apr 16, 2018

jamesray1 Apr 16, 2018

ChosunOne commented Apr 16, 2018

feat: Refactor notary. Detect when running cargo test to conditionally run thread termination code. Setup logger #32

feat: Refactor notary. Detect when running cargo test to conditionally run thread termination code. Setup logger #32

Conversation

ltfschoen commented Apr 14, 2018 • edited Loading

ChosunOne 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

ltfschoen commented Apr 16, 2018

jamesray1 Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesray1 Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltfschoen commented Apr 16, 2018 • edited Loading

jamesray1 Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesray1 commented Apr 16, 2018

ltfschoen commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesray1 Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

jamesray1 Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

jamesray1 Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesray1 left a comment

Choose a reason for hiding this comment

jamesray1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChosunOne commented Apr 16, 2018

ltfschoen commented Apr 14, 2018 •

edited

Loading

jamesray1 Apr 16, 2018 •

edited

Loading

jamesray1 Apr 16, 2018 •

edited

Loading

ltfschoen commented Apr 16, 2018 •

edited

Loading

jamesray1 Apr 16, 2018 •

edited

Loading

jamesray1 Apr 16, 2018 •

edited

Loading

jamesray1 Apr 16, 2018 •

edited

Loading

jamesray1 Apr 16, 2018 •

edited

Loading