ethereum-optimism / design-docs

MIT License
27 stars 20 forks source link

Refactor proofs contracts to allow reusing contract implementations #153

Open ControlCplusControlV opened 3 weeks ago

ControlCplusControlV commented 3 weeks ago

Description

This PR aims to flatten the constructor arguments in DisputeGame.sol so it uses just ProxyWithImmutableArgs rather than Immutable + Immutable variables in its constructor, allowing different factories to all point to the same implementation

Tests

No tests needed for the design doc

Additional context

Currently we need to re-deploy the DisputeGame.sol implementation the factory uses for each new rollup due to its immutable constructor arguments, this PR aims to fix that.

Metadata

ControlCplusControlV commented 1 week ago

@clabby wdyt about storing arbitrary bytes for the base constructor to each CWIA? It keeps each deployment agnostic to the actual underlying solution, but still allows the flattening which simplifies the deployment process greatly. The main downside is just the information loss onchain as base constructor args just become a bytestring

clabby commented 1 week ago

@clabby wdyt about storing arbitrary bytes for the base constructor to each CWIA? It keeps each deployment agnostic to the actual underlying solution, but still allows the flattening which simplifies the deployment process greatly. The main downside is just the information loss onchain as base constructor args just become a bytestring

I think that could work out well, nice idea. You mean just appending some opaque bytes in storage to the base extraData on L164?

If so, that would be nice. Though it would be really great if this were dynamic, i.e. giving the DisputeGameFactory a new mapping of mapping(GameType => IExtraDataFiller), where IExtraDataFiller is a contract that can construct the base extraData.

interface IExtraDataFiller {
    /// @notice Fills the initial `extraData` for a dispute game type.
    /// @param _gameType The dispute game type that the `extraData` is being crafted for.
    /// @returns extraData_ The base `extraData` for the dispute game type. Can be expanded by user input.
    function fill(GameType _gameType) external view returns (bytes memory extraData_);
}

Then, we could also remove the DisputeGameFactory's mandated pass of the parent L1 block hash, and pull that into the IExtraDataFiller used by the dispute games. Nice improvement to the abstraction since all IDisputeGames don't necessarily need the parent L1 block hash, and having it be dynamic allows for dynamic trusted inputs to the CWIA data rather than just constants.

The implementor of that interface could store the L2 chain ID, ASR, & DelayedWETH for the given game type(s), and format accordingly. When upgrading these, it would be a bit easier than the opaque bytes mapping in the DGF since we wouldn't be setting encoded bytes, but just storage slots in the IExtraDataFiller proxy.

One downside of this would be increasing costs of the proposal route. It probably won't be much, but it's an extra STATICCALL and a few SLOADs more.

ajsutton commented 1 week ago

Do we want an IExtraDataFiller or should we abstract one level higher and just delegate creating the actual dispute game contract to a particular contract per game type? ie rather than (with very made up syntax):

bytes extraData = fillers[gameType].fill(gameType);
gameAddr = new ProxyWithArgs(gameImpls[gameType], extraData);

we'd just do:

gameAddr = creators[gameType].create(gameType, proposedOutputRoot, l2BlockNum);

and then I think the gameImpls map goes away entirely and we just have this creators map. That maximises the flexibility of creating a new game contract and we could proxy the creator contract so it can store the l2ChainID and absolute prestate in the proxy storage and follow the MCP pattern properly.

That probably even removes the need for the ChainConfigRegistry since the absolute prestate can easily be updated with a call to the proxy, or if we still wanted to separate them the chain config could just be stored in the creator contract without needing another registry. I think the same thing would work with IExtraDataFiller as well though. I just think the higher abstraction could be useful and it doesn't expose details of the proxy solution DisputeGameFactory is using to the standard interface.

ControlCplusControlV commented 1 week ago

I'll move the proposal to be more in line with @ajsutton, primarily because abstracting at the creation level is really nice in that we can simplify weird interfaces at the creator level, not the DisputeGameFactory level.

Realistically both implementations end up being basically the same (calling ExtraDataFiller -> create, vs calling Creator, who then creates)

ControlCplusControlV commented 1 week ago

Made a design review meeting to finalize and discuss all of this, tried to fit it within everyone's calendar

164

clabby commented 3 days ago

Do we want an IExtraDataFiller or should we abstract one level higher and just delegate creating the actual dispute game contract to a particular contract per game type? ie rather than (with very made up syntax):

bytes extraData = fillers[gameType].fill(gameType);
gameAddr = new ProxyWithArgs(gameImpls[gameType], extraData);

we'd just do:

gameAddr = creators[gameType].create(gameType, proposedOutputRoot, l2BlockNum);

and then I think the gameImpls map goes away entirely and we just have this creators map. That maximises the flexibility of creating a new game contract and we could proxy the creator contract so it can store the l2ChainID and absolute prestate in the proxy storage and follow the MCP pattern properly.

That probably even removes the need for the ChainConfigRegistry since the absolute prestate can easily be updated with a call to the proxy, or if we still wanted to separate them the chain config could just be stored in the creator contract without needing another registry. I think the same thing would work with IExtraDataFiller as well though. I just think the higher abstraction could be useful and it doesn't expose details of the proxy solution DisputeGameFactory is using to the standard interface.

Nice - yes, this is great. Also is much more workable for extensions to the protocol that require different proposal semantics.

ControlCplusControlV commented 2 days ago

Not allowed to make this link public public for some reason, but link for people in OP Labs