x/ibc: implement ADR 032 typed events #7762

akhilkumarpilli · 2020-10-30T15:55:24Z

Description

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

…to events-adr

akhilkumarpilli · 2020-11-25T15:52:49Z

@jackzampolin ParseTypedEvent is failing with error: json: cannot unmarshal number into Go value of type string. Tests for this are present here

colin-axner · 2020-11-25T16:30:01Z

proto/ibc/applications/transfer/v1/event.proto

+}
+
+// EventOnTimeoutPacket is a typed event emitted on packet timeout
+message EventOnTimeoutPacket {


this should be in channel along with all the events above

colin-axner

The Height type shouldn't be changed to pointer (this is would also make this pr a breaking change). Is there a reason this was done? The Height interface is only used to avoid circular dependencies but we intentionally constructed the code such that the concrete Height type is the only one used. We should definitely not be treating the Height type as an Any.

proto/ibc/applications/transfer/v1/tx.proto

proto/ibc/core/channel/v1/event.proto

proto/ibc/core/client/v1/event.proto

amaury1093 · 2020-11-25T17:15:24Z

re #7762 (comment), I explored the issue with @akhilkumarpilli a little bit.

In old events, when we have an event with {attributeKey}={attributeValue}, only string and []byte were allowed for the attributeValue. strings were encoded as-is, without "" around them.

In new typed events, it seems that we want to put any type of value (strings, numbers, also arrays and structs??) inside attributeValue. What's the encoding we use here then? Akhil's error #7762 (comment) is just a wrong encoding used.

I think attributeValue as json.RawMessage would be ideal. However, strings will have "" around them, and I'm proposing to make search events work with "".

colin-axner

Diffs look a lot better. New way of emitting events looks great. Do you think you could update or event spec docs as well? It's be under x/ibc/applications/transfer/spec and x/ibc/core/spec

proto/ibc/core/channel/v1/event.proto

proto/ibc/core/connection/v1/event.proto

x/ibc/applications/transfer/keeper/msg_server.go

x/ibc/applications/transfer/keeper/keeper.go

x/ibc/applications/transfer/module.go

x/ibc/core/04-channel/handler.go

fedekunze · 2020-11-30T17:29:26Z

proto/ibc/applications/transfer/v1/event.proto

@@ -0,0 +1,23 @@
+syntax = "proto3";
+package ibc.applications.transfer.v1;


super nit, but can we rename these proto files to events.proto?

fedekunze · 2020-11-30T17:30:20Z

proto/ibc/applications/transfer/v1/event.proto

+    bytes trace_hash = 1 [
+        (gogoproto.casttype) = "github.com/tendermint/tendermint/libs/bytes.HexBytes",


nit, I'd just use string here

fedekunze · 2020-11-30T17:32:27Z

types/events.go

+	txEvents = "tm.event='Tx'"
+	blEvents = "tm.event='NewBlock'"


I'm pretty sure there are constants for this on Tendermint?

fedekunze · 2020-11-30T17:33:08Z

types/events.go

+	// Replace "." in event type with "-" to fix tm event query issue
+	evtType := strings.ReplaceAll(proto.MessageName(tev), ".", "-")


is this from tendermint? is there an issue that we can track there?

This is the actual issue we have: #7762 (comment)

fedekunze · 2020-11-30T17:34:55Z

types/events.go

+		var txResult abci.TxResult
+		txResBytes, err := json.Marshal(rev.Data)
+		if err != nil {
+			return nil, err
+		}
+		if err := json.Unmarshal(txResBytes, &txResult); err != nil {
+			return nil, fmt.Errorf("failed to unmarshall into abci.TxResult: %s", string(txResBytes))
+		}
+		return txResult.Result.Events, nil


can we use a private function for each of the cases? just for ease of readability

x/ibc/applications/transfer/module.go

AdityaSripal

Looks like this feature will help a lot!

Though there does seem to be a lot of marshalling into JSON and then immediately unmarshalling back into some struct. I wonder how much of this is strictly necessary and how much can be simplified and done without any marshalling.

It seems like at least some of this code will be called for each typed event on each full node. So it is crucial to limit the amount of serialization/deserialization that we are forcing every node to do.

AdityaSripal · 2020-11-30T19:33:00Z

types/events.go

 	evtJSON, err := codec.ProtoMarshalJSON(tev, nil)
 	if err != nil {
 		return Event{}, err
 	}

-	var attrMap map[string]json.RawMessage
+	var attrMap map[string]interface{}
 	err = json.Unmarshal(evtJSON, &attrMap)


Is there something simpler we could do here rather than marshaling and then unmarshalling back again?

AdityaSripal · 2020-11-30T19:38:10Z

types/events.go

+		case string:
+			valueBz = []byte(v)


Is this special case necessary? What's the harm in JSON marshalling everything and then JSON unmarshalling?

AdityaSripal · 2020-11-30T19:43:29Z

types/events.go

+		var value interface{}
+		err := json.Unmarshal(attr.Value, &value)
+		if err != nil {
+			value = string(attr.Value)


Note it seems possible that a module could emit an event string and have it get unmarshalled in JSON.

Imagine that an end user had control over an event attribute. They could construct the string such that it is valid JSON. This function will then successfully unmarshal into interface whatever the user wanted to create and store it into the attribute value.
I'm not sure there's an obviously exploitable issue here, but it depends on the application. So we should make sure to prevent this.
I recommend always JSON marshalling the attribute value in TypedEventToEvent and always unmarshalling here. If the JSON unmarshal fails then we simply return the error.

AdityaSripal · 2020-11-30T19:45:12Z

types/events.go

@@ -137,6 +165,37 @@ func ParseTypedEvent(event abci.Event) (proto.Message, error) {
 	return protoMsg, nil
 }

+// ResultEventToABCIEvent takes the ctypes.ResultEvent and casts it to abci.TxResult, extracting the []abci.Event
+func ResultEventToABCIEvent(rev ctypes.ResultEvent) ([]abci.Event, error) {


Shouldn't this also return some sort of boolean flag indicating whether the returned events are TxEvents or BlockEvents?

AdityaSripal · 2020-11-30T19:51:16Z

types/events.go

+		txResBytes, err := json.Marshal(rev.Data)
+		if err != nil {
+			return nil, err
+		}
+		if err := json.Unmarshal(txResBytes, &txResult); err != nil {


Why are we doing this marshalling and unmarshalling?

Can't we just cast rev.Data to tmtypes.EventDataTx?

And then we can get the abci.TxResult inside?

https://github.com/tendermint/tendermint/blob/master/rpc/core/types/responses.go#L233
https://github.com/tendermint/tendermint/blob/e13b4386ff1a46a5e8879e11bd653b62d4ca0992/types/events.go#L86

AdityaSripal · 2020-11-30T19:51:28Z

types/events.go

+		bl, err := json.Marshal(rev.Data)
+		if err != nil {
+			return nil, err
+		}
+		if err := json.Unmarshal(bl, &blResult); err != nil {


AdityaSripal · 2020-11-30T19:53:23Z

x/ibc/applications/transfer/module.go

-	)
+	channel, _ := am.keeper.GetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel())
+
+	if err := ctx.EventManager().EmitTypedEvent(


ditto @colin-axner 's comment above

AdityaSripal · 2020-11-30T19:53:45Z

x/ibc/applications/transfer/module.go

+	channel, _ := am.keeper.GetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel())
+
+	if err := ctx.EventManager().EmitTypedEvent(
+		&channeltypes.EventChannelTimeoutPacket{


ditto @colin-axner's comment above

colin-axner · 2020-12-01T11:40:27Z

proto/ibc/applications/transfer/v1/events.proto

@@ -12,12 +12,19 @@ message EventTransfer {
    string receiver = 2;
 }

+// EventAcknowledgementSuccess is a typed event emitted on packet acknowledgement success
+message EventAcknowledgementSuccess {


these can go in channel. We have a acknowledgement success/error type defined in channel so other applications may want to use this event. It's usage would make it easier on relayers as well

colin-axner · 2020-12-01T11:54:01Z

x/ibc/applications/transfer/keeper/msg_server.go

-		),
-	})
+	if err := ctx.EventManager().EmitTypedEvent(
+		&types.EventTransfer{


Should we be emitting the token denom and amount here too? This information is redundant since the packet data will always be emitted, but maybe it is useful for exchanges to be notified that a certain transfer occurred?

cc @fedekunze @AdityaSripal

It might be useful yes, don't see a reason why it shouldn't be emitted

colin-axner · 2020-12-01T11:56:01Z

x/ibc/applications/transfer/keeper/relay.go

-	)
+	if err := ctx.EventManager().EmitTypedEvent(
+		&types.EventDenominationTrace{
+			TraceHash: traceHash.String(),


we should emit the amount and receiver here as well

colin-axner · 2020-12-01T11:59:55Z

x/ibc/applications/transfer/module.go

-	ctx.EventManager().EmitEvent(
-		sdk.NewEvent(
-			types.EventTypePacket,
-			sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName),


We still want to emit this information (It isn't 04-channel logic). Ideally we want to bundle this information with whether the ack was a success or failure. Is it possible to embed a event type into another event easily.

I'm thinking we have the ack success/fail event in channel which also get added an event type applications (such as transfer) can embed their own custom event

relevant previous discussion

I think we need to create separate events. We can't embed one event into other.

colin-axner · 2020-12-01T12:00:08Z

x/ibc/applications/transfer/module.go

-		sdk.NewEvent(
-			types.EventTypeTimeout,
-			sdk.NewAttribute(sdk.AttributeKeyModule, types.ModuleName),
-			sdk.NewAttribute(types.AttributeKeyRefundReceiver, data.Sender),


ditto, we still need this

anilcse · 2020-12-14T17:52:58Z

re #7762 (comment), I explored the issue with @akhilkumarpilli a little bit.

In old events, when we have an event with {attributeKey}={attributeValue}, only string and []byte were allowed for the attributeValue. strings were encoded as-is, without "" around them.

In new typed events, it seems that we want to put any type of value (strings, numbers, also arrays and structs??) inside attributeValue. What's the encoding we use here then? Akhil's error #7762 (comment) is just a wrong encoding used.

I think attributeValue as json.RawMessage would be ideal. However, strings will have "" around them, and I'm proposing to make search events work with "".

I also in favour of adding support to search by " for strings from tendermint. Wdyt @marbar3778 @sahith-narahari @alessio ? more info here

colin-axner · 2021-01-25T11:27:47Z

what's the status of this pr?

anilcse · 2021-01-26T12:39:53Z

what's the status of this pr?

Hello @colin-axner , we stuck at querying typed events. Because it's storing extra quotes " for strings and we are not able to use quotes for searching. Best fix we could think of is allowing search with " from tendermint side. Or making some hack around TypedEventToEvent to avoid/remove extra double-quotes (") for strings after marshaling. We tried a few workarounds, nothing seemed to be working as expected.

anilcse · 2021-01-26T13:12:15Z

re #7762 (comment), I explored the issue with @akhilkumarpilli a little bit.
In old events, when we have an event with {attributeKey}={attributeValue}, only string and []byte were allowed for the attributeValue. strings were encoded as-is, without "" around them.
In new typed events, it seems that we want to put any type of value (strings, numbers, also arrays and structs??) inside attributeValue. What's the encoding we use here then? Akhil's error #7762 (comment) is just a wrong encoding used.
I think attributeValue as json.RawMessage would be ideal. However, strings will have "" around them, and I'm proposing to make search events work with "".

I also in favour of adding support to search by " for strings from tendermint. Wdyt @marbar3778 @sahith-narahari @alessio ? more info here

@marbar3778 did you/anyone from tendermint team got a chance to look into this?

tac0turtle · 2021-01-26T13:14:30Z

We have not. I don't think there is an issue related to this in tendermint. Would you want to open one?

anilcse · 2021-01-26T13:27:28Z

We have not. I don't think there is an issue related to this in tendermint. Would you want to open one?

thanks, created an issue tendermint/tendermint#5978

colin-axner · 2021-03-05T15:05:31Z

IBC is no longer in the SDK. It seems to me the path forward with this pr is to:

resolve types/events.go changes necessary in a separate pr. This may require waiting for tendermint changes or figuring out another workaround. Implement ADR 032: Typed Events #7563 can track this work
once the above is resolved, open a pr on the ibc-go repo with the IBC changes

Closing this pr as it is stale and likely easier to open a fresh pr to address the concerns of 1.

akhilkumarpilli and others added 27 commits October 3, 2020 16:06

Add EmitTypedEvent in events

5b539e7

Add parseTypedEvent method

ed85dd7

Use jsonpb

7d9919b

Modify unmarshal proto in events

deb1e3c

Merge branch 'events-adr' of github.com:akhilkumarpilli/cosmos-sdk in…

5e11d14

…to events-adr

Add a test for typed events

171f362

Fix reflect issue in parseTypedEvent

f158344

Modify event tests and add comments

faeab07

Merge remote-tracking branch 'upstream/master' into events-adr

fe5f8ca

Add EmitTypedEvents and refactor other methods

50f3f1f

Merge remote-tracking branch 'upstream/master' into events-adr

45f32f8

Fix golangci-lint issues

fa6a618

Merge remote-tracking branch 'upstream/master' into events-adr

625501d

Update ProtoMarshalJSON params

526d942

Merge remote-tracking branch 'upstream/master' into events-adr

6719759

Merge branch 'master' into events-adr

d8b6883

Address PR comments

5f472bf

Merge remote-tracking branch 'upstream/master' into events-adr

a79c878

Add typed events in ibc transfer

ebb07e3

Merge branch 'master' into events-adr

b16fcd0

x/ibc: migrate EmitEvent to EmitTypedEvents

700f504

Merge branch 'events-adr' into ibc-typed-events

62be11a

Merge remote-tracking branch 'upstream/master' into ibc-typed-events

7f98f3f

Merge branch 'master' into ibc-typed-events

3324f28

Merge branch 'master' into ibc-typed-events

2a7b622

Add TestPackHeight

8d7747e

Merge branch 'master' into ibc-typed-events

123f0c2

akhilkumarpilli requested a review from jackzampolin October 30, 2020 15:55

akhilkumarpilli added 2 commits November 3, 2020 14:15

Use *Height instead of Height

d998bd9

Merge branch 'master' into akhil/7563-ibc-typed-events

5efe687

colin-axner requested review from colin-axner and AdityaSripal November 24, 2020 16:21

colin-axner reviewed Nov 25, 2020

View reviewed changes

colin-axner requested changes Nov 25, 2020

View reviewed changes

proto/ibc/applications/transfer/v1/tx.proto Outdated Show resolved Hide resolved

proto/ibc/core/channel/v1/event.proto Outdated Show resolved Hide resolved

proto/ibc/core/client/v1/event.proto Outdated Show resolved Hide resolved

akhilkumarpilli added 4 commits November 30, 2020 15:01

Revert Height changes

f9eb795

Address PR comments

f3fa0c7

Merge branch 'master' into akhil/7563-ibc-typed-events

d0192c8

Fix error

c00f98d

colin-axner reviewed Nov 30, 2020

View reviewed changes

fedekunze reviewed Nov 30, 2020

View reviewed changes

AdityaSripal requested changes Nov 30, 2020

View reviewed changes

Address few PR comments

7e85d4e

colin-axner reviewed Dec 1, 2020

View reviewed changes

colin-axner mentioned this pull request Jan 25, 2021

Events of the same type are collapsed into one event #8419

Closed

4 tasks

anilcse mentioned this pull request Jan 26, 2021

Allow querying events by strings with double quotes tendermint/tendermint#5978

Closed

colin-axner closed this Mar 5, 2021

alessio deleted the akhil/7563-ibc-typed-events branch March 14, 2021 01:48

colin-axner mentioned this pull request Mar 23, 2021

Standardized Events cosmos/ibc#534

Closed

amaury1093 mentioned this pull request Jul 18, 2022

EmitTypedEvent Escapes Strings #12592

Closed

4 tasks

zale144 mentioned this pull request Aug 21, 2024

Add a EmitTypeEvent function that doesn't add double quotes to string attributes dymensionxyz/sdk-utils#5

Merged

		@@ -0,0 +1,23 @@
		syntax = "proto3";
		package ibc.applications.transfer.v1;

		bytes trace_hash = 1 [
		(gogoproto.casttype) = "github.com/tendermint/tendermint/libs/bytes.HexBytes",

		// Replace "." in event type with "-" to fix tm event query issue
		evtType := strings.ReplaceAll(proto.MessageName(tev), ".", "-")

x/ibc: implement ADR 032 typed events #7762

x/ibc: implement ADR 032 typed events #7762

Conversation

akhilkumarpilli commented Oct 30, 2020

Description

akhilkumarpilli commented Nov 25, 2020

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

amaury1093 commented Nov 25, 2020

colin-axner 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

AdityaSripal 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

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

anilcse commented Dec 14, 2020

colin-axner commented Jan 25, 2021 • edited Loading

anilcse commented Jan 26, 2021

anilcse commented Jan 26, 2021

tac0turtle commented Jan 26, 2021

anilcse commented Jan 26, 2021 • edited Loading

colin-axner commented Mar 5, 2021

colin-axner commented Jan 25, 2021 •

edited

Loading

anilcse commented Jan 26, 2021 •

edited

Loading