feat: decouple webhook event delivery from sdk#447
Conversation
Signed-off-by: andres vallecilla <andresfelipe083195@hotmail.com>
Signed-off-by: andres vallecilla <andresfelipe083195@hotmail.com>
Signed-off-by: andres vallecilla <andresfelipe083195@hotmail.com>
Signed-off-by: andres vallecilla <andresfelipe083195@hotmail.com>
Signed-off-by: andres vallecilla <andresfelipe083195@hotmail.com>
|
Hey @tarunvadde, it probably depends on which one gets approved first. I still need to implement the changes in the other one. |
genaris
left a comment
genaris
left a comment
There was a problem hiding this comment.
I think the SDK (with its VsAgent object) and its plug-ins should be the ones in charge of emitting events, and the app being the one that listens for them and executes the logic it requires (logging, webhooks, executing callbacks, etc.).
My first suggestion is to simplify the implementation by using VsAgent's internal emitter, and a second one (and bigger in terms of refactoring) would be to move most of the credo-ts message processing (i.e. most of what is currently under apps/vs-agent/BaseMessageEvents and apps/vs-agent/ConnectionEvents) to SDK, in such a way that it serves as an abstraction layer of any complexity given by DIDComm/credo-ts's internal events. In my opinion, ideally the SDK should trigger a MessageReceived event and vs-agent app listen to it and trigger webhooks (or executing presentation callback in case of presentations).
With these two together, I think this refactoring will be significant enough to have a consistent behaviour in terms of event flow.
Signed-off-by: andres vallecilla <andresfelipe083195@hotmail.com>
Signed-off-by: andres vallecilla <andresfelipe083195@hotmail.com>
Signed-off-by: andres vallecilla <andresfelipe083195@hotmail.com>
Signed-off-by: andres vallecilla <andresfelipe083195@hotmail.com>
Signed-off-by: andres vallecilla <andresfelipe083195@hotmail.com>
genaris
left a comment
genaris
left a comment
There was a problem hiding this comment.
It's looking better, but I think it still has some overengineered pieces that can be simplified.
| export function emitVsAgentEvent(agent: VsAgent, eventOrMessage: Event | BaseMessage): void { | ||
| const event = | ||
| eventOrMessage instanceof Event | ||
| ? eventOrMessage | ||
| : new MessageReceived({ timestamp: eventOrMessage.timestamp, message: eventOrMessage }) | ||
|
|
||
| agent.events.emit(agent.context, { | ||
| type: busTypeByEventType[event.type], | ||
| payload: { event }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
I don't see the reason of making this so ugly, i.e. just having a method called emitVsAgentEvent(agent: VsAgent, event: Event) and a separate emitVsAgentMessageReceivedEvent(agent: VsAgent, message: BaseMessage). Or even better: don't create any utility method at all, and just call e.g. agent.events.emit(agent.context, { type: VsAgentEventType.ConnectionStateUpdated, payload: { event } in the method that is actually sending such a message.
Why don't simply follow the event pattern we have in Credo, in such a way we won't need to do these "bus channel names"? I think this is overcomplicating the things.
There was a problem hiding this comment.
Got it. I created a single emitVsAgentEvent function mainly to avoid duplicating the emit logic, but I agree that conceptually it should stay aligned with the Credo event structure.
Signed-off-by: andres vallecilla <andresfelipe083195@hotmail.com>
Signed-off-by: andres vallecilla <andresfelipe083195@hotmail.com>
Signed-off-by: andres vallecilla <andresfelipe083195@hotmail.com>
Signed-off-by: andres vallecilla <andresfelipe083195@hotmail.com>
genaris
left a comment
genaris
left a comment
There was a problem hiding this comment.
LGTM, just a comment to make event naming consistent.
Co-authored-by: Ariel Gentile <gentilester@gmail.com>
Signed-off-by: andres vallecilla <andresfelipe083195@hotmail.com>
Signed-off-by: andres vallecilla <andresfelipe083195@hotmail.com>
Signed-off-by: andres vallecilla <andresfelipe083195@hotmail.com>
3 participants
Changes
EventEmitterclass to handle eventseventswithwebhookUrlconfiguration