-
Notifications
You must be signed in to change notification settings - Fork 38
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
Add arbi validator #19
Conversation
Just a few notes on what I reviewed and tested. The code mostly consists of two parts
|
…deployment-with-corsify-and-readme-update Update README.md
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.
Thanks for this!
arb-gateway/src/ArbProofService.ts
Outdated
|
||
return AbiCoder.defaultAbiCoder().encode( | ||
[ | ||
'tuple(bytes32 version, bytes32 sendRoot, bytes32 blockHash,uint64 nodeIndex,bytes rlpEncodedBlock)', |
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.
'tuple(bytes32 version, bytes32 sendRoot, bytes32 blockHash,uint64 nodeIndex,bytes rlpEncodedBlock)', | |
'tuple(bytes32 version, bytes32 sendRoot, bytes32 blockHash, uint64 nodeIndex, bytes rlpEncodedBlock)', |
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.
Is it necessary to supply both the block and its hash?
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.
good catch, will be removed
arb-gateway/src/ArbProofService.ts
Outdated
const nodeIndex = await this.rollup.latestNodeCreated() | ||
|
||
//We fetch the node created event for the node index we just retrieved. | ||
const nodeEventFilter = await this.rollup.filters.NodeCreated(nodeIndex); |
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.
Are any of these operations costly? Should we be caching the latest block?
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're making two RPC calls to fetch the latest node from the L1 contract and the corresponding L2 block. Besides that, it's just event parsing. Given that the latestNodeCreated is updated roughly every hour on the mainnet, I think it could be a really good idea to cache both the Node and Block. However, there might be additional infrastructure like Redis needed to do that properly with the serverless Cloudflare worker the gateway is deployed on.
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.
One of the RPCs is an event filter, though, which can be slow and costly. Perhaps we can at least cache the result in-process in a variable?
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 have added a simple in-memory cache that caches the values for a particular nodeIndex. It can be easily replaced by a more sophisticated cache by implementing the IBlockCache interface
arb-gateway/src/ArbProofService.ts
Outdated
@@ -2,6 +2,7 @@ | |||
import { EVMProofHelper, type IProofService } from '@ensdomains/evm-gateway'; | |||
import { AbiCoder, Contract, EventLog, ethers, toBeHex, type AddressLike, toNumber } from 'ethers'; | |||
import rollupAbi from "./abi/rollupABI.js"; | |||
import type { IBlockCache } from './cache/IBlockCache.js'; |
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 think you forgot to commit this file.
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 it was part of .gitignore. It is committed now
No description provided.