Skip to content
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

Update capabilites in line with Ucan 0.9.0/0.10.0 #105

Merged
merged 1 commit into from
Jun 27, 2023

Conversation

jsantell
Copy link
Contributor

@jsantell jsantell commented Jun 5, 2023

Initial work on updating to support latest capabilities in Ucan spec 0.9.0/0.10.0, with a goal of moving closer to the final serializable shape.

  • Represents capabilities as map-of-maps rather than array of tuples.
  • Renames 'att' to 'cap' (Ucan spec 0.10.0).
  • Addresses/fixes Introduce nb field for capabilities #22 (or what it turned into in 0.10.0)

Looking for feedback on:

  • Leaves caveat parsing up to consumer -- we can filter out capabilities that must be invalid (e.g. empty caveats []) in reduce_capabilities(), or we can introduce a semantical layer (as part of CapabilitySemantics<S, A, C>), but unclear what that looks like and how it would work.
  • Unclear what kind of ordering, if any, should be enforced (Canonicalization of valid-ish tokens  #100)
  • Renames 'Capability' to 'CapabilityView' -- other renaming within semantics that could use some improvements

@codecov-commenter
Copy link

Codecov Report

Merging #105 (b4a9ebc) into main (3d30ec8) will increase coverage by 1.31%.
The diff coverage is 64.06%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #105      +/-   ##
==========================================
+ Coverage   57.10%   58.42%   +1.31%     
==========================================
  Files          24       23       -1     
  Lines         802      849      +47     
  Branches      196      207      +11     
==========================================
+ Hits          458      496      +38     
+ Misses        206      204       -2     
- Partials      138      149      +11     
Impacted Files Coverage Δ
ucan/src/chain.rs 62.61% <50.00%> (+0.71%) ⬆️
ucan/src/ucan.rs 67.36% <50.00%> (-1.45%) ⬇️
ucan/src/capability/semantics.rs 44.79% <62.96%> (+9.14%) ⬆️
ucan/src/capability/data.rs 66.10% <66.10%> (ø)
ucan/src/builder.rs 78.00% <66.66%> (-1.00%) ⬇️
ucan/src/ipld/ucan.rs 71.11% <100.00%> (ø)

... and 1 file with indirect coverage changes

Copy link
Member

@cdata cdata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a promising first pass. Here is my high-level feedback:

  • We need to consider the updates to resources in UCAN that have accumulated as of v0.10.0
  • It would be nice to see some additional test coverage to ensure that caveats work as intended
  • It may not be feasible, but it would be nice to try to winnow down the number of representations of a capability as much as possible (with just one representation being the North Star)

ucan/src/capability/data.rs Show resolved Hide resolved
ucan/src/capability/data.rs Outdated Show resolved Hide resolved
ucan/src/capability/semantics.rs Show resolved Hide resolved
@cdata
Copy link
Member

cdata commented Jun 6, 2023

Oh, should also call out that as of this change we will likely be removing support for UCAN IPLD (to be re-introduced at some future date as the spec is updated to be coherent with v0.10.0+).

@jsantell jsantell force-pushed the capabilities branch 4 times, most recently from 6fb3927 to f214fbf Compare June 12, 2023 15:25
@jsantell
Copy link
Contributor Author

Now with working caveats, testing the cases in the spec.

  • There are a few number of Capability representations, but unavoidable IMO. Names could be improved (e.g. CapabilityView).
    • Capabilities representing the "serialized" map-of-map nature of the changes in UCAN 0.9/0.10.
    • Capability are deconstructed, "deserialized" individual capabilities generated from Capabilities, reduced to a single resource/ability/caveat per. This form is in-line with the current implementation, and the easiest way (IMO) of working with capabilities.
    • CapabilityView, a Capability imbued with semantics, used in proof reduction and validation.
  • Renames With=>Resource, Action=>Ability, Resource=>ResourceURI -- not happy with ResourceURI naming. This may also change when swapping out my/as with ucan, etc. Open to ideas for naming in src/capability/ as well e.g. data.rs.
  • I wanted to add non-semantical capabilities when writing tests, introducing (the poorly named) UcanBuilder::claiming_capability_from_data and UcanBuilder::claiming_capabilities_from_data functions. As semantics are only used in proof chain reduction, claim_capability/claim_capabilities could ingest the data forms, and we introduce a new method to build via semantics/views. Additionally, other tests could use these "raw capabilities", as only proof chain parsing needs to imbue semantics as well.

@jsantell jsantell requested a review from cdata June 12, 2023 16:02
ucan/src/builder.rs Outdated Show resolved Hide resolved
ucan/src/tests/attenuation.rs Show resolved Hide resolved
Copy link
Member

@cdata cdata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome. Only one meaningful change suggested.

ucan/src/builder.rs Outdated Show resolved Hide resolved
ucan/src/capability/data.rs Outdated Show resolved Hide resolved
…wg#22.

* Represents capabilities as map-of-maps rather than array of tuples.
* Validates caveats in proof chain
* Renames 'att' to 'cap' (Ucan spec 0.10.0).
* Renames various capability semantics structs with spec names (With=>Resource,
  Can/Action=>Ability, Resource=>ResourceURI)
* Renames 'Capability' to 'CapabilityView'.
@jsantell
Copy link
Contributor Author

Updated:

  • UcanBuilder::claiming_capability now generic over C: Into<Capability> and UcanBuilder::claiming_capabilities for the &[C] variant.
  • Removed the now extraneous clone() and into() for previous claiming_capability implementations
  • Linked spec where appropriate in doc comments

Copy link
Member

@cdata cdata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this!

@cdata cdata merged commit 0bdf98f into ucan-wg:main Jun 27, 2023
5 checks passed
@jsantell jsantell deleted the capabilities branch June 27, 2023 23:16
@github-actions github-actions bot mentioned this pull request Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce nb field for capabilities
3 participants