-
Notifications
You must be signed in to change notification settings - Fork 179
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[EVM] Remove special EVM type #5791
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5791 +/- ##
==========================================
+ Coverage 55.68% 55.71% +0.02%
==========================================
Files 1094 1094
Lines 85622 85605 -17
==========================================
+ Hits 47681 47693 +12
+ Misses 33326 33297 -29
Partials 4615 4615
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Could you also please remove the EVM location again from the Go SDK? |
fvm/evm/evm_test.go
Outdated
assert.Equal(t, fmt.Sprintf( | ||
"A.%s.%s", | ||
sc.EVMContract.Address.String(), | ||
types.EventTypeBlockExecuted, | ||
), string(blockEvent.Type)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use common.Location.TypeID()
instead. Same below
@m-Peter plans to |
@sideninja Maybe forgot to push? There are no new commits |
@turbolent @sideninja Here is the PR to remove the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sideninja There are a couple more files that ideally should be updated:
- https://github.com/onflow/flow-go/blob/master/model/flow/event.go#L18
- https://github.com/onflow/flow-go/blob/master/model/events/parse.go#L28-L35
- https://github.com/onflow/flow-go/blob/master/model/events/parse_test.go#L46-L57
It would also be very useful to address this suggestion as well: #5414 (comment)
We should also include the event definitions in the EVM
contract itself, for example:
access(all)
event BlockExecuted(
height: UInt64,
hash: String,
...
)
access(all)
event TransactionExecuted(
blockHeight: UInt64,
blockHash: String,
hash: String,
payload: String,
...
logs: String
)
This will be helpful for other tools (such as the Cadence testing framework), to make use of these types cc @turbolent
I did, sorry |
Great feedback, thanks. I like the idea of defining the event in the contract. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Closes: #5414
💔 API Breaking Change
Remove the special event type for evm:
evm.BlockExecuted
andevm.TransactionExecuted
and make them be generic Flow event types:A.{evm contract address}.EVM.TransactionExecuted
andA.{evm contract address}.EVM.BlockExecuted
.