refactor!: module to api and module config #943

TimoGlastra · 2022-07-11T15:57:45Z

Extracts all modules to a separate module class and rename all previous module classes to XXXApi. For now the module is still internal and takes config from the agent config, but it shows how custom module configuration works. This is one step closer to configurable modules.

Basically how you'd use it (if we have full modularization):

const agent = new Agent({
  modules: {
    credentials: new CredentialsModule({
        autoAcceptCredentials: AutoAcceptCredential.always
    })
  }
})

The credentials api can then depend on it like this:

CredentialsApi {
  public constructor(
    credentialsModuleConfig: CredentialsModuleConfig
  ) {
   const autoAcceptCredentials = this.credentialsModuleConfig.autoAcceptCredentials
  }
}

@karimStekelenburg @blu3beri @genaris @2byrds @JamesKEbert Thoughts? If we can agree on the approach for configuration, I can update it for all modules. We can still keep it in init config (so there won't be much breaking changes for now). But from then on it will be trivial to start extracting modules out of core

karimStekelenburg · 2022-07-11T19:05:45Z

I'm a bit confused on what the intent is of this addition. Is my understanding correct that we're currently in a sort of 'hybrid state' where we can register modules both the old and the new way?

Regardless, a showcase/reference implementation is useful indeed. Probably needless to say, but I would prefer to refactor the configuration for all modules before releasing 0.3.0.

TimoGlastra · 2022-07-12T06:48:57Z

Is my understanding correct that we're currently in a sort of 'hybrid state' where we can register modules both the old and the new way?

Well this PR is just to demonstrate how it would look and get some feedback (hence draft PR). If we think this is a good approach I'll update this PR to refactor all modules to the new format with config. However I won't immediately remove the old way, I'd like to do that in a separate PR

berendsliedrecht

Really nice! Just let some comments, mainly on usability.

packages/core/src/agent/BaseAgent.ts

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

berendsliedrecht · 2022-07-12T07:11:56Z

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

    // Api
-    dependencyManager.registerContextScoped(CredentialsModule)
+    dependencyManager.registerContextScoped(CredentialsApi)


I am still not the biggest fan of these register function names. I think that registerContextScoped is a bit ambiguous and does not tell me when to use this method in the context of AFJ.

aliassing it to something like: registerPublicApi or whatever fits better might be worth it to make writing external plugins a lot easier.

I see your point. What I'm not sure about is that we're not sure yet what types of components people want to register. Maybe we can come up with a set of components, but it would need to cover all cases.

registerApi for the api instead of registerContextScoped

registerStatelessService instead of registerSingleton

registerInstance stays as is

We would still need another name for the registerContextScoped if you have something that is not an API but needs to be unique per context. Do you like this direction better?

Yeah this makes it a lot easier to understand. Maybe for the registerContextScoped without an api and needs to be unique, we can use registerStatefulService? I believe that that is what you want to register, right?

I'll raise it during the WG call and gather some more input on this. If we agree we want to change something, I'll raise another PR

berendsliedrecht · 2022-07-12T07:13:05Z

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

+  autoAcceptCredentials: AutoAcceptCredential
+}
+
+export class CredentialsModuleConfig {


This is awesome! Way more clear which properties from the agent config are with that module.

Just more curious, how do we deal with configuration shared between modules, if it ever occurs?

Good question. I think either declare it in both modules and provide it twice, or one module can depend on the configuration of the other module (if they are dependant on each other).

E.g. the outOfBandModule can depend on the autoAcceptConnections from the ConnectionsModule (as you can't use the oob module without the connections module). Does that make sense?

Yeah that makes sense. Is it obvious to the user that the autoAcceptConnections from the ConnectionsModule will be used in the outOfBandModule?

Not sure. We could add it to the documentation of the out of band module. So in the receiveInvitation method documentation we can mention it will use the autoAcceptConnections property from the connections module config to determine whether to auto accept the config.

I've added the config of each module as a public readonly property to the api class, which means it's public api and makes sense it can be used by other modules.

So agent.connections.config is a public property that exposes the config of the connections module

codecov-commenter · 2022-07-13T22:41:17Z

Codecov Report

Merging #943 (af8f301) into 0.3.0-pre (d8ec58b) will increase coverage by 0.14%.
The diff coverage is 82.60%.

@@              Coverage Diff              @@
##           0.3.0-pre     #943      +/-   ##
=============================================
+ Coverage      87.13%   87.28%   +0.14%     
=============================================
  Files            539      562      +23     
  Lines          13086    13272     +186     
  Branches        2244     2142     -102     
=============================================
+ Hits           11403    11584     +181     
- Misses          1587     1684      +97     
+ Partials          96        4      -92

Impacted Files	Coverage Δ
packages/core/src/agent/AgentConfig.ts	`95.00% <ø> (ø)`
...les/credentials/models/CredentialAutoAcceptType.ts	`100.00% <ø> (ø)`
...s/generic-records/services/GenericRecordService.ts	`88.88% <ø> (ø)`
packages/core/src/plugins/Module.ts	`50.00% <0.00%> (-50.00%)`	⬇️
...ages/core/src/storage/migration/UpdateAssistant.ts	`89.41% <33.33%> (-4.26%)`	⬇️
...e/src/modules/question-answer/QuestionAnswerApi.ts	`56.41% <56.41%> (ø)`
packages/core/src/modules/routing/RecipientApi.ts	`63.36% <63.36%> (ø)`
...c/modules/discover-features/DiscoverFeaturesApi.ts	`63.46% <63.46%> (ø)`
packages/core/src/modules/dids/DidsApi.ts	`64.70% <64.70%> (ø)`
...ges/core/src/modules/connections/ConnectionsApi.ts	`66.07% <66.07%> (ø)`
... and 111 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8ec58b...af8f301. Read the comment docs.

Signed-off-by: Timo Glastra <timo@animo.id>

berendsliedrecht

Nice stuff in here! Also a good idea to mark stuff with @deprecated. However, do we have any backwards compatibility with the agent config from before?

TimoGlastra · 2022-07-19T08:37:37Z

Nice stuff in here! Also a good idea to mark stuff with @deprecated. However, do we have any backwards compatibility with the agent config from before?

What do you mean exactly with from before?

berendsliedrecht · 2022-07-19T08:40:25Z

What do you mean exactly with from before?

Well, you added a deprecation notice like so:

  /**
    * @deprecated use indyLedgers from the `LedgerModuleConfig` class
    */
   public get indyLedgers() {
     return this.initConfig.indyLedgers ?? []
   }

This would mean that this is still works and will get the value from the initConfig. Is that correct?

TimoGlastra · 2022-07-19T08:46:59Z

Yes, currently all values still come from the init config. This is mainly because there's no way to configure the internal modules yet. So the main init config is the easiest to make sure it keeps working, but already allowing custom config for custom modules.

I'm still not sure what the best approach is for internal module configuration. My thoughts were to maybe allow to define the module yourself and we'll only register it internally with default config if you don't provide the module.

Without custom config the connections module is registerd internally and auto accept connections will have default value false.

const agent = new Agent({
  modules: {

  }
})

If i register it, it won't be registered internally and we'll use the config as passed by the user.

const agent = new Agent({
  connections: new ConnectionsModule({
     autoAcceptConnections: true
  })
})

But maybe there's other / better appraoches?

…nfiguration

berendsliedrecht · 2022-07-19T08:54:38Z

But maybe there's other / better appraoches?

I quite like this approach and I think that we should do it like that.

One thing that might become slightly annoying here is that shared configuration, I don't think it will happen a lot so it might just be ignored for now.

One quick fix that I have for that is to do it like so:

const agent = new Agent({
  config: {
     autoAcceptConnections: true
  },
  connections: new ConnectionsModule()
})

We can create some cool type that infers the config types from the modules you have added, but this will leave us again with a large object where all the configuration is mashed together...

TBH I prefer your method, but maybe this can give some inspiration for another solution that fixes every problem :)

TimoGlastra · 2022-07-19T09:48:00Z

Merging as only failing test is BBS+ test

Signed-off-by: Timo Glastra <timo@animo.id> BREAKING CHANGE: All module api classes have been renamed from `XXXModule` to `XXXApi`. A module now represents a module plugin, and is separate from the API of a module. If you previously imported e.g. the `CredentialsModule` class, you should now import the `CredentialsApi` class

Signed-off-by: Timo Glastra <timo@animo.id> BREAKING CHANGE: All module api classes have been renamed from `XXXModule` to `XXXApi`. A module now represents a module plugin, and is separate from the API of a module. If you previously imported e.g. the `CredentialsModule` class, you should now import the `CredentialsApi` class Signed-off-by: Ariel Gentile <gentilester@gmail.com>

TimoGlastra requested a review from a team as a code owner July 11, 2022 15:57

TimoGlastra marked this pull request as draft July 11, 2022 15:57

TimoGlastra added the modularization Tasks related to modularization of the framework label Jul 11, 2022

berendsliedrecht reviewed Jul 12, 2022

View reviewed changes

TimoGlastra force-pushed the feat/module-configuration branch 2 times, most recently from 5141660 to 20c35d0 Compare July 13, 2022 22:14

TimoGlastra added the breaking change PR that introduces a breaking change label Jul 14, 2022

TimoGlastra force-pushed the feat/module-configuration branch from 20c35d0 to a9c7716 Compare July 15, 2022 20:15

TimoGlastra changed the title ~~feat(credentials): credential module config~~ @TimoGlastra refactor!: module to api and module config Jul 15, 2022

TimoGlastra changed the title ~~@TimoGlastra refactor!: module to api and module config~~ refactor!: module to api and module config Jul 15, 2022

feat(credentials): credential module config

b0882ba

Signed-off-by: Timo Glastra <timo@animo.id>

TimoGlastra force-pushed the feat/module-configuration branch 2 times, most recently from b6f3256 to 55b94ac Compare July 15, 2022 20:31

TimoGlastra marked this pull request as ready for review July 15, 2022 20:31

refactor!: module to api and module config

b555236

Signed-off-by: Timo Glastra <timo@animo.id>

TimoGlastra force-pushed the feat/module-configuration branch from 55b94ac to b555236 Compare July 15, 2022 20:33

test: fix broken tests

6ee9da5

Signed-off-by: Timo Glastra <timo@animo.id>

berendsliedrecht approved these changes Jul 19, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/0.3.0-pre' into feat/module-co…

af8f301

…nfiguration

TimoGlastra merged commit 0f4cc18 into openwallet-foundation:0.3.0-pre Jul 19, 2022

TimoGlastra deleted the feat/module-configuration branch July 19, 2022 09:49

TimoGlastra restored the feat/module-configuration branch July 20, 2022 19:34

berendsliedrecht mentioned this pull request Jan 18, 2023

fix/transport connection id animo/aries-framework-javascript#24

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: module to api and module config #943

refactor!: module to api and module config #943

TimoGlastra commented Jul 11, 2022 •

edited

Loading

karimStekelenburg commented Jul 11, 2022

TimoGlastra commented Jul 12, 2022 •

edited

Loading

berendsliedrecht left a comment

berendsliedrecht Jul 12, 2022

TimoGlastra Jul 12, 2022

berendsliedrecht Jul 18, 2022

TimoGlastra Jul 18, 2022

berendsliedrecht Jul 12, 2022

TimoGlastra Jul 12, 2022

berendsliedrecht Jul 18, 2022

TimoGlastra Jul 18, 2022

codecov-commenter commented Jul 13, 2022 •

edited

Loading

berendsliedrecht left a comment

TimoGlastra commented Jul 19, 2022

berendsliedrecht commented Jul 19, 2022

TimoGlastra commented Jul 19, 2022

berendsliedrecht commented Jul 19, 2022

TimoGlastra commented Jul 19, 2022

refactor!: module to api and module config #943

refactor!: module to api and module config #943

Conversation

TimoGlastra commented Jul 11, 2022 • edited Loading

karimStekelenburg commented Jul 11, 2022

TimoGlastra commented Jul 12, 2022 • edited Loading

berendsliedrecht 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

codecov-commenter commented Jul 13, 2022 • edited Loading

Codecov Report

berendsliedrecht left a comment

Choose a reason for hiding this comment

TimoGlastra commented Jul 19, 2022

berendsliedrecht commented Jul 19, 2022

TimoGlastra commented Jul 19, 2022

berendsliedrecht commented Jul 19, 2022

TimoGlastra commented Jul 19, 2022

TimoGlastra commented Jul 11, 2022 •

edited

Loading

TimoGlastra commented Jul 12, 2022 •

edited

Loading

codecov-commenter commented Jul 13, 2022 •

edited

Loading