ICS4: Event emission Specification #1165
Conversation
AdityaSripal
requested review from
damiannolan,
sangier and
angbrav
as code owners
|
if you could also give an ack on current events spec'ed for |
|
Done! @DimitrisJim |
| sequence: sequence, | ||
| packet: packet, | ||
| timeoutTimestamp: timeoutTimestamp, | ||
| destId: channel.counterpartyChannelId, |
There was a problem hiding this comment.
I see these are defined as bytes in spec atm (for eth side of things where these are a contract address I assume?). Should they be hex encoded too in spec?
There was a problem hiding this comment.
Mmmm this is a good point. In ibc-go this is an alphanumeric string. It feels wasteful to convert that to a hex string only to convert it back. Tho it is possible that other implementations use bytes that do not decode to ASCII characters.
What do you think @gjermundgaraba ?
There was a problem hiding this comment.
It is a string in Solidity (which is technically dynamic bytes array), but I don't think we suffer too much on this. Unless it was a fixed-length byte array I don't think we would save much on Solidity.
| // Event Emission | ||
| emitLogEntry("sendPacket", { | ||
| // Event Emission for send packet | ||
| emitEvents("send_packet", { | ||
| sourceId: sourceChannelId, |
There was a problem hiding this comment.
Is there any particular reason why we use sourceId here rather than sourceChannelId? I don't care too much either way, but why not use the same names as the packet structure?
| sequence: sequence, // value is string in decimal format | ||
| timeoutTimestamp: timeoutTimestamp, // value is string in decimal format | ||
| payloadLength: len(payloads), // value is string in decimal format | ||
| // include first payload data in events if there is only one payload |
There was a problem hiding this comment.
Does that mean that if this is a multi-payload packet, these need to be empty strings?
There was a problem hiding this comment.
I think we can still fill these in.
There was a problem hiding this comment.
Right, yes because you would check the length to find out if there should be more to look for.
| // reconstructed by relayers | ||
| for i, payload in payloads { | ||
| emitLongEntry("send_payload", { | ||
| sourceId: sourceChannelId, |
There was a problem hiding this comment.
Same question as above regarding naming differences
| emitLogEntry("writeAcknowledgement", { | ||
| sequence: packet.sequence, | ||
| emitEvents("write_acknowledgement", { | ||
| sequence: packet.sequence, // value is string in decimal format |
There was a problem hiding this comment.
nit/question: Curious about the use of the word "decimal" here. I immediately think of a number with some kind of precision. Would it be simpler to say integer?
Also, what is the reason for using string-formatted numbers here?
There was a problem hiding this comment.
I mean that the string is base10 as opposed to hexadecimal or something else.
This is assuming the event system is a <string, string> attribute system.
| @@ -784,14 +825,49 @@ function writeAcknowledgement( | |||
| // Note that the event should be emitted by this function only in the asynchrounous ack case. Otherwise the event is emitted during the onReceive | |||
| packet=getPacket(destChannelId,sequence) | |||
| if(packet!=nil){ | |||
| emitLogEntry("writeAcknowledgement", { | |||
| sequence: packet.sequence, | |||
| // construct acknowlegement event by concatenating each app acknowledgment together with a "/" delimiter | |||
There was a problem hiding this comment.
Since we emit separate events for multiple payloads, could we not do the same here? Is it because it would be simpler for an event handler to always have to deal with a single writeAcknowledgement event per packet?
There was a problem hiding this comment.
We could do this yes. Think I just did this before thinking about the payload case
|
I think this is mostly written under the assumptions that all events must look like key-value pairs where both key and value must be string. Also assumes that there must be a key identifying the event. In solidity it is not necessarily like this, and serializing the packet this way would likely cost a lot of gas. Instead we'd be better off just emitting the full packet (abi encoded) each time rather than emitting each individual field. For example: emit SendPacket(packet)I don't see any issue with this unless we have to implement the events this way in every implementation. |
|
Yes its a bit of a tough needle to thread which is why we faced this problem in the v1 spec. Note the exact view of the events is not strictly necessary to be followed in order to be a compliant implemention (only provable key/value logic is absolutely necessary). If different implementations emit events the same way, then the work on the relayer is simpler since they will use the same parsing and reconstruction logic. We could also just tell different implementations what they have to emit without specifying how, then relayers must build custom integrators for each implementation. This has been a consistent complaint and request from relayer teams to standardize and specify the events for the v1 spec. Open to doing things differently but should ensure its ok with potential relayer implementations |
| // Event Emission for receive packet | ||
| emitEvents("recv_packet", { | ||
| sourceChannel: sourceChannelId, | ||
| destChannel: channel.counterpartyChannelId, |
There was a problem hiding this comment.
meant to drop comment here, counterparty channel id on recv will be equal to source channel no? Should be packet.DestChannel?
|
@srdtrk @AdityaSripal one other thing we have not looked too closely at is how different way of structuring the events affect gas cost on Ethereum. I would need to dive a bit deeper into it, but the general way it works is Currently, I don't believe we haven't added any extra indexed parameters, so we are mostly concerned with size. So where we can keep size smaller, it will be advantageous. Obviously any marshaling/encoding we have to do first is also very much relevant.
Would at least being able to use the local "type" for different variables such as timestamp and so on be OK in this context (rather than having to encode it to string)? |
| }) | ||
| } | ||
|
|
||
| // Event Emission for receive packet. emit again so relayer can reconstruct the packet |
There was a problem hiding this comment.
Note that re-emission may be expensive. Can't relayers store somewhere these events instead?
|
working on the solidity events, I can confirm that if we don't emit events like serdar suggested |
| // for multi payload cases, we will emit each payload as a separate event | ||
| // these will include the packet identifier so they can be indexed and | ||
| // reconstructed by relayers | ||
| for i, payload in payloads { |
There was a problem hiding this comment.
In the single payload case, a send_payload event would be emitted anyway. Since we emit an event with the first payload related data in the send_packet event, this results in a duplicated field emission.
Do we want to emit the send_payload event even in the single payload case?
If not, shall we discard the first element of the payload array here? Alternatively, shall we remove version,encoding,data from the send_packet event?
Emit events for relayers to construct the packet messages