-
Notifications
You must be signed in to change notification settings - Fork 268
Conversation
Started implementing commit() low level API
Moved dispatch_action_and_wait() to instance.rs genesis WIP in nucleus InitApplication reduce()
Calling Genesis loop in init WIP Added UML-class.png Added more Error types Refactoring ExecuteZome for better Error returns
Fixed tests
Handled genesis return value Added genesis tests Refactored testing in core/src/lib.rs
Conflicts: core/src/lib.rs core/src/nucleus/mod.rs core/src/nucleus/ribosome.rs
Using cargo.lock from develop
@@ -1,447 +0,0 @@ | |||
[[package]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't forget to clean this up before merging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that @lucksus added this file in develop 3 days ago, probably by mistake. I'm erasing it here. So I'm not sure what you are asking. Should I leave it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh! yes i see, good job then 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! Ooops.. sorry. Yes, this needs deleted..
core/src/lib.rs
Outdated
use std::fs::File; | ||
|
||
pub fn create_wasm_from_file(fname: &str) -> Vec<u8> { | ||
use std::io::prelude::*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this normal, sticking a use
inside a fn?
core/src/lib.rs
Outdated
r#" | ||
use std::fs::File; | ||
|
||
pub fn create_wasm_from_file(fname: &str) -> Vec<u8> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you put some doc comments ///
on each fn?
core/src/lib.rs
Outdated
use std::io::prelude::*; | ||
let mut file = File::open(fname).unwrap(); | ||
let mut buf = Vec::new(); | ||
file.read_to_end(&mut buf).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unwrap()
panics, right? i think we're trying to avoid that for recoverable errors (e.g. the file might not exist)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in tests. We want tests to panic.
And anyway those unwraps are already in develop. They come from PR#44.
core/src/lib.rs
Outdated
.unwrap(); | ||
nucleus::ribosome::RESULT_OFFSET | ||
); | ||
let wat_str = match wat { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think unwrap_or
could help here https://doc.rust-lang.org/std/option/enum.Option.html#method.unwrap_or
core/src/lib.rs
Outdated
.canonicalize_lebs(false) | ||
.write_debug_names(true) | ||
.convert(wat_str) | ||
.unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unwrap could be bad because of panic
core/src/lib.rs
Outdated
} | ||
|
||
|
||
// Prepare valid DNA struct with that WASM in a zome's capability |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doc comment is triple slash ///
core/src/lib.rs
Outdated
} | ||
|
||
|
||
// Prepare valid DNA struct helper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's this? still needed?
Moved WAT for testing from test_utils to tests Added FromStr trait to Reserved Enums as suggested by clippy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! The nucleus section is pretty huge but I guess we'll have to stick with it for now
core/src/lib.rs
Outdated
capability.code = DnaWasm { code: wasm }; | ||
zome.name = "test_zome".to_string(); | ||
zome.name = zome_name.to_string(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the to_string()
call here is redundant.
core/src/lib.rs
Outdated
} | ||
Err(_) => assert!(false), | ||
_ => { assert!(false) }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need the {
}
core/src/nucleus/mod.rs
Outdated
self.status.clone() | ||
} | ||
pub fn has_initialized(&self) -> bool { | ||
self.status == NucleusStatus::Initialized | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be empty lines between the function definitions.
core/src/nucleus/mod.rs
Outdated
impl EntrySubmission { | ||
pub fn new<S>(zome_name: S, type_name: S, content: S) -> Self | ||
where | ||
S: Into<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good but a little too verbose. What about:
pub fn new<S: Into<String>>(zome_name: S, type_name: S, content: S) -> Self { ...
Improved some comments Trying --warn clippy
Makefile
Outdated
@@ -27,7 +27,7 @@ C_BINDING_CLEAN = $(foreach dir,$(C_BINDING_DIRS),$(dir)Makefile $(dir).qmake.st | |||
# apply formatting / style guidelines, and build the rust project | |||
main: | |||
cargo fmt | |||
cargo clippy -- --deny clippy | |||
cargo clippy -- --warn clippy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would you do that? We want the process to fail if it doesn't comply
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to be able to put returns when it makes the code more understandable and clippy is preventing me from doing that, so I'm trying to make clippy less annoying. To my knowledge, there is no agreement to what the code is suppose to comply to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only one tiny change (other than the others requested) about linking the println to #21.
All in all quite a bunch of great code using the redux infrastructure. Thanks!
core/src/lib.rs
Outdated
assert_eq!(instance.state().nucleus().initialized(), true); | ||
// Wait for Init to finish | ||
while instance.state().history.len() < 2 { | ||
println!("Waiting... {}", instance.state().history.len()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This println!
should be converted to either a call to the app logger, or to the core debug log. For now we can just add a TODO that references #21.
core/src/lib.rs
Outdated
|
||
// Wait for Init to finish | ||
while instance.state().history.len() < 4 { | ||
println!("Waiting... {}", instance.state().history.len()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same...
if has_succeeded { | ||
(*nucleus_state).status = NucleusStatus::Initialized | ||
} else { | ||
*nucleus_state = NucleusState::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I totally understand what you were thinking here, but it seems like if the initialization didn't succeed we can't just reset the state, to uninitialized, because a client of the state object actually might want to know that the initialization failed and distinguish between these states.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thought was that if init fails, then undo everything that was done in the InitApplication Action reducer, which basically means resetting the state. In the absence of clarity for the whole App init procedure (#86) it seemed like the safest thing to do for now. I agree about needing some callback to notify that the init failed. I am not satisfied with the wait loop on history counts.
Made clippy less annoying.
Codecov Report
@@ Coverage Diff @@
## develop #77 +/- ##
===========================================
- Coverage 82.26% 79.35% -2.91%
===========================================
Files 13 13
Lines 451 499 +48
===========================================
+ Hits 371 396 +25
- Misses 80 103 +23
Continue to review full report at Codecov.
|
Conflicts: core/src/lib.rs core/src/nucleus/mod.rs core/src/nucleus/ribosome.rs core/src/state.rs core_api/src/lib.rs dna/src/lib.rs
I do agree with @sphinxc0re that we will have to refactor |
Yes, we can easily pull out functions for each Action reducer like I did for the |
This branch is getting huge so I'll not finish all three functions here.
I did lay out some work for COMMIT and VALIDATE, but this branch is focused on GENESIS
Genesis is more complex than expected because it is called during the InitApplication Action and generates more Actions to complete. Thus Init is finished asynchronously so I added a NucleusStatus member to properly keep track of that.
Here is what a stack of Actions might look like when calling InitApplication on a app with two Zomes with each one there genesis() that commits something to the source chain.
I copied some of Instance's methods out of Instance because those methods cannot be called in NucleusState because there is no instance object available.
What's missing in my current implementation is handling error cases where not all of the Zomes have done their genesis() for some reason and we would want a second call to InitApplication to complete the Init and do genesis() only on the remaining Zomes.