Update event validation logic to account for the `evm.*` events #5262

m-Peter · 2024-01-22T10:16:35Z

It turns out that flow service/protocol events, are allowed to have only 2 parts, e.g. flow.AccountCreated.

The two newly-introduced EVM related events:

flow.evm.BlockExecuted
flow.evm.TransactionExecuted

do not comply with this format, hence, they are considered as invalid event types:

rpc error: code = InvalidArgument desc = invalid event filter: invalid event type flow.evm.BlockExecuted: invalid event type: flow.evm.BlockExecuted

m-Peter · 2024-01-22T10:17:15Z

cc @sideninja The previous PR (#5203) was accidentally closed I guess.

sideninja · 2024-01-22T15:03:15Z

model/events/parse.go

@@ -31,7 +31,7 @@ func ParseEvent(eventType flow.EventType) (*ParsedEvent, error) {
 	parts := strings.Split(string(eventType), ".")

 	switch parts[0] {
-	case "flow":
+	case "flow", "evm":


better to reference the https://github.com/onflow/flow-go/blob/master/fvm/evm/types/events.go#L20

@sideninja That's a good point, but it is not an exported constant. Should I export it?

yeah go ahead

Updated in a61609a

ramtinms · 2024-01-22T21:02:49Z

model/events/parse.go

+	"github.com/onflow/flow-go/fvm/evm/types"
 	"github.com/onflow/flow-go/model/flow"
 )


hmm, this looks like a dependency that shouldn't happen.

Mainly added so that we re-use the "evm" constant, but I can remove it. "flow" is also matched against a string literal.

ramtinms · 2024-01-22T21:03:18Z

hmm, I remember there was a discussion in discord to expose the address as if its emitted as part of a contract?

sideninja · 2024-01-23T12:40:53Z

hmm, I remember there was a discussion in discord to expose the address as if its emitted as part of a contract?

Not sure if this is the relevant PR, the PR to introduce this event ID was already merged #5091

On the topic I remember some feedback around that but I believe the conclusion was to keep it same way as we do for service events on flow: flow.AccountCreated etc. Since all these events will always be propagated from the single service account.

m-Peter · 2024-01-23T13:09:12Z

Just like @sideninja mentioned, Janez and Bastian suggested to go with the evm.* version. (https://discord.com/channels/613813861610684416/1167476806333513800/1192880930856124486)
Basically introducing a dedicated location (similar to stdlib.FlowLocation{}) for the EVM events.
Since these are events that cannot be found on the EVM contract, and they are not emitted from the contract itself, it makes more sense to go with this approach. With something like: A.<Address>.EVM.*, developers would expect to find the event definition in the said contract, which is not the case.

janezpodhostnik · 2024-01-23T14:27:39Z

I think I remember that we briefly mentioned that we could have a format like evm.<Address>.<event> where the address would be the address of the EVM storage account since we said that in the future there might be more than one.

sideninja · 2024-01-23T15:20:07Z

I think I remember that we briefly mentioned that we could have a format like evm.<Address>.<event> where the address would be the address of the EVM storage account since we said that in the future there might be more than one.

I'm not sure these events would be emitted by different accounts in the future, even if we shard the storage. That would only mean the access under the hood is done on multiple accounts but the events would still come from the main "service" EVM account unless I'm mistaken.

janezpodhostnik · 2024-01-24T18:58:16Z

If we shard storage, doesnt that mean that a common use case would be that you are only interested in your shard?

sideninja · 2024-01-25T12:42:18Z

If we shard storage, doesnt that mean that a common use case would be that you are only interested in your shard?

I'm not sure that would be the case. The sharding should be hidden from the developer. And thus developer could compose apps from other shards and would be interested in the whole network. I feel that listening to the events such as block created is something you want to in any case do for the whole network not just your storage shard.

janezpodhostnik · 2024-01-25T13:51:00Z

model/events/parse.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"strings"

+	"github.com/onflow/flow-go/fvm/evm/types"
 	"github.com/onflow/flow-go/model/flow"


I think EVM location prefix should go into model/flow/event.go

Updated in dfa6eed

janezpodhostnik · 2024-01-25T13:52:17Z

model/events/parse.go

@@ -31,7 +32,7 @@ func ParseEvent(eventType flow.EventType) (*ParsedEvent, error) {
 	parts := strings.Split(string(eventType), ".")

 	switch parts[0] {
-	case "flow":
+	case "flow", types.EVMLocationPrefix:


the comment should also be updated.

Good point 👍 I missed that.
Updated in dfa6eed

codecov-commenter · 2024-01-25T14:30:17Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (5006941) 55.59% compared to head (1dd87b5) 56.68%.

Files	Patch %	Lines
fvm/evm/types/events.go	16.66%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5262      +/-   ##
==========================================
+ Coverage   55.59%   56.68%   +1.09%     
==========================================
  Files        1002      815     -187     
  Lines       96600    79668   -16932     
==========================================
- Hits        53700    45156    -8544     
+ Misses      38839    31055    -7784     
+ Partials     4061     3457     -604

Flag	Coverage Δ
unittests	`56.68% <28.57%> (+1.09%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…validation logic

…r ParseEvent

sideninja reviewed Jan 22, 2024

View reviewed changes

m-Peter force-pushed the update-flow-event-validation-logic branch from a780063 to 5ae9e19 Compare January 22, 2024 16:36

m-Peter requested review from ramtinms and janezpodhostnik as code owners January 22, 2024 16:42

sideninja approved these changes Jan 22, 2024

View reviewed changes

ramtinms reviewed Jan 22, 2024

View reviewed changes

m-Peter mentioned this pull request Jan 24, 2024

Use Flow Emulator for local development onflow/flow-evm-gateway#20

Closed

janezpodhostnik requested changes Jan 25, 2024

View reviewed changes

m-Peter force-pushed the update-flow-event-validation-logic branch from dfa6eed to 33ffc3f Compare January 25, 2024 14:20

m-Peter requested a review from janezpodhostnik January 25, 2024 14:21

janezpodhostnik approved these changes Jan 25, 2024

View reviewed changes

m-Peter added 3 commits January 25, 2024 22:44

Update event validation logic to account for the evm.* events

a3c1e66

Export the EVMLocationPrefix constant and re-use it in event parsing/…

d349d6a

…validation logic

Move EVMLocationPrefix constant to flow package and update comment fo…

1dd87b5

…r ParseEvent

m-Peter force-pushed the update-flow-event-validation-logic branch from 33ffc3f to 1dd87b5 Compare January 25, 2024 20:44

franklywatson enabled auto-merge January 25, 2024 20:59

franklywatson added this pull request to the merge queue Jan 25, 2024

Merged via the queue into onflow:master with commit e050365 Jan 25, 2024
51 checks passed

sideninja pushed a commit that referenced this pull request Feb 6, 2024

chery-pick #5262

c3318f0

m-Peter deleted the update-flow-event-validation-logic branch February 21, 2024 18:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update event validation logic to account for the `evm.*` events #5262

Update event validation logic to account for the `evm.*` events #5262

m-Peter commented Jan 22, 2024

m-Peter commented Jan 22, 2024

sideninja Jan 22, 2024

m-Peter Jan 22, 2024

sideninja Jan 22, 2024

m-Peter Jan 22, 2024

ramtinms Jan 22, 2024

m-Peter Jan 23, 2024

ramtinms commented Jan 22, 2024

sideninja commented Jan 23, 2024

m-Peter commented Jan 23, 2024

janezpodhostnik commented Jan 23, 2024

sideninja commented Jan 23, 2024 •

edited

Loading

janezpodhostnik commented Jan 24, 2024

sideninja commented Jan 25, 2024

janezpodhostnik Jan 25, 2024

m-Peter Jan 25, 2024

janezpodhostnik Jan 25, 2024

m-Peter Jan 25, 2024

codecov-commenter commented Jan 25, 2024 •

edited

Loading

Update event validation logic to account for the evm.* events #5262

Update event validation logic to account for the evm.* events #5262

Conversation

m-Peter commented Jan 22, 2024

m-Peter commented Jan 22, 2024

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

ramtinms commented Jan 22, 2024

sideninja commented Jan 23, 2024

m-Peter commented Jan 23, 2024

janezpodhostnik commented Jan 23, 2024

sideninja commented Jan 23, 2024 • edited Loading

janezpodhostnik commented Jan 24, 2024

sideninja commented Jan 25, 2024

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

codecov-commenter commented Jan 25, 2024 • edited Loading

Codecov Report

Update event validation logic to account for the `evm.*` events #5262

Update event validation logic to account for the `evm.*` events #5262

sideninja commented Jan 23, 2024 •

edited

Loading

codecov-commenter commented Jan 25, 2024 •

edited

Loading