-
Notifications
You must be signed in to change notification settings - Fork 31
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
[WIP] OP Fault Proof Dispute Game implementation for op-verifier and op-gateway #36
base: main
Are you sure you want to change the base?
Conversation
Add indentation Co-authored-by: Nick Johnson <[email protected]>
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.
Thank you again for putting this together!
import { StateProof, EVMProofHelper } from "@ensdomains/evm-verifier/contracts/EVMProofHelper.sol"; | ||
import { Types } from "@eth-optimism/contracts-bedrock/src/libraries/Types.sol"; | ||
import { Hashing } from "@eth-optimism/contracts-bedrock/src/libraries/Hashing.sol"; | ||
import "./interfaces/IDisputeGame.sol"; |
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 these published anywhere under @eth-optimism
?
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.
Published on https://www.npmjs.com/package/@eth-optimism/contracts-bedrock
Will change to use @eth-optimism/contracts-bedrock
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'm facing Error HH411: The library src, imported from @eth-optimism/contracts-bedrock/src/dispute/interfaces/IDisputeGame.sol, is not installed. Try installing it using npm.
due to they have migrated to use foundry absolute import path.
I have asked at NomicFoundation/hardhat#5049 but no answer yet
Please let me know when this is ready for review again! |
It should be ready to review. However, let's review this sub PR first: #37
|
50 | ||
); | ||
|
||
const timestampNow = Math.floor(Date.now() / 1000); |
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.
Instead of relying on the local time, we should probably get the latest blocktime.
const games: FindLatestGamesResult[] = | ||
await this.disputeGameFactory.findLatestGames( | ||
respectedGameType, | ||
gameCount - 1, | ||
50 | ||
); | ||
|
||
const timestampNow = Math.floor(Date.now() / 1000); | ||
let disputeGameIndex = -1; | ||
let disputeGameLocalIndex = -1; | ||
|
||
for (let i = 0; i < games.length; i++) { | ||
if (timestampNow - Number(games[i].timestamp) >= this.delay) { | ||
disputeGameIndex = Number(games[i].index); | ||
disputeGameLocalIndex = i; | ||
break; | ||
} | ||
} | ||
|
||
if (disputeGameIndex == -1) throw new Error('Game Not Found'); |
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.
This should probably be refactored into a method called something like findDisputeGame
.
gameType != respectedGameType || | ||
timestamp != games[disputeGameLocalIndex].timestamp | ||
) { | ||
throw new Error('Mismatched Game Data'); |
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.
Can you please define classes for the various errors being thrown here, so callers can identify them?
export type OPGateway = EVMGateway<OPProvableBlock>; | ||
export type OPGateway = EVMGateway< | ||
OPProvableBlock | OPDisputeGameProvableBlock | ||
>; | ||
|
||
export async function makeOPGateway( |
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.
Given that the third argument depends on the type of service - it's either an L2OutputOracle
or a DisputeGameFactory
, I'm not sure it makes sense to have this distinction here. Two separate functions might be clearer.
'number of delay to use (in minutes for DisputeGame)', | ||
'5' | ||
) | ||
.option('-t, --type <number>', '0 - L2OutputOracle, 1 - DisputeGame', '1'); |
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.
version
, perhaps? Or better yet accept two different arguments, to supply either the L2OutputOracle or DisputeGameFactory and choose the implementation on that basis.
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'm thinking to have a separate path prefix like /v1
for L2OutputOracle, /v2
for DisputeGameFactory and /vN
for future upgrades. This would allow old immutable OPVerifier contract to continue working after the gateway upgrade.
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.
Do OP expect to continue to operate the old contracts on the same network once the new ones are deployed?
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 can do it in a simpler way by passing OptimismPortalProxy address instead of either L2OutputOracle or DisputeGameFactory
- OptimismPortal can resolve to l2Oracle only on the old contract
- OptimismPortal can resolve to disputeGameFactory only on the new contract (Because old method is replaced by spacer_54_0_20)
This make it possible to automatically determine whether OP is using an new or old implementation
Types.OutputRootProof outputRootProof; | ||
} | ||
|
||
// https://docs.optimism.io/builders/notices/fp-changes#overview-of-changes |
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.
Some context for this comment would be good.
} | ||
|
||
if (block.timestamp - gameCreationTime < disputeGameDelay) { | ||
revert GameTooEarly(opData.disputeGameIndex); |
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.
Perhaps you can include the age and the minimum age in this error?
Hi @Arachnid following #37 I have made a PR to ccip-read to patch |
97522cf
to
985da03
Compare
* Add crosschain resolver details * Updated @ensdomains/ens-contracts ensdomains/ens-contracts#feature/crosschain-resolver-with-reverse-registrar to the latest commit * Change slot * Add ReverseRegistrar deployment details * Update reverse registrar deployed addresses * Add Op sepolia reverse registrar deployment info * Added L1Resolver contract deployment details * Remove comment out * Add bun.lockb * Update bun.lockb * Change ens- ontracts to point to l2-deployment branch * Fix failing test * Update bun.lockb * Add L2ReverseRegistrar to deps.sol on crosschain-resolver (was throwing error) * Fix failing test * Remove unused subheader * Redeploy base l1 resolver * Update README.md * Added Base Reverse Resolver contract address * Add Arbitrum Reverse Resolver deployement * Redeploy ArbL1ReverseResolver with correct L2 Resolver Address * Redeploy l2 contracts with official reverse record namespace (cointype.reverse) * Sepolia resolvers with analytics (ensdomains#30) * WIP * Fix errors * Ignore favicon * Update bun.lockb * Fix lint error * Update gateway url * Add console.log * Changed endpoint * Update apiEndpoint * Add sender and calldata to props * Move Tracker to evm-gateway * Revert "Move Tracker to evm-gateway" This reverts commit 5d3ba37. * WIP * Downgrade to 4.20231121.0 * Bump it to the latest * Add type * Replace tracker with @ensdomains/server-analytics * Pass custom apiEndpoint and props * Add GATEWAY_DOMAIN and ENDPOINT_URL * Point to correct branch * Fix CORS problem * Add gateway log tracker to OP * Fix lint error * Update readme * Default reverse resolver (ensdomains#33) * Change ens-contracts to use default-reverse-resolver * Add suport for DefaultReverseResolver * Add hexToAddress to extract address * Add faulback to name function * Add fallback for text * Use imported IDefaultReverseResolver * Update bun.lockb and README * Update comment * Move DefaultReverseResolver * Add DefaultReverseResolver * Update bun.lockb * Add support for resolve on DefaultReverseResolver * Deployed new contracts * Add setdefaultname * Update README.md * Update README.md * Remove console.log * Point to default-reverse-resolver-2 * Reswitch to default-reverse-resolver * Update bun.ockb * Add whitespace * Check if invalid address * Override .text * Fix TypeError: ambiguous function error * Re-point to default-reverse-resolver * Added wait * Update storage location after removing Owner * Added wait on crosschain resolver * Use L2ReverseRevolver and fix broken test * Update ens-contracts branch * Eip 5559 support (ensdomains#34) * Add IResolverSetter * Simplify metadata function * Fix failing tests * Add test for EIP 5559 * Added wait * Rename from IResolverSetter to IAddrSetter * Add resolveDeferral * Store chainId directly * Rename from resolveDeferral to setAddr
Should I merge this PR: #34 before continuing the development? |
That PR is now merged, so please go ahead. Can you explain why you think the global variable is causing your issue with |
As per my temporary fix, I have commented Also this blog: https://zuplo.com/blog/2022/03/04/the-script-will-never-generate-a-response suggests a similar cause about the use of global variables. |
This is strange, because |
I'm assuming I should start over from the main branch and incorporate diff... |
Yes please! |
On March 19, 2024, Optimism initiated a significant upgrade on the Optimism Sepolia network to introduce a fault-proof system. Unfortunately, this upgrade broke the functionality of the op-verifier and op-gateway components of the ENS EVM Gateway. Below is an overview of the upgrade's key points.
Reference: https://docs.optimism.io/builders/notices/fp-changes#overview-of-changes
You can see proof on Etherscan that L2OutputOracle has ceased on Optimism Sepolia: https://sepolia.etherscan.io/address/0x90E9c4f8a994a250F6aEfd61CAFb4F2e895D458F
Despite the lack of documentation on upgrading from L2OutputOracle to DisputeGameFactory, we have reverse-engineered the Optimism code and implemented the necessary upgrades to the op-verifier and op-gateway in this pull request. Here is a summary of the changes introduced:
To facilitate a smooth transition, it's crucial to incorporate an activation point system into the op-verifier smart contract. Moreover, to ensure our gateway remains adaptable to future updates, the op-verifier contract should be designed with an upgradeable pattern. However, it's been observed that the EVM Gateway is intended to be immutable, leading us to forego these enhancements for the EVM Gateway. Should you be interested in exploring the implementation of an activation point or an upgradeable smart contract, please let us know.
We have already applied the upgradeable design pattern to all contracts within our Opti.Domains CCIP Gateway and plan to introduce an activation point to our Opti.Domains CCIP Gateway shortly. This approach will enable us to upgrade without disrupting the user experience.
The source code for Opti.Domains CCIP Gateway is available at: https://github.com/Opti-domains/optidomains-ccip-contracts
The Opti.Domains team is eager to continue contributing to developing and maintaining the Optimism verifier and gateway implementations.
The integration of the fault-proof system is scheduled to merge into the Optimism Mainnet in Q3 2024.
Note: This is a work in progress and is not fully tested.