-
Notifications
You must be signed in to change notification settings - Fork 267
capabilities-2: separate from zome function declaration #791
Conversation
this removes duplicate naming in DNA
renamed "main" capability to "public" renamed "main" function to "public_test_function"
#[serde(default)] | ||
pub functions: Vec<FnDeclaration>, | ||
pub functions: Vec<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.
function_names
maybe clearer?
|
||
/// Represents an trait definition for bridging | ||
#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Hash)] | ||
pub struct Trait { |
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.
So here's my understanding from the code: Trait
is a lot like a Capability
, but the only difference is that Trait
contains the full function signature because it's referring to functions that might exist in other DNAs, whereas Capability
is entirely inward-facing, and only holds the function names because it can just look up the function signatures from its own DNA. Is that right?
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.
Yep... Trait is what's used at the composition level of the container for bridging. Maybe we use a different name for it: https://realtimeboard.com/app/board/o9J_kyiXmFs=/?moveToWidget=3074457346285220939
|
||
/// "functions" array | ||
#[serde(default)] | ||
pub functions: Vec<FnDeclaration>, |
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.
should this be called fn_declarations
also?
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.
So the reason I didn't call it declarations here is that it isn't where the functions are declared, its where the trait is saying that "I need a function that looks like this". There's only one place where it's declared, and that's in the zome. Possible we could make a distinctions between declared and defined... I think I'm pushing this to our DND session.
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.
Verreh nice.
I just have a few little naming questions for consistency. Also seeking clarity on the difference between Trait and Capability. Is it like, if the Trait is the "socket", then the Capability is the "plug"? Also does a Trait have to fully cover a Capability in order for it to match, i.e. can a Trait have a subset of functions in a Capability and still be a match? Or does it have to be one-to-one?
/// A Container can be initialized either by: | ||
/// - an Object representation of a Configuration struct | ||
/// - a string representing TOML | ||
pub class JsContainer for NodeContainer { |
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 this duplication of node Container? js_test_container.rs still exists.
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 didn't add that, I just made things work for what was there.
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.
Was this file renamed from container.rs
to js_test_container.rs
in develop and accidentally added here after a merge, @zippy?
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.
Yeah makes sense, scenario-api renamed this file, and this PR spans that merge. We'll have to clean that up.
So, @maackle, to answer your Trait vs Capability question. It's really two separate concerns that we probably need to clarify because they appear to overlap. Capabilities are strictly for the security concern. The identify the kind of capability token that's necessary to call sets of functions. Traits are for composibility. They identify groups of functions (plus signatures) that you may want to declare as existing. It may be the case that these overlap, but note that in this PR I'm mostly trying to land the security concern not the composibility concern. |
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.
Wow. Lot's of good stuff. Thanks for taking on this chore, @zippy!
I would prefer the rename fn_declarations
-> functions
and functions
-> functions_names
, but don't consider this blocking.
The reason I am requesting changes is I think this file nodejs_container/native/src/container.rs got added accidentally. That should be removed before merging into develop.
@@ -79,6 +79,10 @@ pub struct Zome { | |||
#[serde(default)] | |||
pub capabilities: ZomeCapabilities, | |||
|
|||
/// An array of functions declared in this this zome. | |||
#[serde(default)] | |||
pub fn_declarations: ZomeFnDeclarations, |
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.
Hm, I remember that the field functions
in Capability once was called fn_declarations
as well and we renamed it to functions
.
I would actually prefer if this was functions
, and functions in ZomeCapabilities would be renamed function_names
as @maackle suggested above.
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.
pub fn_declarations: ZomeFnDeclarations, | |
pub functions: ZomeFnDeclarations, |
/// A Container can be initialized either by: | ||
/// - an Object representation of a Configuration struct | ||
/// - a string representing TOML | ||
pub class JsContainer for NodeContainer { |
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.
Was this file renamed from container.rs
to js_test_container.rs
in develop and accidentally added here after a merge, @zippy?
ok, spuriously added file deleted. Re naming, I'd like to deal with this in our DND session, because I asked a few other people and got a different answer... :-P |
The PR is all pre-work for capabilities fully working which means tokens being passed in an checked. To get there we first need to:
define_zome!
macro to separate capability definition from function declarations eg:Public
capability type, i.e. if you don't declare your functions as public, you'll see aCapabilityCheckFailed
errordefine_zome!
macro to produce a __list_functions() zome function (like the __list_capabilities() functionmain
->public
for capability and frommain
->public_test_fn
for function)