feat!: Discover Features V2 #991

genaris · 2022-08-25T04:39:10Z

Discover Features V1 refactor + Initial API implementation of Discover Features V2 with a FeatureRegistry where arbitrary features can be registered and queried by modules, and also externally (i.e. to let any controller app setting a custom feature that is willing to disclose).

Discover Features module now has a configuration setting to define whether queries should be automatically responded, or leave that decision (and feature set to disclose) to business logic. In either case, events are emitted when receiving queries and disclosures.

Module API is very simple, as we have only two methods whose goal is self-explanatory:

queryFeatures: to query features to a connection
discloseFeatures: to send a disclosure of features to a connection (either proactively or as a response of a query)

Depending on options, queries can use Discover Features V1 or V2 (maybe we could add an auto-selection depending on the complexity of the query) and can be done either asynchronously or synchronously, meaning that the promise will resolve when the response is received (or a timeout). This is not a common pattern in the rest of the framework, but in this case could be useful to quickly ask for features like it happens in RecipientModule to determine mediator pickup strategy.

Feature Registry has been exposed in agent.features key, in order to let the framework controller to add their own custom features (such as Goal Codes). Probably we should add some checks to prevent adding certain feature types from outside of the framework in order to provide consistent features in disclosures (e.g. we should not allow to register a protocol if there is no module that handles it).

Signed-off-by: Ariel Gentile gentilester@gmail.com

codecov-commenter · 2022-08-25T04:57:14Z

Codecov Report

Merging #991 (8102ae0) into 0.3.0-pre (5cdcfa2) will increase coverage by 2.25%.
The diff coverage is 97.05%.

@@              Coverage Diff              @@
##           0.3.0-pre     #991      +/-   ##
=============================================
+ Coverage      85.94%   88.19%   +2.25%     
=============================================
  Files            615      636      +21     
  Lines          14763    15036     +273     
  Branches        2495     2536      +41     
=============================================
+ Hits           12688    13261     +573     
+ Misses          1940     1677     -263     
+ Partials         135       98      -37

Impacted Files	Coverage Δ
...es/discover-features/protocol/v1/messages/index.ts	`100.00% <ø> (ø)`
packages/core/src/plugins/Module.ts	`50.00% <ø> (ø)`
packages/core/src/modules/routing/RecipientApi.ts	`63.23% <16.66%> (-0.32%)`	⬇️
...e/src/agent/models/features/GovernanceFramework.ts	`60.00% <60.00%> (ø)`
packages/core/src/agent/models/features/Feature.ts	`92.00% <92.00%> (ø)`
...c/modules/discover-features/DiscoverFeaturesApi.ts	`96.29% <94.11%> (+32.83%)`	⬆️
packages/core/src/agent/Agent.ts	`97.60% <100.00%> (+0.07%)`	⬆️
packages/core/src/agent/BaseAgent.ts	`95.34% <100.00%> (+0.11%)`	⬆️
packages/core/src/agent/FeatureRegistry.ts	`100.00% <100.00%> (ø)`
...ges/core/src/agent/models/features/FeatureQuery.ts	`100.00% <100.00%> (ø)`
... and 48 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

berendsliedrecht

Nice work @genaris! I had some very minor notes, not really about the specification but more just code cleanup.

I remember from the last WG call that there would be some breaking changes with this module. Will these be introduced in a separate PR?

packages/core/src/modules/discover-features/FeatureRegistry.ts

packages/core/src/modules/basic-messages/BasicMessagesModule.ts

packages/core/src/modules/credentials/CredentialsModule.ts

packages/core/src/modules/discover-features/DiscoverFeaturesModule.ts

packages/core/src/modules/discover-features/FeatureRegistry.ts

packages/core/src/modules/discover-features/protocol/v2/V2DiscoverFeaturesService.ts

packages/core/src/modules/question-answer/QuestionAnswerModule.ts

genaris · 2022-08-25T11:47:28Z

Nice work @genaris! I had some very minor notes, not really about the specification but more just code cleanup.

I remember from the last WG call that there would be some breaking changes with this module. Will these be introduced in a separate PR?

Thanks for the review @blu3beri! Actually this is an attempt to introduce Discover Features V2 before 0.3.0, present some concepts such as the Feature Registry and, as you mention in one of your comments, figure out how to improve module registration API (and module architecture in general) to leverage registration of all features they add to the framework. I would say that it's currently more a discussion than a PR 😄.

During the coding and research over Aries RFCs/DIDComm spec I found these 'features' quite arbitrary and generic, so I'm trying to support the mandatory ones (with some nice typing) while still allowing to define other ones that any custom module may want to add. For the moment I'm registering only protocols but there is room for others such as goal codes, DIDComm version and AIP.

Resulting Discover Features module API is a bit confusing because V1 uses a single query while V2 allows multiple queries. Not simple to improve without introducing breaking changes in the API, but for sure in 0.3.0 will be handled better. Anyway I'll keep looking at it and hearing suggestions 🤔

TimoGlastra

🚀 this is really great @genaris!! Left some notes, but overall looks really good

packages/core/src/agent/__tests__/Agent.test.ts

TimoGlastra · 2022-08-29T16:25:12Z

packages/core/src/modules/basic-messages/BasicMessagesModule.ts

+  private registerFeatures(featureRegistry: FeatureRegistry) {
+    featureRegistry.register(
+      new Protocol({
+        id: 'https://didcomm.org/basicmessage/1.0',
+        roles: [BasicMessageRole.Sender, BasicMessageRole.Receiver],
+      })
+    )
+  }


Nice, I really like this!

The one downside is that you now have to manually register the protocol for discovery. Or can we still leverage the registered message types for feature discover and this allows to add extra metadata to it?

I was thinking about when adding the 'combine' method, so we can call FeatureRegistry from Dispatcher and make this registration "automatically". A problem I see with this approach (which is current AFJ approach of disclosing features) is that this will make the other agent think we fully support a protocol even if we just implement a single message from it (something related to #956). So in this sense I feel it will be more accurate if the module registers and states "I fully support this protocol, with these roles". Of course it can be wrong, but at least it will be a human interpretation error.

That being said, probably it will be better to add this Dispatcher approach for current 0.2.x (will be useful also for Discover Features V1) and move exclusively to a module method in 0.3.x as mentioned in other comment. What do you think?

TimoGlastra · 2022-08-29T16:26:07Z

packages/core/src/modules/connections/ConnectionsModule.ts

@@ -67,6 +69,8 @@ export class ConnectionsModule {
    this.messageSender = messageSender
    this.didResolverService = didResolverService
    this.registerHandlers(dispatcher)
+
+    this.registerFeatures(featureRegistry)


What if we call this from within the register method of the module? That way it is more aligned with modules as we'll have them in 0.3.0. In 0.3.0 I would also like to move handler registration to the module

Absolutely. I think it should be there in order to enforce feature registration when defining a module (and also the handlers, as you mentioned). As I said in a previous comment, I've done some tries by using the register method (had to add featureRegistry as parameter) but didn´t like so much. Maybe we can improve this module API to add separate methods such as:

registerDependencies(dependencyManager)

registerFeatures(featureManager)

registerHandlers(dispatcher)

packages/core/src/modules/discover-features/DiscoverFeaturesModule.ts

TimoGlastra · 2022-08-29T16:32:32Z

packages/core/src/modules/discover-features/FeatureRegistry.ts

+      const index = this.features.findIndex((item) => item.type === feature.type && item.id === feature.id)
+
+      if (index > -1) {
+        this.features[index] = this.features[index].combine(feature)


I like the combine approach 👍 . Maybe we can add an optional override feature later that will not combine but override

packages/core/src/modules/discover-features/FeatureRegistry.ts

genaris · 2022-09-01T18:43:36Z

As discussed in today's WG, it will be better to focus on next 0.3.0 release so I'll move the base branch of this PR to 0.3.0-pre. This will help also to not need to worry about breaking changes that will be not so easy to handle with current API.

Some features that will be added to this module:

Emit events when receiving queries and disclosures, so we won't need to actively wait for them or be online to respond them
Add setting to define whether queries should be automatically responded or leave that decision (and set of features to disclose) to business logic

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

TimoGlastra

This is perfect @genaris! Left some small notes, but overall looks good

TimoGlastra · 2022-09-09T06:15:09Z

packages/core/src/agent/Agent.ts

@@ -156,6 +163,7 @@ export class Agent extends BaseAgent {
    dependencyManager.registerSingleton(TransportService)
    dependencyManager.registerSingleton(Dispatcher)
    dependencyManager.registerSingleton(EnvelopeService)
+    dependencyManager.registerSingleton(FeatureRegistry)


If feature registry is part of core, I think it should not live in the discover features module.

TimoGlastra · 2022-09-09T06:18:46Z

packages/core/src/modules/basic-messages/BasicMessagesModule.ts


+import { Protocol } from '../discover-features'


Same here. I think the protocol / feature registry should be part of core so not all modules depend on the discover features module. This way the discover features module just exposes it as a protocol, but the internal registration is in core. What do you think?

TimoGlastra · 2022-09-09T06:22:37Z

packages/core/src/modules/basic-messages/__tests__/BasicMessagesModule.test.ts

 describe('BasicMessagesModule', () => {
  test('registers dependencies on the dependency manager', () => {
-    new BasicMessagesModule().register(dependencyManager)
+    new BasicMessagesModule().register(featureRegistry, dependencyManager)


Any reason you put featureRegistry first? It introduces a breaking change that may not be necessary?

TimoGlastra · 2022-09-09T06:27:29Z

packages/core/src/modules/discover-features/DiscoverFeaturesApi.ts


    const outbound = createOutboundMessage(connection, queryMessage)
+
+    const replaySubject = new ReplaySubject<Feature[]>(1)
+    if (options.awaitDisclosures) {


Interesting approach! I think I like it 😀

packages/core/src/modules/discover-features/DiscoverFeaturesEvents.ts

TimoGlastra · 2022-09-09T06:32:49Z

packages/core/src/modules/discover-features/DiscoverFeaturesApi.ts

  private eventEmitter: EventEmitter
  private stop$: Subject<boolean>
  private agentContext: AgentContext
+  private serviceMap: ServiceMap<DFSs>



In other modules I always expose the config as read only property on the API. This was you can access the config later on (and we may add a way to update certain properties at runtime)

TimoGlastra · 2022-09-09T06:34:12Z

packages/core/src/modules/discover-features/DiscoverFeaturesModuleConfig.ts

+    this.options = options ?? {}
+  }
+
+  public get autoAcceptDiscoverFeatureQueries() {


Might be good to add an inheritDoc here (as used in other modules) so you get documentation when getting a property from the config

TimoGlastra · 2022-09-09T06:35:10Z

packages/core/src/modules/discover-features/DiscoverFeaturesModuleOptions.ts

+/**
+ * Get the supported protocol versions based on the provided discover features services.
+ */
+export type ProtocolVersionType<DFSs extends DiscoverFeaturesService[]> = DFSs[number]['version']


This file should be Called DiscoverFeatureApiOptions I think?

TimoGlastra · 2022-09-09T06:40:37Z

packages/core/src/modules/discover-features/DiscoverFeaturesApi.ts

+   * @param options feature queries to perform, protocol version and optional comment string (only used
+   * in V1 protocol). If awaitDisclosures is set, perform the query synchronously with awaitDisclosuresTimeoutMs timeout.
+   */
+  public async queryFeatures(options: QueryFeaturesOptions<DFSs>) {


Maybe too much overhead/complex but do you think we can add return type definition that takes into account the answer to awaitDosclosures?

That way you don't have an optional return type, which can improve DX

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

genaris · 2022-09-09T15:25:53Z

This is perfect @genaris! Left some small notes, but overall looks good

Thanks @TimoGlastra for your re-review 😄 . I think I've addressed all your suggestions (very appropriate BTW). There are of course some things to improve but I think it's ready to merge.

TimoGlastra · 2022-09-09T17:27:54Z

Can you write a short message describing the breaking changes of this PR? Then I can include those in the commit for the change log

genaris · 2022-09-09T18:37:54Z

Can you write a short message describing the breaking changes of this PR? Then I can include those in the commit for the change log

OK I think that finally it's only about Discover Features V1 module usage so I would say:

queryFeatures method parameters have been unified to a single QueryFeaturesOptions object that requires specification of Discover Features protocol to be used.
isProtocolSupported has been replaced by the more general synchronous mode of queryFeatures, which works when awaitDisclosures in options is set. Instead of returning a boolean, it returns an object with matching features
Custom modules implementing protocols must register them in Feature Registry in order to let them be discovered by other agents (this can be done in module register(dependencyManager, featureRegistry) method)

* feat: expose featureRegistry & add some e2e tests * feat: add synchronous feature query Signed-off-by: Ariel Gentile <gentilester@gmail.com> BREAKING CHANGE: - `queryFeatures` method parameters have been unified to a single `QueryFeaturesOptions` object that requires specification of Discover Features protocol to be used. - `isProtocolSupported` has been replaced by the more general synchronous mode of `queryFeatures`, which works when `awaitDisclosures` in options is set. Instead of returning a boolean, it returns an object with matching features - Custom modules implementing protocols must register them in Feature Registry in order to let them be discovered by other agents (this can be done in module `register(dependencyManager, featureRegistry)` method) Signed-off-by: Ariel Gentile <gentilester@gmail.com>

berendsliedrecht reviewed Aug 25, 2022

View reviewed changes

genaris linked an issue Aug 25, 2022 that may be closed by this pull request

Add support for RFC 0557: Discover Features v2 #495

Closed

TimoGlastra reviewed Aug 29, 2022

View reviewed changes

genaris changed the base branch from main to 0.3.0-pre September 2, 2022 11:44

genaris force-pushed the feat/discover-features-v2 branch from 9f7d14e to 7337b00 Compare September 2, 2022 12:30

feat: discover features v2 preliminary implementation

8af513c

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

genaris force-pushed the feat/discover-features-v2 branch from 7337b00 to 8af513c Compare September 2, 2022 15:14

genaris added 4 commits September 2, 2022 13:12

fix: module tests, lint issues

6b8f25e

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

feat: services as subclasses, add events / config

284d62b

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

feat: expose featureRegistry & add some e2e tests

dc24461

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

docs: some API updates

53a73c7

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

genaris marked this pull request as ready for review September 8, 2022 02:50

genaris requested a review from a team as a code owner September 8, 2022 02:50

genaris added 2 commits September 8, 2022 19:19

feat: add synchronous feature query

54d6cf1

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

test: service and e2e tests for v1 and v2

46f1393

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

genaris requested review from TimoGlastra and berendsliedrecht September 9, 2022 01:09

TimoGlastra approved these changes Sep 9, 2022

View reviewed changes

genaris added 7 commits September 9, 2022 09:39

refactor: move FeatureRegistry to agent + module register() args order

95304c6

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

feat: add messages to discover features events

96c6397

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

refactor: rename module options

0c00b24

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

refactor: query return type

c923d52

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

fix: query return type

9d03b4c

Signed-off-by: Ariel Gentile <gentilester@gmail.com>

Merge branch '0.3.0-pre' into feat/discover-features-v2

91e4c05

Merge branch '0.3.0-pre' into feat/discover-features-v2

8102ae0

genaris changed the title ~~feat: Discover Features V2~~ feat!: Discover Features V2 Sep 9, 2022

TimoGlastra merged commit 273e353 into openwallet-foundation:0.3.0-pre Sep 9, 2022

genaris linked an issue Sep 10, 2022 that may be closed by this pull request

Pickup protocol returned in discover features protocol, but only supported on the client #956

Closed

genaris deleted the feat/discover-features-v2 branch October 13, 2022 20:17

berendsliedrecht mentioned this pull request Jan 18, 2023

fix/transport connection id animo/credo-ts#24

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Discover Features V2 #991

feat!: Discover Features V2 #991

genaris commented Aug 25, 2022 •

edited

Loading

codecov-commenter commented Aug 25, 2022 •

edited

Loading

berendsliedrecht left a comment

genaris commented Aug 25, 2022

TimoGlastra left a comment

TimoGlastra Aug 29, 2022

genaris Aug 31, 2022

TimoGlastra Aug 29, 2022

genaris Aug 31, 2022

TimoGlastra Aug 29, 2022

genaris commented Sep 1, 2022

TimoGlastra left a comment

TimoGlastra Sep 9, 2022

TimoGlastra Sep 9, 2022

TimoGlastra Sep 9, 2022

TimoGlastra Sep 9, 2022

genaris Sep 9, 2022

TimoGlastra Sep 9, 2022

TimoGlastra Sep 9, 2022

TimoGlastra Sep 9, 2022

TimoGlastra Sep 9, 2022

genaris commented Sep 9, 2022

TimoGlastra commented Sep 9, 2022

genaris commented Sep 9, 2022

feat!: Discover Features V2 #991

feat!: Discover Features V2 #991

Conversation

genaris commented Aug 25, 2022 • edited Loading

codecov-commenter commented Aug 25, 2022 • edited Loading

Codecov Report

berendsliedrecht left a comment

Choose a reason for hiding this comment

genaris commented Aug 25, 2022

TimoGlastra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

genaris commented Sep 1, 2022

TimoGlastra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

genaris commented Sep 9, 2022

TimoGlastra commented Sep 9, 2022

genaris commented Sep 9, 2022

genaris commented Aug 25, 2022 •

edited

Loading

codecov-commenter commented Aug 25, 2022 •

edited

Loading