cosmos / ibc

Interchain Standards (ICS) for the Cosmos network & interchain ecosystem.
Other
940 stars 383 forks source link

ICS4: Event emission Specification #1165

Open AdityaSripal opened 2 weeks ago

AdityaSripal commented 2 weeks ago

Emit events for relayers to construct the packet messages

DimitrisJim commented 2 weeks ago

if you could also give an ack on current events spec'ed for CreateChannel/RegisterCounterparty it would be great (e.g I see for RegisterCounterparty we emit the creator which should have been deleted from state after that tx runs, so may not make sense to emit https://github.com/cosmos/ibc/pull/1143/files#diff-f32300fb26667f1d0dc384ad4d8fd45665bd5a7edd6b4e6cb648ae89721d7f13R383)

AdityaSripal commented 2 weeks ago

Done! @DimitrisJim

srdtrk commented 1 week ago

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.

AdityaSripal commented 1 week ago

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

gjermundgaraba commented 1 week ago

@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 375 + 375 * numberOfIndexedParameters + numberOfUnindexedBits

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.

Yes its a bit of a tough needle to thread

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)?

sangier commented 3 days ago

working on the solidity events, I can confirm that if we don't emit events like serdar suggested emit SendPacket(packet) it increases the cost and we end up in a stack too deep issue. I guess that's fine to emit event like suggested as long as all the relevant fields are emitted. The relayer reconstruction should then be custom for the eth side.