decentralized-identity / veramo

A JavaScript Framework for Verifiable Data
https://veramo.io
Apache License 2.0
431 stars 131 forks source link

feat(coordinate-mediation): upgrade from 2.0 to 3.0 #1282

Closed radleylewis closed 10 months ago

radleylewis commented 11 months ago

closes #1255

What is being changed

This PR updates Veramo's Coordinate Mediation from the v2 specification to the recently released v3 Coordinate Mediation specification. A summary of the changes introduced is as follows:

Coordinate Mediation:

Mediate Grant or Deny control mechanism:

mediate cli command

Track Granted and Denied Mediation Requests

Recipient Update

Testing & Documentation

Quality

Check all that apply:

Details

If applicable, add screen captures, error messages or stack traces to help explain your problem.

codecov-commenter commented 11 months ago

Codecov Report

Attention: 102 lines in your changes are missing coverage. Please review.

Comparison is base (17a2991) 84.97% compared to head (a4d8e61) 85.89%. Report is 1 commits behind head on next.

Files Patch % Lines
...otocols/coordinate-mediation-v3-message-handler.ts 91.16% 51 Missing :warning:
...kages/kv-store/src/store-adapters/typeorm/index.ts 12.90% 27 Missing :warning:
...ackages/mediation-manager/src/mediation-manager.ts 88.81% 18 Missing :warning:
packages/kv-store/src/key-value-store.ts 57.14% 3 Missing :warning:
packages/data-store/src/migrations/index.ts 80.00% 2 Missing :warning:
packages/data-store/src/index.ts 90.90% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## next #1282 +/- ## ========================================== + Coverage 84.97% 85.89% +0.92% ========================================== Files 167 170 +3 Lines 18105 18939 +834 Branches 2040 2137 +97 ========================================== + Hits 15384 16268 +884 + Misses 2721 2671 -50 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

pauldesmondparker commented 10 months ago

@mirceanis Just a reminder that this is here :v:

mirceanis commented 10 months ago

@mirceanis Just a reminder that this is here ✌️

I saw it, but I'm traveling this week so I'll be able to review only next week. Thank you for the effort and the patience.

radleylewis commented 10 months ago

closes #1255

What is being changed

This PR updates Veramo's Coordinate Mediation from the v2 specification to the recently released v3 Coordinate Mediation specification. A summary of the changes introduced is as follows:

Coordinate Mediation:

  • update Mediate Request (and its response types of Mediate Grant/Mediate Deny)
  • adds Recipient Update and Recipient Update Response
  • adds Recipient Query and Recipient

Mediate Grant or Deny control mechanism:

  • adds a Mediate Grant or Mediate Deny control mechanism
  • mechanism is currently hardcoded, but TODO marked to allow injection of this as default or a user-defined custom handler
  • adds a MediatePolicy data table
  • adds a global rule to allow-all or deny-all by default
  • adds core-types for Mediate Policy
  • adds associated dataStore methods
  • adds associated declarations

mediate cli command

  • adds the mediate cli command
  • adds: list, remove, allow-from and deny-from sub-commands

Track Granted and Denied Mediation Requests

  • adds a Mediation table to the dataStore to track GRANTED/DENIED status
  • adds associated dataStore methods
  • adds associated type declarations

Recipient Update

  • adds a recipient_dids table to record recipient dids
  • adds associated dataStore methods
  • adds associated type declarations

Testing & Documentation

  • builds upon existing tests
  • introduces additional tests for new functionality in coordinate mediation messages

Quality

Check all that apply:

  • [x] I want these changes to be integrated
  • [x] I successfully ran pnpm i, pnpm build, pnpm test, pnpm test:browser locally.
  • [x] I allow my PR to be updated by the reviewers (to speed up the review process).
  • [x] I added unit tests.
  • [x] I added integration tests.
  • [ ] I did not add automated tests because _____, and I am aware that a PR without tests will likely get rejected.

Details

If applicable, add screen captures, error messages or stack traces to help explain your problem.

To make it easier to track the scope of the PR I have summarised the main changes introduced by this PR below:

radleylewis commented 10 months ago

It looks much better now with an independent plugin for managing mediation metadata. Kudos for the various workarounds you've found! It's been a delight reviewing this :)

This PR can probably be merged as is. But if you still have time for some improvements, I have a few points:

  • revert the did-comm plugin constructor changes
  • define the IMediationManager and collateral types in the new package instead of the @veramo/core-types
  • maybe simplify the mediation manager plugin config by using a single kv-store?

Thanks for the thorough review, it is greatly appreciated! I have pushed some updates in line with your comments. Specifically:

Thanks again and we look forward to your comments.