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

Polygon zkEVM Bridge indexer and API v2 extension #9098

Merged
merged 40 commits into from Feb 14, 2024
Merged

Conversation

varasev
Copy link
Contributor

@varasev varasev commented Jan 4, 2024

Closes #8268.

Motivation

This PR adds an indexer for Polygon zkEVM Bridge operations (Deposits and Withdrawals) and extends API v2 for the corresponding views on UI.

Please note that this PR renames the following env variables:

INDEXER_ZKEVM_BATCHES_ENABLED ➡️ INDEXER_POLYGON_ZKEVM_BATCHES_ENABLED
INDEXER_ZKEVM_BATCHES_CHUNK_SIZE ➡️ INDEXER_POLYGON_ZKEVM_BATCHES_CHUNK_SIZE
INDEXER_ZKEVM_BATCHES_RECHECK_INTERVAL ➡️ INDEXER_POLYGON_ZKEVM_BATCHES_RECHECK_INTERVAL

Other env variables are added in blockscout/docs#217.

Checklist for your Pull Request (PR)

end
end

defp reorg_queue_get(table_name) do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like code duplication with Indexer.Fetcher.PolygonEdge. Does it make sense to introduce a library for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Polygon Edge is not supported anymore by Polygon team and Blockscout doesn't have such instances. We are going to remove Polygon Edge modules from Blockscout soon, so I'd leave it as it is for now

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is my initial comment still applicable because we have now Shibarium related code?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Moreover similar code exists in Optimism branch. Does it make sense to make a framework of re-orgs handling re-usable in all L2-chains?

Copy link
Contributor Author

@varasev varasev Jan 25, 2024

Choose a reason for hiding this comment

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

Yes, I will move these functions to a separate module

Copy link
Contributor Author

@varasev varasev Feb 5, 2024

Choose a reason for hiding this comment

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

Done this in 9888f59 and in 869c924

Copy link
Member

@vbaranov vbaranov left a comment

Choose a reason for hiding this comment

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

@varasev please rebase to master branch. After merging #8929 there are multiple conflicts.

@varasev
Copy link
Contributor Author

varasev commented Jan 25, 2024

@vbaranov Rebased

Copy link
Collaborator

@akolotov akolotov left a comment

Choose a reason for hiding this comment

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

Please provide @doc comments for every public function introduced. If a changed public function does not have @doc comments it will be great if you add it there as well.

@@ -157,6 +158,11 @@ defmodule Indexer.Block.Fetcher do
do: ShibariumBridge.parse(blocks, transactions_with_receipts, logs),
else: []
),
zkevm_bridge_operations =
if(callback_module == Indexer.Block.Realtime.Fetcher,
do: ZkevmBridge.parse(blocks, logs),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this call could make lots of RPC calls, it could delay the block importing, which could cause latency in the new block appearance on UI side. Consider to move logic of getting blocks timestamps and token data (both from DB and RPC) to an async worker (see async_import_remaining_block_data of the realtime fetcher).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, done this in affe983

Comment on lines 286 to 290
query = from(t in BridgeL1Token, select: {t.address, t.id}, where: t.address in ^addresses)

query
|> Repo.all(timeout: :infinity)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make sense to move this to the Reader module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, moved in f4f68ea

Comment on lines 319 to 329
query =
from(
t in BridgeL1Token,
where: t.address in ^token_addresses,
select: {t.address, t.decimals, t.symbol}
)

token_data =
query
|> Repo.all()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make sense to move this to the Reader module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, moved in f4f68ea

token_addresses
|> Enum.map(fn address ->
# we will call symbol() and decimals() public getters
Enum.map(["95d89b41", "313ce567"], fn method_id ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make sense to define the methods selectors as the module attributes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, done in f4f68ea

if status == :ok do
response = parse_response(response)

address = String.downcase(request.contract_address)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a function address_hash_to_string in Indexer.Helper

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored in f4f68ea

query
|> Repo.all()
|> Enum.reduce(%{}, fn {address, decimals, symbol}, acc ->
token_address = String.downcase(Hash.to_string(address))
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a function address_hash_to_string in Indexer.Helper

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored in f4f68ea

tokens_inserted_outside = token_addresses_to_ids_from_db(tokens_not_inserted)

tokens_inserted
|> Enum.reduce(%{}, fn t, acc -> Map.put(acc, Hash.to_string(t.address), t.id) end)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is Hash.to_string(<address>) equal Helper.address_hash_to_string(<address>, false)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, refactored in f4f68ea

tokens_not_inserted =
tokens_to_insert
|> Enum.reject(fn token ->
Enum.any?(tokens_inserted, fn inserted -> token.address == Hash.to_string(inserted.address) end)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is Hash.to_string(<address>) equal Helper.address_hash_to_string(<address>, false)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, refactored in f4f68ea

apps/indexer/lib/indexer/fetcher/zkevm/bridge.ex Outdated Show resolved Hide resolved
@akolotov
Copy link
Collaborator

akolotov commented Feb 1, 2024

@varasev, a general question about the approach to execute L1 RPC requests during the block fetcher job asynchronously. My understanding is that initiating bridge L1 messages happens first, and the corresponding L2 messages appear after some 'finalization' time, correct? So, if we have an independent fetcher (Indexer.Fetcher.Zkevm.BridgeL1?) that looks for L1 messages first, then handling L2 messages will be quite fast during the block fetcher job, without the necessity to make RPC calls to the L1 endpoint.

@varasev
Copy link
Contributor Author

varasev commented Feb 2, 2024

Please provide @doc comments for every public function introduced. If a changed public function does not have @doc comments it will be great if you add it there as well.

Done

@varasev
Copy link
Contributor Author

varasev commented Feb 2, 2024

My understanding is that initiating bridge L1 messages happens first, and the corresponding L2 messages appear after some 'finalization' time, correct?

@akolotov Yes, if you mean Deposit operation. For Withdrawals it's vice versa: L2 message first, then L1 one.

For realtime case on L1:

  • If it's Deposit operation (L1 -> L2), Indexer.Fetcher.Zkevm.BridgeL1 catches BridgeEvent and initiates separate requests to RPC node (to get block timestamp and token data from L1).

  • If it's Withdrawal operation (L2 -> L1), Indexer.Fetcher.Zkevm.BridgeL1 catches ClaimEvent, but doesn't send separate requests to RPC as they only needed for BridgeEvent.

For realtime case on L2:

Indexer.Fetcher.Zkevm.BridgeL2 is only responsible for finding historical operations, so in real time it's off.

For the realtime we have Indexer.Transform.Zkevm.Bridge instead of Indexer.Fetcher.Zkevm.BridgeL2. The transformer catches BridgeEvent and ClaimEvent events without sending RPC requests to L1 for getting block timestamp (as they are already sent by the block fetcher). But for getting L1 token data we still need to send separate RPC requests on L1 for BridgeEvent only. They are now sent asynchronously through Indexer.Fetcher.Zkevm.BridgeL1Tokens, so they don't affect the block fetcher.

These token data RPC requests are not sent by Indexer.Fetcher.Zkevm.BridgeL1Tokens if the token data is already in the database (it could be written there earlier by Indexer.Fetcher.Zkevm.BridgeL1 which works in real time, separately and asynchronously).

Performs a specified number of retries (up to) if the first attempt returns error.
"""
@spec get_blocks_by_events(list(), list(), non_neg_integer()) :: list()
def get_blocks_by_events(events, json_rpc_named_arguments, retries) do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is my understanding correct and if the event list contains 200 records, in the worst case 200 of eth_getBlockByNumber will be sent in one batch? Does it make sense to divide it on chunks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, added chunks in 95e522b

|> Helper.get_blocks_by_events(json_rpc_named_arguments, 100_000_000)
|> Enum.reduce(%{}, fn block, acc ->
block_number = quantity_to_integer(Map.get(block, "number"))
{:ok, timestamp} = DateTime.from_unix(quantity_to_integer(Map.get(block, "timestamp")))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider to use EthereumJSONRPC.timestamp_to_datetime here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in c4ad14b

Copy link
Member

@vbaranov vbaranov left a comment

Choose a reason for hiding this comment

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

Now, since we have INDEXER_POLYGON_EDGE_ and INDEXER_POLYGON_ZKEVM_ envs set families I would suggest change the structure of folders. It would be natural if everywhere instead polygon_edge and zkevm folders we'd have:

polygon
  edge
  zkevm

And I'd rename corresponging modules to have Polygon.Edge, Polygon.Zkevm.

CHANGELOG.md Outdated Show resolved Hide resolved
@varasev
Copy link
Contributor Author

varasev commented Feb 14, 2024

Now, since we have INDEXER_POLYGON_EDGE_ and INDEXER_POLYGON_ZKEVM_ envs set families I would suggest change the structure of folders. It would be natural if everywhere instead polygon_edge and zkevm folders we'd have:

polygon
  edge
  zkevm

And I'd rename corresponging modules to have Polygon.Edge, Polygon.Zkevm.

I renamed Zkevm to PolygonZkevm: d8fd9b2

As PolygonEdge will be removed soon, I left it as it is

@vbaranov vbaranov merged commit 6ae6692 into master Feb 14, 2024
41 checks passed
@vbaranov vbaranov deleted the va-zkevm-bridge branch February 14, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

L1/L2 views for Polygon zkEVM
3 participants