refactor!: add agent context #881

TimoGlastra · 2022-06-19T10:02:28Z

This is a big pull request, but doesn't change any functionality of the agent. All services, repositories etc.. now use agent context so we can start using them as stateless services for multitenancy (as described here: https://hackmd.io/vGLVlxLvQR6jsEEjzNcL8g#Agent-Context).

This doesn't change the public API of the framework, but because of the large number of changes and also the implications for custom modules I've marked this as a breaking change.

Will add more detailed migration docs later on, but generally you have to update your modules to now inject the agent context object and pass that around to services. See the DummyModule for an example: https://github.com/TimoGlastra/aries-framework-javascript/blob/fb26a4dfe656b38da984ed46faf5b5a5f3cda1df/samples/extension-module/dummy/DummyModule.ts

codecov-commenter · 2022-06-19T10:23:16Z

Codecov Report

Merging #881 (2be8776) into main (3c84265) will increase coverage by 0.13%.
The diff coverage is 87.74%.

@@            Coverage Diff             @@
##             main     #881      +/-   ##
==========================================
+ Coverage   87.65%   87.78%   +0.13%     
==========================================
  Files         470      473       +3     
  Lines       11172    11251      +79     
  Branches     1868     1905      +37     
==========================================
+ Hits         9793     9877      +84     
+ Misses       1313     1309       -4     
+ Partials       66       65       -1

Impacted Files	Coverage Δ
packages/core/src/agent/AgentConfig.ts	`94.64% <ø> (-0.28%)`	⬇️
packages/core/src/constants.ts	`100.00% <ø> (ø)`
...les/credentials/formats/CredentialFormatService.ts	`100.00% <ø> (ø)`
...cation/handlers/V1RevocationNotificationHandler.ts	`71.42% <ø> (ø)`
...cation/handlers/V2RevocationNotificationHandler.ts	`71.42% <ø> (ø)`
...ges/core/src/modules/credentials/services/index.ts	`100.00% <ø> (ø)`
...ore/src/modules/dids/methods/web/WebDidResolver.ts	`60.00% <ø> (ø)`
...c/modules/routing/handlers/MediationDenyHandler.ts	`62.50% <0.00%> (+6.94%)`	⬆️
...ckages/core/src/transport/HttpOutboundTransport.ts	`9.75% <0.00%> (-1.88%)`	⬇️
packages/core/src/transport/WsOutboundTransport.ts	`9.23% <0.00%> (-2.37%)`	⬇️
... and 95 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 3c84265...2be8776. Read the comment docs.

berendsliedrecht

I have reviewed the first 36 files and will continue somewhere tomorrow (it is quite a bit).

But I do have a comment and that is about the function signatures. I believe we can make every function signature way more consistent by having a common pattern.

Seeing that most functions now take an AgentContext it would nice to have something like this:

const afjFunc = (agentContext: AgentContext, parameters: {...}, options?: {...}) => { ... }


// or 
const afjFunc = (parameters: {...}, options?: {...}) => { ... }

With this we will always have named parameters and have a nice division between the parameters of a function and the configuration/options we supply to it (like shouldDeleteCredential or timeOut). I am interested to hear what you think about this structure and whether it is something we want to implement in AFJ. I think it has value, but it does require a big breaking change.

I am not too sure if we want to also add this to the public api, might be worth discussing.

packages/core/src/agent/MessageSender.ts

berendsliedrecht · 2022-06-19T12:03:56Z

packages/core/src/agent/Dispatcher.ts

    this.messageSender = messageSender
    this.eventEmitter = eventEmitter
-    this.logger = agentConfig.logger
+    this.logger = logger


I am a bit confused by this. Maybe I am missing something about the context, but could we not get the logger from agentContext.logger?

Checking a bit through this PR it seems a bit "messy" on how we deal with the logger instance. We get it via the injection but it is also in the agentConfig. I am not too knowledgeable on dependency injection with tsyringe, but is this really the best way to deal with the logger instance?

Also, do we want each agent to have their instance of a logger or do we want to use a RootContainer (if I understand your design correctly) logger instance?

Yeah that's something I also wasn't sure about, so good that you're calling it out.

It's in the agent config because we configure it in the agent init config. However my thought was that the logger would be shared between the different tenants. I'm not sure if it would work well if we have a lot of logger instances, and there's ways to different context of the different agents in logs.

As mentioned in the design document we should probably look at a way to separate agent config (global, used by all tenants) and tenant config (specific to that tenant).

What would be your preferred approach here?

Yeah I think a global configuration, maybe a Provider/manager agent aswell, and tenant specific configuration would be a good solution.

Quickly looking through the AgentConfig I do not see a lot that should be globally defined. That being said, I still think it has value for future configuration.

berendsliedrecht · 2022-06-19T12:05:21Z

packages/core/src/index.ts

@@ -1,6 +1,7 @@
 // reflect-metadata used for class-transformer + class-validator
 import 'reflect-metadata'

+export { AgentContext } from './agent/AgentContext'


Why do we want to expose the AgentContext publicly?

If we want to inject it (in extension modules for example) we need it. So it's not necessary for the public api for users, but is needed if you want to write extension modules.

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

berendsliedrecht · 2022-06-19T12:09:46Z

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

@@ -164,7 +166,7 @@ export class DidExchangeProtocol {
      )
    }

-    const didDocument = await this.extractDidDocument(message)
+    const didDocument = await this.extractDidDocument(messageContext.agentContext, message)


Why exactly is the agentContext inside the messageContext? Just curious.

Because each message can have their own context. Handlers, services and repositories are now all stateless. The agent context gives the context to process the message. So if we receive a message we'll find the correct tenant, assign the agent context for that tenant to the message context and then process the message.

We could make it a separate parameter, but it kinda made sense to me to tie the agent context to a specific message context. Would you prefer to make it a separate parameter?

Its a difficult one. I think for consistency a seperate agentContext might be nicer, but I see your point of containing the agentContext inside the messageContext. I am okay with keeping it like this.

packages/core/src/modules/connections/handlers/DidExchangeResponseHandler.ts

berendsliedrecht · 2022-06-19T12:15:20Z

packages/core/src/modules/connections/services/ConnectionService.ts

  }

-  public async findSingleByQuery(query: { did: string; theirDid: string }) {
-    return this.connectionRepository.findSingleByQuery(query)
+  public async findByDids(agentContext: AgentContext, query: { ourDid: string; theirDid: string }) {


Is there a reason why we call it ourDid? I believe ACA-Py uses my_did? I have no preference, more just curious.

Not sure, @jakubkoci added this

packages/core/src/modules/connections/services/ConnectionService.ts

TimoGlastra · 2022-06-19T12:56:57Z

Seeing that most functions now take an AgentContext it would nice to have something like this:

const afjFunc = (agentContext: AgentContext, parameters: {...}, options?: {...}) => { ... }


// or 
const afjFunc = (parameters: {...}, options?: {...}) => { ... }

Not 100% sure what you mean by this. This is a pattern already used by some methods, do you mean we should enforce it for all methods? How would we do something like that?

I'm also not sure it will work for all methods. It can be difficult sometimes to differentiate between what parameters and what options are (in the case of the shouldDeleteCredential it's clear, but I think there's a lot of methods where it's not that clear. I'm also not sure if it will improve the API for all methods. E.g. this looks cleaner to me:

agent.credentials.offerCredential({
  connectionId: 'the-connection-id',
  autoAcceptCredential: AutoAcceptCredential.Always,
  credentialFormats: {
  /* formats */
  }
})

than this:

agent.credentials.offerCredential({
  connectionId: 'the-connection-id',
  credentialFormats: {
  /* formats */
  }
}, {
  autoAcceptCredential: AutoAcceptCredential.Always
})

Anyway it seems this would be better addressed to me in a separate issue, except if this involves moving the AgentContext around. I made it the first parameter to each method now for consistency, would you change that?

berendsliedrecht · 2022-06-19T14:24:08Z

Anyway it seems this would be better addressed to me in a separate issue, except if this involves moving the AgentContext around. I made it the first parameter to each method now for consistency, would you change that?

No the agentContext as first param is a good choice. My point is to get more consistency within the framework. TBH, I do think that the second method with the object for autoAccept is better. And 100% it will be difficult, but I think that finding a clear division between parameters and config/options might cleanup the functions aswell.

berendsliedrecht

Reviewed this time until file: 91. Will continue later today :)

packages/core/src/modules/ledger/IndyPool.ts

packages/core/src/storage/migration/updates/0.1-0.2/mediation.ts

berendsliedrecht

Great work @TimoGlastra! I do think that we should not mark this as refactor as there are many many changes to functions and the way we use them.

I left a couple of notes but nothing blocking.

We should also merge it probably after the 0.2.0 release.

packages/core/tests/mocks/MockWallet.ts

Signed-off-by: Timo Glastra <[email protected]>

TimoGlastra · 2022-07-03T11:15:01Z

Superseded by #920 (which is based on 0.3.0-pre instead of main)

TimoGlastra added the multitenancy Tasks related to multi-tenancy label Jun 19, 2022

TimoGlastra requested a review from a team as a code owner June 19, 2022 10:02

berendsliedrecht reviewed Jun 19, 2022

View reviewed changes

TimoGlastra mentioned this pull request Jun 21, 2022

feat!: add agent context provider #898

Closed

berendsliedrecht reviewed Jun 22, 2022

View reviewed changes

packages/core/src/modules/ledger/IndyPool.ts Show resolved Hide resolved

berendsliedrecht reviewed Jun 24, 2022

View reviewed changes

packages/core/src/storage/migration/updates/0.1-0.2/mediation.ts Show resolved Hide resolved

berendsliedrecht approved these changes Jun 24, 2022

View reviewed changes

packages/core/tests/mocks/MockWallet.ts Show resolved Hide resolved

TimoGlastra mentioned this pull request Jun 25, 2022

feat: initial plugin api #907

Merged

TimoGlastra force-pushed the refactor/add-agent-context branch 4 times, most recently from 2be8776 to fb26a4d Compare June 30, 2022 13:22

TimoGlastra added the breaking change PR that introduces a breaking change label Jun 30, 2022

refactor!: add agent context

afca7dc

Signed-off-by: Timo Glastra <[email protected]>

TimoGlastra force-pushed the refactor/add-agent-context branch from fb26a4d to afca7dc Compare July 3, 2022 09:51

This was referenced Jul 3, 2022

feat: add base agent class #919

Closed

refactor!: add agent context #920

Merged

TimoGlastra closed this Jul 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: add agent context #881

refactor!: add agent context #881

TimoGlastra commented Jun 19, 2022 •

edited

Loading

codecov-commenter commented Jun 19, 2022 •

edited

Loading

berendsliedrecht left a comment

berendsliedrecht Jun 19, 2022

TimoGlastra Jun 19, 2022

berendsliedrecht Jun 19, 2022

berendsliedrecht Jun 19, 2022

TimoGlastra Jun 19, 2022

berendsliedrecht Jun 19, 2022

TimoGlastra Jun 19, 2022

berendsliedrecht Jun 19, 2022

berendsliedrecht Jun 19, 2022

TimoGlastra Jun 19, 2022

TimoGlastra commented Jun 19, 2022 •

edited

Loading

berendsliedrecht commented Jun 19, 2022

berendsliedrecht left a comment

berendsliedrecht left a comment

TimoGlastra commented Jul 3, 2022

refactor!: add agent context #881

refactor!: add agent context #881

Conversation

TimoGlastra commented Jun 19, 2022 • edited Loading

codecov-commenter commented Jun 19, 2022 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimoGlastra commented Jun 19, 2022 • edited Loading

berendsliedrecht commented Jun 19, 2022

berendsliedrecht left a comment

Choose a reason for hiding this comment

berendsliedrecht left a comment

Choose a reason for hiding this comment

TimoGlastra commented Jul 3, 2022

TimoGlastra commented Jun 19, 2022 •

edited

Loading

codecov-commenter commented Jun 19, 2022 •

edited

Loading

TimoGlastra commented Jun 19, 2022 •

edited

Loading