Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
FLIP 243: Flow EVM Gateway #235
base: main
Are you sure you want to change the base?
FLIP 243: Flow EVM Gateway #235
Changes from 6 commits
a646e86
0751850
20045d2
c8a550a
bce3b32
61f7b20
65222ac
8c434ce
595b65b
0b916d9
c56e73e
d1c2d9f
948cf2c
0d74ad6
555b6d5
ea88510
d3b10d7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The integrity mechanism outlined below is a good start though we may need to build in more logic to recover missed events. While the goal is to not miss events we should plan for a reality where it will eventually happen.
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.
Yes, certainly. Back-filling of past events is something we should account for.
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.
I would say this would be good to change in the future as a performance improvement, ideally we would get multiple events and still preserve the order and integrity, however this currently might not be possible yet due to API not providing the heights, but it might be worth linking to an issue and keeping a promise to improve it.
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.
Just to clarify, even with a heartbeat interval of
1
, we still get all the events emitted within the transactions of a single block. You are right though, I guess it will improve performance if we use a larger heartbeat interval when the backend introduces the sequence number field. Quoting the original message from Peter Argue in onflow/flow-evm-gateway#11 (comment)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.
I would also create stress tests that could be run as part of the Flow benchnet, with good metrics on the gateway side we could measure performance. The metrics should also be used in production for monitoring. It would be also worth exploring tracing with OTEL.
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.
Very good suggestion 👏
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.
We expect the community to maintain the EVM Gateways the same way as currently are for AN.
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.
Sounds good, I guess I should introduce more flexible config options then.