Skip to content
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

Flow EVM Gateway FLIP #235

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Flow EVM Gateway FLIP #235

wants to merge 13 commits into from

Conversation

m-Peter
Copy link

@m-Peter m-Peter commented Jan 3, 2024

No description provided.

@m-Peter m-Peter changed the title Add initial draft of Flow EVM Gateway FLIP Flow EVM Gateway FLIP Jan 3, 2024
@m-Peter m-Peter marked this pull request as ready for review January 7, 2024 17:45
If this were to happen, the storage index could end up corrupted, and we would
return incorrect data for things such as account balances, nonces etc.

In the code snippet below, we specify a heartbeat interval of `1` and check
Copy link
Member

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.

Copy link
Author

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)

The SubscribeEvents endpoint has a heartbeatInterval request parameter:
https://github.com/onflow/flow/blob/b7ed2c3e452b5e44561399afc6b5e868e8957117/protobuf/flow/executiondata/executiondata.proto#L140-L148

This is used to set how often the backend will return a response message. If you set it to 1, it will return a response for every block. If the block has no events, the list is empty. If you check the block height from each message increases sequentially, you can verify that you received all messages, and thus all blocks were searched.

The API is currently missing an important sequence number field, which would let you get the same guarantee with larger heartbeatIntervals without receiving a response for every block.

Given missing any events may corrupt your index, I'd recommend using the 1 block heartbeat for now.

We should have some end-to-end tests, perhaps with the usage of the Flow Emulator.
As well as some benchmarks, to assess the service degradation with regards to the
increase of the storage, the incoming requests and possibly the emitted events.

Copy link
Member

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good suggestion 👏


## Questions and Discussion

- Do we expect to have one official EVM Gateway run by the Flow team and
Copy link
Member

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.

Copy link
Author

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.

application/20240103-evm-gateway.md Outdated Show resolved Hide resolved
application/20240103-evm-gateway.md Outdated Show resolved Hide resolved
application/20240103-evm-gateway.md Outdated Show resolved Hide resolved

![Flow EVM Events](./2024-01-03-evm-gateway-resources/flow-evm-events.png)

### Storage Integrity
Copy link
Contributor

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.

Copy link
Author

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.

application/20240103-evm-gateway.md Outdated Show resolved Hide resolved
m-Peter and others added 5 commits January 10, 2024 09:54
Co-authored-by: Gregor G. <75445744+sideninja@users.noreply.github.com>
…rence to MetaMask

Co-authored-by: j pimmel <frankly.watson@gmail.com>
Co-authored-by: j pimmel <frankly.watson@gmail.com>
Co-authored-by: j pimmel <frankly.watson@gmail.com>
Co-authored-by: j pimmel <frankly.watson@gmail.com>
@franklywatson franklywatson added the flip: application Application FLIP label Jan 11, 2024
This was referenced Jan 24, 2024
@franklywatson
Copy link
Contributor

Marked this FLIP as proposed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flip: application Application FLIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants