Add Delegation Builder #20

expede · 2024-04-02T01:04:53Z

We can change the naming, but figured let's do one of these to figure out how we want to manage this pattern going forward. Overall I like where it landed, though I did the boiler-plate-y thing and made the interface extremely OO with some methods on DelegationRequired that implicitly convert to DelegationBuilder without requiring an explicit call to .to_builder().

DelegationRequired {
    subject: Subject::Known(alice),
    issuer: alice,
    audience: bob,
    command: "/".to_string(),

    signer: alice_signer,
    codec: encoding::Preset::DagCbor,
    varsig_header: header::Preset::EdDsa(header::EdDsaHeader {
        codec: encoding::Preset::DagCbor,
    })
}
.with_via(carol)
.with_policy(vec![])
.with_metadata(BTreeMap::from_iter([("foo".into(), 123.into())]))
.try_finalize()

I attempted to automatically build the varsig header from the signer + codec, but that won't work super well once we have more than the simplest of signature types, so I reverted those changes.

This code also uses the more common by-ownership version of builder, which lets you easily chain the calls. I previously has this as a series of &muts, but it was less ergonomic; it can be useful in some cases, but I doubt that we'll need to reuse partially-built delegations as a factory.

codecov · 2024-04-02T01:10:03Z

Codecov Report

Attention: Patch coverage is 71.87500% with 27 lines in your changes are missing coverage. Please review.

Project coverage is 13.67%. Comparing base (a8d1b1d) to head (d33c1f5).

Additional details and impacted files

@@              Coverage Diff              @@
##           v1.0-rc.1      #20      +/-   ##
=============================================
+ Coverage      12.84%   13.67%   +0.82%     
=============================================
  Files             68       68              
  Lines           5620     5711      +91     
  Branches        2703     2724      +21     
=============================================
+ Hits             722      781      +59     
  Misses          4307     4307              
- Partials         591      623      +32

Files	Coverage Δ
src/crypto/signature/envelope.rs	`32.43% <100.00%> (-3.47%)`	⬇️
src/invocation/agent.rs	`36.36% <ø> (ø)`
src/invocation/payload.rs	`32.94% <0.00%> (ø)`
src/delegation/store/memory.rs	`28.33% <0.00%> (-0.98%)`	⬇️
src/delegation/payload.rs	`46.70% <74.07%> (-0.19%)`	⬇️
src/delegation.rs	`84.33% <78.68%> (-15.67%)`	⬇️

... and 5 files with indirect coverage changes

matheus23 · 2024-04-02T10:10:49Z

I attempted to automatically build the varsig header from the signer + codec, but that won't work super well once we have more than the simplest of signature types, so I reverted those changes.

Good call on not doing it in this PR. IMO I still want to find a solution so it's a little less visual noise eventually.

matheus23

I like it.

I see why we need both DelegationRequired and DelegationBuilder (because both are still not signed, and Delegation is then signed and "finalized").

Also here's a soft-proposal (I don't expect this PR needs any changes):

Renames:

Delegation -> SignedDelegation
DelegationRequired -> Delegation
to_builder -> builder (IMO I liked this kind of naming from tokio & iroh)
try_finalize -> try_sign

And then make builder a required call & explicit. And remove all with_ calls from DelegationRequired. So you end up with this:

Delegation {
    subject: Subject::Known(alice),
    issuer: alice,
    audience: bob,
    command: "/".to_string(),

    signer: alice_signer,
    codec: encoding::Preset::DagCbor,
    varsig_header: header::Preset::EdDsa(header::EdDsaHeader {
        codec: encoding::Preset::DagCbor,
    })
}::builder()
.with_via(carol)
.with_policy(vec![])
.with_metadata(BTreeMap::from_iter([("foo".into(), 123.into())]))
.try_sign()

That said, it's kinda unfortunate to use the Delegation name to an unimportant struct like that.

src/crypto/signature/envelope.rs

matheus23 · 2024-04-02T10:18:31Z

src/delegation.rs

+    pub fn subject(&self) -> &Subject<DID> {
+        &self.payload.subject
    }


If we change this, then a petition to keep it consistent with how via(), not_before() and expiration() work.

How's that?

Ah. There are two changes at the same time: From Option -> Subject and from T<&S> to &T<S>. I missed the first.
Can we implement Subject::as_ref(&self) -> Subject<&T> and make this return Subject<&DID>?

matheus23 · 2024-04-02T10:20:59Z

src/delegation.rs

+    pub audience: DID,
+    pub command: String,
+
+    pub codec: PhantomData<C>,


Do we absolutely need the codec PhantomData? Can't rust figure out that it's "inside" the V: varsig::Header<C>? Just double-checking.

Yeah, I'm also not happy about it. I've tried a bunch of things, but I don't think it's possible. Open to ideas!

Yeah. Weird. It's not needed in Delegation::try_sign. I guess the inference in functions is better than in structs. :/

expede · 2024-04-02T15:58:24Z

Delegation -> SignedDelegation

I've tried to avoid similar in the spec, because its not "done" until it's signed — I want to make sure that people don't skip this step, and making it the simple term helps imply that.

Also I've noticed that the ergonomics are different in construction vs consumption: on construction the "main thing" you interact with is unsigned, but on consumption the main thing must be signed.

expede · 2024-04-02T16:09:58Z

to_builder -> builder (IMO I liked this kind of naming from tokio & iroh)

I've seen this pattern as Foo::builder() to get a blank FooBuilder. Will it be confusing, or is it used both ways?

expede · 2024-04-02T16:11:12Z

And remove all with_ calls

Oh, that's what I had originally: I thought you wanted the with_s from previous discussion. Will revert 👍

expede · 2024-04-02T16:12:39Z

try_sign

Hmmm 🤔 try_sign has a different type on DelegationPayload, but I guess the types will guide users here

expede · 2024-04-02T17:04:46Z

Sooo

    pub fn not_before(mut self, not_before: Timestamp) -> DelegationBuilder<DID, V, C> {
        self.not_before = Some(not_before);
        self
    }

Why not just

let d = DelegationRequired {
    subject: Subject::Known(alice),
    issuer: alice,
    audience: bob,
    command: "/".to_string(),

    signer: alice_signer,
    codec: encoding::Preset::DagCbor,
    varsig_header: header::Preset::EdDsa(header::EdDsaHeader {
        codec: encoding::Preset::DagCbor,
    })
}.to_builder();

d.via = Some(carol);
d.policy = vec![];
d.metadata = BTreeMap::from_iter([("foo".into(), 123.into())]);

d.try_finalize()

matheus23 · 2024-04-03T12:29:32Z

I've seen this pattern as Foo::builder() to get a blank FooBuilder. Will it be confusing, or is it used both ways?

It would've kinda made sense together with the rename from DelegationRequired to Delegation. So that I'd be Delegation::builder().
Consider my proposal as a "all or nothing" kinda idea. The individual renames don't make sense without the rest.

And remove all with_ calls

Oh, that's what I had originally: I thought you wanted the with_s from previous discussion. Will revert 👍

Yeah - I changed my mind about that. Another .builder() call in between isn't too bad. (And to be clear - now my stance is I don't want with_ implementations twice).

FWIW, this is how I create delegations in fission-server tests right now (not depending on this PR yet):

let ucan = Delegation::try_sign(
    &issuer,
    varsig_header(),
    delegation::PayloadBuilder::default()
        .issuer(issuer_did)
        .audience(audience_did)
        .subject(Some(resource_did))
        .command("/".to_string())
        .build()?,
)?;

(varsig_header() is an imported function I use for simplicity.)

Builder + required pattern

90f7fd7

expede added 4 commits April 1, 2024 21:44

Specific -> Known

d8bcdf3

Clean up PhantomDatae

866e3c9

Cleanup fixme

cb1f9c8

Add tests

d33c1f5

expede marked this pull request as ready for review April 2, 2024 05:27

matheus23 approved these changes Apr 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Delegation Builder #20

Add Delegation Builder #20

expede commented Apr 2, 2024 •

edited

codecov bot commented Apr 2, 2024 •

edited

matheus23 commented Apr 2, 2024

matheus23 left a comment

matheus23 Apr 2, 2024

expede Apr 2, 2024

matheus23 Apr 2, 2024 •

edited

matheus23 Apr 2, 2024

expede Apr 2, 2024

matheus23 Apr 2, 2024

expede commented Apr 2, 2024

expede commented Apr 2, 2024

expede commented Apr 2, 2024

expede commented Apr 2, 2024

expede commented Apr 2, 2024 •

edited

matheus23 commented Apr 3, 2024 •

edited

Add Delegation Builder #20

Are you sure you want to change the base?

Add Delegation Builder #20

Conversation

expede commented Apr 2, 2024 • edited

codecov bot commented Apr 2, 2024 • edited

Codecov Report

matheus23 commented Apr 2, 2024

matheus23 left a comment

Choose a reason for hiding this comment

matheus23 Apr 2, 2024

Choose a reason for hiding this comment

expede Apr 2, 2024

Choose a reason for hiding this comment

matheus23 Apr 2, 2024 • edited

Choose a reason for hiding this comment

matheus23 Apr 2, 2024

Choose a reason for hiding this comment

expede Apr 2, 2024

Choose a reason for hiding this comment

matheus23 Apr 2, 2024

Choose a reason for hiding this comment

expede commented Apr 2, 2024

expede commented Apr 2, 2024

expede commented Apr 2, 2024

expede commented Apr 2, 2024

expede commented Apr 2, 2024 • edited

matheus23 commented Apr 3, 2024 • edited

expede commented Apr 2, 2024 •

edited

codecov bot commented Apr 2, 2024 •

edited

matheus23 Apr 2, 2024 •

edited

expede commented Apr 2, 2024 •

edited

matheus23 commented Apr 3, 2024 •

edited