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

Rename CannotCidOr, API for creating powerlines & more #18

Merged
merged 10 commits into from
Mar 28, 2024

Conversation

matheus23
Copy link
Member

@matheus23 matheus23 commented Mar 26, 2024

  • Renamed the associated types DelegationStoreError and InvocationStoreError into just Error. They're namespaced by the trait anyways, and is a common pattern in rust
  • Renamed CannotCidOr to DelegationInsertError

Also, previously, given a delegation::Agent, there was no way to create a powerline. The Agent::signer is private, so it can't be used with Delegation::try_sign directly.
I get the fact that store.get_chain shouldn't take a subject: Option<DID>, but I think Agent::delegate should.
I needed that to keep creating powerlines from the fission-server crate.


And more things:

  • Made expiration optional (although we're still discussing)
  • Refactored to use let-else in Envelope::try_from_ipld_envelope
  • Fixed and made use of Envelope::varsig_encode (I needed some API for getting a byte representation of a UCAN, and I think that's probably the best function?)
  • Created missing via and command functions for accessing those Payload fields

@matheus23 matheus23 self-assigned this Mar 26, 2024
Copy link

codecov bot commented Mar 26, 2024

Codecov Report

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

Project coverage is 12.86%. Comparing base (53c49d2) to head (2ed32e3).

❗ Current head 2ed32e3 differs from pull request most recent head 7dec84c. Consider uploading reports for the commit 7dec84c to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##           v1.0-rc.1      #18      +/-   ##
=============================================
+ Coverage      12.77%   12.86%   +0.08%     
=============================================
  Files             68       68              
  Lines           5612     5620       +8     
  Branches        2698     2703       +5     
=============================================
+ Hits             717      723       +6     
- Misses          4302     4309       +7     
+ Partials         593      588       -5     
Files Coverage Δ
src/delegation.rs 100.00% <ø> (ø)
src/delegation/store/memory.rs 29.31% <100.00%> (ø)
src/delegation/store/traits.rs 60.00% <100.00%> (ø)
src/invocation/store/memory.rs 63.63% <100.00%> (ø)
src/invocation/store/traits.rs 100.00% <100.00%> (ø)
src/invocation/agent.rs 36.36% <0.00%> (ø)
src/invocation/payload.rs 32.94% <0.00%> (-0.20%) ⬇️
src/crypto/signature/envelope.rs 35.89% <42.85%> (+1.68%) ⬆️
src/delegation/payload.rs 46.89% <18.18%> (-0.59%) ⬇️

... and 4 files with indirect coverage changes

@expede
Copy link
Member

expede commented Mar 27, 2024

and is a common pattern in rust

Yeah makes sense. I really dislike having to do nested <<Foo as Bar> as Baz>::Error, and giving unique names gets around this. It's probably just worth biting the bullet on the line noise 😆

@expede
Copy link
Member

expede commented Mar 27, 2024

Renamed CannotCidOr to DelegationStoreError

Given that this is really specific to insert, have you considered DelegationInsertError? If we end up needing more error types over time, it would be nice to keep them separate so that we don't have to handle impossible cases downstream.

Base automatically changed from matheus23/v1.0-3 to v1.0-rc.1 March 27, 2024 16:44
@matheus23
Copy link
Member Author

Renamed CannotCidOr to DelegationStoreError

Given that this is really specific to insert, have you considered DelegationInsertError? If we end up needing more error types over time, it would be nice to keep them separate so that we don't have to handle impossible cases downstream.

Yeah you're raising a good point. I'll rename it now.

@matheus23
Copy link
Member Author

I made expiration optional in one of the commits. I know we're still discussing this, so unless that discussion comes to an end, we shouldn't merge this yet.

@matheus23
Copy link
Member Author

Rebased onto the latest v1.0-rc1 state.

@matheus23 matheus23 changed the title Rename CannotCidOr and add API for creating powerlines Rename CannotCidOr, API for creating powerlines & more Mar 27, 2024
Copy link
Member

@expede expede left a comment

Choose a reason for hiding this comment

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

🎉 Looks great!

@expede expede merged commit a00db2d into v1.0-rc.1 Mar 28, 2024
2 of 9 checks passed
@expede expede deleted the matheus23/v1.0-4 branch March 28, 2024 20:49
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.

2 participants