adapt event types and values to messaging-api spec#3839
adapt event types and values to messaging-api spec#3839Ivansete-status wants to merge 2 commits into
Conversation
|
You can find the image built from this PR at Built from 83bf249 |
Ivansete-status
requested review from
NagyZoltanPeter,
darshankabariya and
fcecin
NagyZoltanPeter
left a comment
NagyZoltanPeter
left a comment
There was a problem hiding this comment.
Approve with reservations. We should not forget the follow up on delivery_module
There was a problem hiding this comment.
These are breaking changes for logos-delivery-module! Needs follow up and new release afterward.
| proc(event: MessageSentEvent) {.async: (raises: []).} = | ||
| callEventCallback(ctx, "onMessageSent"): | ||
| $newJsonEvent("message_sent", event), | ||
| $newJsonEvent("message:sent", event), |
There was a problem hiding this comment.
Why the : is better than _? just curious!
There was a problem hiding this comment.
Also looks like a breaking change 🤔
| info "Message successfully propagated", | ||
| requestId = task.requestId, msgHash = task.msgHash.to0xHex() | ||
| MessagePropagatedEvent.emit( | ||
| MessageSendPropagatedEvent.emit( |
There was a problem hiding this comment.
I think here the spec is wrong and the simpler form MessagePropagatedEvent is the better.
The wording of Send here misleading... compared to 'MessageSentEvent'.
WDYT?
There was a problem hiding this comment.
This is my fault; I should have renamed the events in the code when I cleaned up the spec before the LMAPI release.
I agree with Zoltan here. We should fix the spec, not the code.
MessageSendErrorEvent is technically correct, but MessageErrorEvent stuck and at the "simple send" LMAPI level, the only MessageErrorEvent is the send event, so the discrimination that I added there is doing zero actual work.
Also MessageSendPropagatedEvent -> MessagePropagatedEvent with even less loss of meaning or risk of symbol collision or rot.
What do you guys think? Should we submit a fix to the spec instead?
There was a problem hiding this comment.
Okay fair enough. Thanks for the comments both!
Let's keep this opened and we can adapt the spec once we fully move to lips. logos-co/logos-lips#315
|
Let's carry on the discussion in the following PR: logos-co/logos-lips#333 |
4 participants
While revisiting the messaging-api.md I noticed that there are some minor discrepancies regarding some event types. This PR aims to perform such small adjustments.
Issue