-
Notifications
You must be signed in to change notification settings - Fork 268
Conversation
# Conflicts: # core/src/instance.rs # core/src/nucleus/mod.rs # core/src/nucleus/ribosome/api/print.rs # core/src/nucleus/ribosome/debug.rs # core/src/nucleus/ribosome/mod.rs # core/src/nucleus/ribosome/print.rs # core/src/persister.rs # core_api/src/lib.rs
core/src/nucleus/mod.rs
Outdated
_ => return_initialization_result(None, &action_channel), | ||
} | ||
}) | ||
.collect(); |
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.
.for_each(drop)
might be better. From the docs:
"if you really need to exhaust the iterator, consider `.for_each(drop)` instead"
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.
It seems you have mistaken Lifecycle
with callback
.
Currently, the HC API docs calls functions in the zome that are called by the ribosome callbacks
.
In the new API, Lifecycle
is the name of the capability where callbacks related to an app's lifecycle resides, as described in the refactor doc: https://docs.google.com/document/d/1744ZnaILkFdiiIFNOo0VpS9OEmoAhWgAsu__VLeEfww/edit
This PR implies that there is a different validation function for an entry when it comes from a commit. Is this what we want?
Reviewing this PR is difficult because it mixes a new feature with refactoring where files are moved around. I would appreciate having seperate PRs for feature and refactor.
Also splitting ribosome between api
and lifecycle
seems premature. This is starting to become folder hell. Would like this to be undone and planned in the future when it would actually add value.
For now I would merge everything thats under lifeycle\
in a file named: callbacks.rs
On a side note, I fail to see the coherence about splitting stuff up as much as possible but not splitting up the testing code.
I do like and see the value in all the other refactors in this PR.
I believe all the confusion/misunderstandings comes from a lack of specs.
core/src/action.rs
Outdated
impl Eq for Action {} | ||
|
||
#[derive(Clone, PartialEq, Hash, Debug)] | ||
pub enum Signal { |
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 understand why Action has been renamed Signal, and this will be confusing with actual Signals as described in the event/signal ADR.
Also the new Action struct doesn't seem to serve any purpose.
I would like this to change the following way:
- Delete the current Action struct
- Rename Signal -> Action
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 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 purpose of the changes i made other than naming things is consolidation. i'm working towards:
- a flatter structure overall (e.g. something like what you suggested) so less
match
ing - a single snowflake ID per action dispatch
- a single standard representation for key/values for action/result pairs across all state slices
- a single enum (rather than struct) that defines which types go where
happy to rename Signal
if its confusing, there aren't many good words in this space though
core/src/error.rs
Outdated
} | ||
|
||
impl fmt::Display for HolochainError { | ||
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
write!(f, "{}", self) | ||
write!(f, "{:?}", self) |
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 seems to contradicts fmt::Display's purpose:
Display is similar to Debug, but Display is for user-facing output
Why would HolochainError display work only in Debug?
Doesn't Rust strip this out in optimized code?
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.
@ddd-mtl i don't know the subtleties
the high level is that what was there before blows a stack overflow on to_string()
calls (which is also why i added that in a test below)... some implicit recursion somewhere
if you know the right fix we can drop it in, otherwise i'd be inclined to flag a followup for this
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.
@sphinxc0re any ideas for a real fix here?
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've opened a followup #223
core/src/nucleus/ribosome/api/mod.rs
Outdated
/// Enumeration converts to str | ||
#[repr(usize)] | ||
#[derive(FromPrimitive)] | ||
pub enum ZomeAPIFunction { |
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.
Please don't use CAPS in names, even for acronyms.
Rename to ZomeApiFunction
core/src/nucleus/ribosome/api/mod.rs
Outdated
} | ||
} | ||
|
||
fn str_index(s: &str) -> usize { |
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 function name is confusing to me.
How about get_index
?
core/src/nucleus/ribosome/api/mod.rs
Outdated
|
||
/// Object holding data to pass around to invoked API functions | ||
#[derive(Clone)] | ||
pub struct FunctionRuntime { |
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 understand why you renamed this. What was wrong with Runtime
?
I find FunctionRuntime
misleading because it makes you believe that it has a lifetime of just one ZomeApi function call but that's not true, it has the lifetime of the Wasm call.
How about WasmCallRuntime
?
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.
@ddd-mtl this was part of something i did that i ended up rolling back, i'll change it back
fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
match s { | ||
"genesis" => Ok(LifecycleFunction::Genesis), | ||
"validate_commit" => Ok(LifecycleFunction::Genesis), |
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.
copy-paste error?
LifecycleFunction::ValidateCommit
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.
yar, nice catch
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 dropped some tests in for this
} | ||
} | ||
|
||
fn capabilities(&self) -> ReservedCapabilityNames { |
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.
A callback function can only live in one capability, so capability
would make more sense.
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.
kk
core/src/nucleus/ribosome/api/mod.rs
Outdated
} | ||
} | ||
|
||
fn capabilities(&self) -> ReservedCapabilityNames { |
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.
rename to capability
Also shouldn't this not be implemented, or panic instead?
dna/src/zome/capabilities.rs
Outdated
@@ -20,6 +28,7 @@ impl FromStr for ReservedCapabilityNames { | |||
match s { | |||
"hc_lifecycle" => Ok(ReservedCapabilityNames::LifeCycle), | |||
"hc_web_gateway" => Ok(ReservedCapabilityNames::Communication), | |||
"hc_zome_api_function" => Ok(ReservedCapabilityNames::ZomeAPIFunction), |
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 implies a capability named hc_zome_api_function
which doesn't make any sense in my understanding of the new rust version. Zome Api functions should be callable in any capability, no?
// @see https://github.com/holochain/holochain-rust/issues/200 | ||
|
||
#[derive(FromPrimitive)] | ||
pub enum LifecycleFunction { |
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.
Confusion between lifecycle
and callback
Rename to callback
or list only the functions that belongs the the lifecycle 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.
@ddd-mtl as i'm sure you picked up, i don't know how capabilities work, i cargo culted that in... still waiting on a pair session with @zippy to go over that but maybe you can run me through it too
we've been going back and forward on names like "hook", "zome function", etc.
callback doesn't seem right to me either because they aren't being passed around at runtime, let's have a quick straw poll in our next meeting to see if we can pick something
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 also not comfortable with callback
and agree with what you said. I was just sticking with the names used in the Go version.
# Conflicts: # doc/holochain_101/src/SUMMARY.md
Lifecycle functions are passed some parameters and expected to return one of | ||
three possible `LifecycleFunctionParams` variants: | ||
Callback functions are passed some parameters and expected to return one of | ||
three possible `CallbackParams` variants: | ||
|
||
- `Pass`: The lifecycle function has executed/validated successfully |
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.
lifecycle
-> callback
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.
you can just edit this in github UI... @sphinxc0re
This was a heavy lift! Nice work David! |
let a1 = test_action(); | ||
let a2 = test_action(); | ||
|
||
// unlike actions and wrappers, signals are equal to themselves |
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.
did you mean to say 'unlike signals and wrappers, actions are equal to themselves'?
.history | ||
.iter() | ||
.find(|aw| match aw.action() { | ||
Action::ReturnZomeFunctionResult(_) => true, |
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 should be Action::ReturnZomeInitializationResult
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 too late with reading this PR but I nevertheless responded to @Connoropolous' direct question.
Wow, a lot of great work! Thank you @thedavidmeister!
Every entry must pass a ValidateCommit lifecycle function check before it can be | ||
committed. | ||
|
||
If ValidateCommit is not implemented for the zome committing the entry then this |
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.
Yes, this is contradictory to what I said at some point.
A missing validation function should be treated as not valid.
What we saw (and you can confirm or invalidated, @Connoropolous) a lot with people starting to write Holochain apps with the Go prototype was: not really implementing the validation function and just returning true. But actually that is the most important part of every Holochain app. So: for the current state it is not a problem to have it this way, but we should change this before the next release.
Called internally by the `commit` Zome API function. | ||
|
||
- `Pass`: the entry will be committed | ||
- `NotImplemented`: the entry will be committed |
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.
no, should not be committed.
fixes #97
fixes #176
fixes #171
notes:
invoke_commit
changes:
Pass
orNotImplemented
HolochainError
trait implementation forto_string()
Action
is now global rather than specific to a state slice, e.g. zome can send actions that are reduced by either zome or agentActionResponse
is now reducer module specific, e.g.agent::state::ActionResponse
unwrap_to!
macro to help get at enum wrapped values more easilyDefn
trait that ensures some consistency across lifecycle vs. API function definitionsErrorLifecycleResult
as a zome WASM error codetodo/clarification in this tx:
Signal::ValidateEntry
and how does it relate to validate commit if at all? @ddd-mtlActionWrapper
andAction
? @lucksusfollowups:
ActionResponse
trait standardActionResponse
trait #196test_genesis_err
#197