-
-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
v1.0-rc.1 #6
base: quinn-wip-rework
Are you sure you want to change the base?
v1.0-rc.1 #6
Conversation
flake.nix
Outdated
name = "docs:build"; | ||
help = "Refresh the docs"; | ||
category = "dev"; | ||
command = "${cargo} doc --features=mermaid_docs"; |
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 features
is an annoying workaround since it's duplicated in the Cargo.toml
, but AFAICT the pkgs.cargo
here and cargo
in the shell are different cargo
s... my guess is that the shell one probably one comes from the toolchain, but I haven't debugged it much yet
Cargo.toml
Outdated
documentation = "https://docs.rs/rs-ucan" | ||
repository = "https://github.com/ucan-wg/rs-ucan" | ||
authors = ["Quinn Wilton <quinn@quinnwilton.com>"] | ||
authors = ["Quinn Wilton <quinn@quinnwilton.com>", "Brooklyn Zelenka <hello@brooklynzelenka.com"] |
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.
we have matching websites 🙈
src/ability/msg.rs
Outdated
|
||
impl From<MsgReceive> for Ipld { | ||
fn from(msg: MsgReceive) -> Self { | ||
let mut map = BTreeMap::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.
Nitpick: for cases like this where the set of keys is statically known, it can sometimes be a little less verbose to use the FromIterator
trait to construct maps, and avoid needing to mutate the map key-by-key:
BTreeMap::from_iter([
("to".to_string(), msg.to.to_string().into()),
("from".to_string(), msg.from.to_string().into())
]).into()
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.
Great tip! Thanks :)
I'd call this more of a protoip than a nitpick, even 😁
src/ability/msg.rs
Outdated
} | ||
} | ||
|
||
impl TryFrom<&Ipld> for MsgReceiveBuilder { |
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.
Rather than manually writing this implementation, if you'd like, you can probably use libipld_core
with the serde-codec
feature to derive Deserialize
on MsgReceive
, and then use IntoDeserializer
to do MsgReceive::deserialize(some_ipld.into_deserializer())
.
Something like this:
use serde::de::{Deserialize, IntoDeserializer};
use serde_derive::Deserialize;
#[derive(Debug, Clone, PartialEq, Eq, Deserialize)]
pub struct MsgReceive {
to: Url,
from: Url,
}
impl TryFrom<&Ipld> for MsgReceiveBuilder {
type Error = ();
fn try_from(ipld: &Ipld) -> Result<Self, ()> {
MsgReceive::deserialize(some_ipld.into_deserializer()).map_err(|_| ())
}
}
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 trait Deserialize<'_> is not implemented for Url
Womp womp
But good tip in general! I guess I can newtype Url
, but it's some code either way
UPDATE
27 | pub struct MyUrl(pub Url);
| ^^^ the traitDeserialize<'_>
is not implemented forUrl
Welp. I guess I'm learning how to write a deserializer 😛
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.
@expede are you importing the url lib w/ the serde feature?
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.
@zeeshanlakhani good point! Ugh, I really dislike how invisible those are in tooling.
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 I learned:
libipld-core = { version = "0.16", features = ["serde-codec"] }
use libipld_core::{ipld::Ipld, serde as ipld_serde};
// ...
#[derive(Debug, Clone, PartialEq, Eq, Deserialize)]
pub struct MsgReceive {
to: Url,
from: Url,
}
#[derive(Debug, Clone, PartialEq, Eq, Deserialize)]
pub struct MsgReceiveBuilder {
to: Option<Url>,
from: Option<Url>,
}
// ...
impl TryFrom<Ipld> for MsgReceiveBuilder {
type Error = ();
fn try_from(ipld: Ipld) -> Result<Self, ()> {
ipld_serde::from_ipld(ipld).map_err(|_| ())
// ^^^^^^^^^^^^^^^^^^^^^
// this exists, but only on owned Ipld
}
}
src/invocation.rs
Outdated
pub subject: DID, | ||
pub audience: Option<DID>, | ||
|
||
pub ability: B, |
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 without a task/instruction (not being steadfast about layers here per se, but Task in 0.2 spec), this structure, as is shown and implemented, makes it difficult to have an index to match on a reusable key for replayability. Going off previous versions, this reusable key was composed of:
rsc
input
/args/etcop
/(old) ability string, i.e. wasm/run, crud/etcnonce
(nnc)
The nonce as part of this was handy b/c it could separate one instruction/task from the other if you so chose to. With 1.0, rsc
is now inside ability/run/arguments, which is fine. But, calculating this generic task "index" is key, and nonce should be a part of it?
As per the 1.0 spec, A Task MUST be uniquely defined by the following fields:
Subject
Command
Arguments
Nonce
For reusability, the sub
subject may not be handy here, but the rest are. If we don't want this represented within the structure, then it needs to be something that can be rendered from it (function with a known name quantity that we use).
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.
makes it difficult to have an index to match on a reusable key for replayability.
Yeah, we're going to have to calculate our own. This is better for a few reasons, but the two bigs ones are:
- We can pick the serialisation
- Getting people to respect the nonce parameter for idempotent actions was going to be hard
Previously, if two people submitted the same invocation, but one was DAG-CBOR and the other was DAG-JSON, we wouldn't have the same index. We can also determine if we want to include the nonce or not based on metadata about the task being run.
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 reusability, the sub subject may not be handy here, but the rest are
It depends. It doesn't matter for pure Wasm, but it does as soon as effects show up (i.e. calling the same function on two agents can yield different results)
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.
As per the 1.0 spec, A Task MUST be uniquely defined by the following fields:
Sorry, that's probably my bad. All (100%) of the indexing stuff should be removed from the invocation spec. It's an IPVM concern, not a UCAN one.
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 reusability, the sub subject may not be handy here, but the rest are
It depends. It doesn't matter for pure Wasm, but it does as soon as effects show up (i.e. calling the same function on two agents can yield different results)
Right. I guess, it's not something you always want in the calculation is what I want to say.
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.
makes it difficult to have an index to match on a reusable key for replayability.
Yeah, we're going to have to calculate our own. This is better for a few reasons, but the two bigs ones are:
- We can pick the serialisation
- Getting people to respect the nonce parameter for idempotent actions was going to be hard
Previously, if two people submitted the same invocation, but one was DAG-CBOR and the other was DAG-JSON, we wouldn't have the same index. We can also determine if we want to include the nonce or not based on metadata about the task being run.
yeah, I think the key is dealing with duplicate tasks once again, and that nonce has meaning there if you want to run the same thing (for whichever reason).
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 trying to remember back to when this was written, but we probably usually (always?) do want the subject in there, because ultimately they're the one that set the semantics
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. It wasn't part of the original task/instruction in 0.1/0.2. It was there, but not included. If it's the case that sub (in like the wasm case) will typically be the same across runs (and machines), then that's fine. Mainly, what works for reusability when we want to allow that.
src/ability/msg.rs
Outdated
from: Url, | ||
} | ||
|
||
#[derive(Debug, Clone, PartialEq, Eq)] |
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.
as per your video, we can remove these via https://crates.io/crates/optional_struct, so you can generate optional versions.
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's also builder_macro
... any reason to prefer optional_struct
?
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 mean https://docs.rs/derive_builder/latest/derive_builder/? Yeah, I'd say the latter is more explicit about the Option version. derive_builder
actually has different use cases. So, I'd say it's about intention.
tests/conformance.rs
Outdated
@@ -2,7 +2,7 @@ use libipld_core::{ipld::Ipld, raw::RawCodec}; | |||
use serde::{Deserialize, Serialize}; | |||
use std::{collections::HashMap, fs::File, io::BufReader, str::FromStr}; | |||
|
|||
use rs_ucan::{ | |||
use rs-ucan::{ |
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.
minor: I can't find @matheus23's comments now, but while the internal use of crate names remains unclear
, snake_case
still seems to be the convention (dash for dep of crate, _ for internal use).
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.
FWIW the original rs-ucan crate uses the ucan
crates.io name.
Also rs-wnfs
uses the wnfs
crate name.
Not sure why Quinn changed the ucan
crate name into rs_ucan
.
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. I was also going to say we shouldn't rs
everything anyhow.
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 yeah, it's against the community guidelines
Crate names should not use
-rs
or-rust
as a suffix or prefix. Every crate is Rust! It serves no purpose to remind users of this constantly.
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 sure why Quinn changed the ucan crate name into rs_ucan.
It was me being annoyed by mistyping, and the highlighted line is me pushing a global find-and-replace to revert that
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 was me being annoyed by mistyping, and the highlighted line is me pushing a global find-and-replace to revert that
I was referring to the fact that Quinn changed the crate name from ucan
to rs_ucan
, not the fact that you changed rs_ucan
to rs-ucan
:)
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.
Thanks for opening up the discussion on this so early 🤗 It's certainly not the most 'fun' thing to have everyone give their thoughts on something that's in an early phase.
But I think that was a really good decision. It's going to be a fairly central piece of the stack, most likely making its way into the server, homestar, possibly bound into frontend libs, possibly a future dependency of wnfs (submarine sealing), etc.
src/prove.rs
Outdated
pub trait TryProve<'a, T> { | ||
type Error; | ||
type Proven; | ||
|
||
fn try_prove(&'a self, candidate: &'a T) -> Result<&'a Self::Proven, Self::Error>; | ||
} |
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 suspect that this version of the trait won't quite work, or at least I'm confused about how it would.
To write a function that takes an Invocation
and verifies that some A: Ability
can be proven, that function will need to take the proofs: Vec<Cid>
in the Invocation
and start fetching and parsing them to valid Delegation
s.
But what type would you use for the abilities in these Delegation
s?
Likely it'll be the same ability type for all elements in the Vec
, because it needs to be homogenous.
However, trait TryProve<'a, T>
allows self
, the candidate
and return type Self::Proven
to be different types.
Grepping all impl TryProve
s it seems like in most cases you're doing impl TryProve<X> for X
with type Proven = X
.
So perhaps it should just be fn try_prove(&self, candidate: &Self) -> Result<Self, Self::Error>
?
The only exception is the Crud
ability. In that case it looks like there exist e.g. impl TryProve<Crud> for CrudRead
with type Proven = CrudRead
. I'm not sure exactly what you're trying to solve, but my gut reaction would be to simply have an enum Crud { CrudAll, CrudRead, CrudWrite, ... }
.
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.
Ah, it does not take an invocation! It only takes Abilities (command + args). The validation around that just depends on this being implemented for the T
in Invocation<T>
/ Delegation<T>
/ Receipt<T>
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.
Hmm. I'm confused. Let me try to get clarification:
- There will be a
Invocation::<T>::check(&self, ability: impl TryProve<...>, ...) -> Result<...>
function that verifies the given ability is valid in the invocation, right? Or perhaps that's even justInvocation::<T>::check(&self, ...) -> Result<...>
, and it'll just return the ability in the invocation or sth. - The invocation checking code will call the
TryProve
implementations of your abilities, right?
To be clear, I wrote:
To write a function that takes an Invocation and verifies that some A: Ability can be proven
But I wasn't talking about try_prove
as being "that function". I was talking about a hypothetical Invocation::check
function
src/ability/crud.rs
Outdated
pub struct Crud { | ||
uri: Field<Url>, | ||
} | ||
|
||
pub struct CrudRead { | ||
pub uri: Field<Url>, | ||
} | ||
|
||
pub struct CrudMutate { | ||
uri: Field<Url>, | ||
} | ||
|
||
pub struct CrudCreate { | ||
pub uri: Field<Url>, | ||
pub args: BTreeMap<Box<str>, Field<String>>, | ||
} | ||
|
||
pub struct CrudUpdate { | ||
pub uri: Field<Url>, | ||
pub args: BTreeMap<Box<str>, Field<String>>, | ||
} | ||
|
||
pub struct CrudDestroy { | ||
pub uri: Field<Url>, | ||
} |
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 setting a really high bar for type safety, which I commend, but could possibly be hard to realize.
I'm trying to categorize the 'needs' for these different types:
- in Delegations: Needs the ability to "name" the ability (kind of like a function name), and needs to know about which abilities can be derived from others. E.g.
account/info
can be derived fromaccount/noncritical
, butaccount/delete
cannot. No need to know about arguments yet (unless we try to integrate conditions here, too). - in Invocations: Needs not only the ability name, but also values for all arguments. Which abilities derive others is irrelevant. Stuff like
account/noncritical
doesn't even have a defined set of arguments and shouldn't even be usable in an invocation. - in Workflows/Tasks(? I'm fuzzy on the terminology here): Needs ability arguments to have a representation for "deferred". Otherwise fairly similar needs to "in Invocations".
- in Receipts: Needs all arguments to be there, none of them optional nor deferred.
Maybe for now, we'll separate these into let's say two? I.e. require uses to define two structs for each , one for use within Delegation
and one for use within Invocation
/Tasks/Receipts.
I can see a future where we improve the situation with a derive proc-macro like in the future 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.
Yep, this is the "I wish I had HKTs" stuff that we talked about. You'll see the expanded code hopefully later today, but we can also macro-ize it. If it turns out to be too much of a pain, we can drop to a dynamic version, but so far writing this feels more straightforward than perhaps it feels?
This is also all standard lib — I think 99% of of people will just use the stuff off the shelf because they're general enough for most use cases.
But hey, we'll see as this comes together :)
haha yes, I have been somewhat descended upon... thanks for noting that! I also kind of want to model the pattern. It's something that we used to do at Fission, but seems to have stopped (because it's uncomfortable!), buuuuuut it also lets people pipe in with "oh shit I have something for that over here!" or "uh, you probably don't want to do that", which is happening in this thread :) Working in the open! |
src/ability/wasm.rs
Outdated
} | ||
|
||
#[derive(Debug, Clone, PartialEq)] | ||
pub enum Module { |
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.
@expede is more of a spec question. Can't this also be a URL? Or should there be a tag where it may be fetched from, as we do not always know it's a Cid from IPFS for example? So, there needs to be some info somewhere on how to retrieve it. Is it elsewhere and I'm not seeing 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.
Can't this also be a URL?
There's no spec for the wasm ability yet, so we'll define it together!
The way I was thinking about this is that resolving a module for $SOMEWHERE_ELSE
would be captured as an effect so that we can cache the output using the normal machinery. I think the higher level DSL will do things like construct the correct workflow for this.
The other option is to special-case URL resolution specifically for Wasm at this layer, which I don't love — because we're already going to have that functionality in the system with our first-class effects — but I'm open to talking about 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.
Yeah. We should discuss this, as it just needs to be defined somewhere. It's fine if it's outside of the execution (which is how it's done today in the runtime anyway but just pulled from what was rsc
of course).
src/delegation/payload.rs
Outdated
/// given as a [Unix timestamp]. | ||
/// | ||
/// [Unix timestamp]: https://en.wikipedia.org/wiki/Unix_time | ||
pub expiration: Timestamp, |
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 expiration: Timestamp, | |
pub expiration: Option<Timestamp>, |
Shouldn't this be optional? I know it needs to be present, but it can also be set to null
, and Timestamp
doesn't have a representation for null
.
The exp field MUST be set. Following the principle of least authority, it is RECOMMENDED to give a timestamp expiry for UCANs. If the token explicitly never expires, the exp field MUST be set to null. If the time is in the past at validation time, the token MUST be treated as expired and invalid.
* refactor: Make delegation::Store::get return Option * refactor: Make delegation::Store::insert take &self instead of &mut self * refactor: Make invocation::Store take &self instead of &mut self * refactor: Make `delegation::Agent` not take `&mut self` in methods * refactor: Make `Agent` take `DID` by value * refactor: Take `DID` by value in `delegation::Agent::new` * refactor: Change generic order in `delegation::Agent` and add defaults
Test delegation store
Wrap auto CID Insert error
…x-chain Remove unrestricted powerbox chain
This reverts commit c06e6de.
More bugfixes & some suggested changes
Readme Preview
FIXME
sTODO
s