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

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

Conversation

radleylewis
Copy link
Contributor

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:

  • I want these changes to be integrated
  • I successfully ran pnpm i, pnpm build, pnpm test, pnpm test:browser locally.
  • I allow my PR to be updated by the reviewers (to speed up the review process).
  • I added unit tests.
  • 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.

@codecov-commenter
Copy link

codecov-commenter commented Oct 24, 2023

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 ⚠️
...kages/kv-store/src/store-adapters/typeorm/index.ts 12.90% 27 Missing ⚠️
...ackages/mediation-manager/src/mediation-manager.ts 88.81% 18 Missing ⚠️
packages/kv-store/src/key-value-store.ts 57.14% 3 Missing ⚠️
packages/data-store/src/migrations/index.ts 80.00% 2 Missing ⚠️
packages/data-store/src/index.ts 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@nickreynolds nickreynolds left a comment

Choose a reason for hiding this comment

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

overall this seems like a great update. I think @mirceanis has some thoughts on the changes to the data store, so I'll hold off on approval until hearing from him

packages/message-handler/src/message.ts Outdated Show resolved Hide resolved
@radleylewis radleylewis changed the title feat: coordinate mediation changes feat(coordinate-mediation): upgrade from 2.0 to 3.0 Oct 25, 2023
Copy link
Member

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

Thanks for this awesome contribution, it's really great to see such a high quality PR as a first contribution!

There are a couple of things to be addressed before we can merge this:

  • There's a lot of coupling between the new protocol and the data-store API + implementations which will definitely cause lots of friction down the road. It's best to separate the config storage into its own plugin.
  • The v2 functionality is being replaced altogether. Anyone using or depending on v2 will be stuck on an old version of veramo if we merge it as is.

packages/cli/src/createCommand.ts Show resolved Hide resolved
packages/data-store-json/src/data-store-json.ts Outdated Show resolved Hide resolved
packages/did-comm/src/didcomm.ts Outdated Show resolved Hide resolved
packages/did-comm/src/didcomm.ts Outdated Show resolved Hide resolved
radleylewis and others added 14 commits November 10, 2023 10:11
update coordinate mediation from v2 to v3
 - add recipient update
 - add recipient update response
 - add recipient query
 - add recipient
add grant or deny control mechanism
 - add default logic
 - add TODO for injection of grant/deny mechanism logic
add datastore for:
 - mediation
 - recipient did
 - mediation policy
add core-types:
 - IDataStore
 - IMediation
 - IMediationPolicy
 - IRecipientDid
modify validation to use type guards
remove return_route on MEDIATE_REQUEST
add tests for added functionality
 - modify mediate test suite
 - add recipient update test suite
 - add recipient query test suite
documentation:
 - add documentation on introduced methods
cli:
 - add mediate allow-from command
 - add mediate deny-from command
 - add mediate list
 - add mediate remove
Co-authored-by: Paul Parker <114397409+pauldesmondparker@users.noreply.github.com>
Co-authored-by: Paul Parker <114397409+pauldesmondparker@users.noreply.github.com>
@radleylewis radleylewis force-pushed the feat/didcomm_v3_coordinate_mediation_update branch from adf9c8d to 70fcde3 Compare November 10, 2023 04:53
@pauldesmondparker
Copy link

@mirceanis Just a reminder that this is here ✌️

@mirceanis
Copy link
Member

@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
Copy link
Contributor Author

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:

  • I want these changes to be integrated
  • I successfully ran pnpm i, pnpm build, pnpm test, pnpm test:browser locally.
  • I allow my PR to be updated by the reviewers (to speed up the review process).
  • I added unit tests.
  • 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:

  • retain coordinate-mediation v2 (protocol)
  • add coordinate-mediation v3 (protocol)
  • mediation manager (plugin):
    • add mechanism to choose between all deny OR all accept mediation strategies
    • add cli and api support for adding specific mediation accept or deny decisions in advance of mediation requests
    • use kv-store as storage adapter for mediation plugin

Copy link
Member

@mirceanis mirceanis left a comment

Choose a reason for hiding this comment

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

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?

package.json Outdated Show resolved Hide resolved
packages/mediation-manager/src/mediation-manager.ts Outdated Show resolved Hide resolved
packages/cli/src/mediate.ts Outdated Show resolved Hide resolved
packages/mediation-manager/README.md Show resolved Hide resolved
packages/mediation-manager/LICENSE Outdated Show resolved Hide resolved
@radleylewis
Copy link
Contributor Author

radleylewis commented Nov 22, 2023

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:

  • On the first point around revert the did-comm plugin constructor changes my thinking here is that a object allowing for key parameters is more adaptable to future changes than ordered parameter arguments. We'll revert if you don't find our reasoning compelling.
  • The mediation-manager types have been shifted from the core-types to the mediation manager plugin directory.
  • The kv-store is covered in the PR comment responses. Just to point out that in some cases the keys may be the same, resulting in clashes, and so would need to be prefixed with group identifiers which does have some trade offs.

Thanks again and we look forward to your comments.

@mirceanis mirceanis merged commit 462735d into decentralized-identity:next Dec 3, 2023
9 checks passed
@radleylewis radleylewis deleted the feat/didcomm_v3_coordinate_mediation_update branch December 7, 2023 05:58
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.

Add Support for DIDCommv3 Coordinate Mediation
5 participants