Refactor proofs contracts to allow reusing contract implementations

[ControlCplusControlV]

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

• Fixes #12421

[ajsutton]

I'd say we need to be specific about the args that we're passing in from the factory. There are a lot of game configuration options that should be hard coded (but are immutables to support testing), some that are chain specific but generic enough for the factory to know about and some that are chain specific and very specific to a particular game type which the factory shouldn't know about.

[ajsutton]

The factory can pass through the L2 chain ID but I don't think it's appropriate for the DisputeGameFactory to know of details of specific fault proof implementations like the absolute prestate. Not all game types will need that and DisputeGameFactory should be generic enough to work with any game implementation.

[ControlCplusControlV]

It is possible to store all constructor args as just generic bytes, which while kind of messy because it hides the actual constructor args (we just store Implementations and byte arrays to pass in as "base" constructor args) I think its the only way to have generic chain-indifferent implementation contracts to clone from each type, but still be adaptable to multiple L2 deployments.

Is there another approach you had in mind for flattened immutable args on games?

[maurelian]

Not all game types will need that and DisputeGameFactory should be generic enough to work with any game implementation

What if the constructor args were stored within the DGF, in a flexible way. So, the current gameImpls mapping just holds the addresses of the impls:

mapping(GameType => IDisputeGame) public gameImpls;

but could be updated to hold the constructor args too:

struct GameData {
	IDisputeGame gameImpl;
	bytes constructorArgs;
}
mapping(GameType => GameData) public gameImpls;

Note that this breaks the gameImpls() interface, but gives an idea of one approach that avoids baking in game specific details.

[ajsutton]

I'm not a huge fan of "here's a random binary blob that configures critical aspects of the dispute game", but this is logically very close to the approach that we're planning where the DisputeGameFactory holds a list of contracts that know how to create specific dispute game types (#153 (comment)). So rather than creating directly it calls those contracts to create the new game contract. Then they can hold whatever game specific configuration they need in an understandable format. Things like absolutePrestate can easily be written to storage instead of being an immutable and so we can follow the MCP pattern and hard forks could be done by just calling setAbsolutePrestate and not need to deploy any new contracts.

[maurelian]

So, the DisputeGameFactory will make reference to a set of game proposal type specific factories?

I find that somewhat confusing, but I can get over it if it means that the proposal factories are MCP compatible, and we don't actually need to worry about the proposals being proxied to some single implementation (right?).

[maurelian]

OK, I found the comment here. Noting that this means that N game types will mean N implementations and N proxies per system.

[ControlCplusControlV]

So, the DisputeGameFactory will make reference to a set of game proposal type specific factories?

I find that somewhat confusing, but I can get over it if it means that the proposal factories are MCP compatible, and we don't actually need to worry about the proposals being proxied to some single implementation (right?).

Do you think this is too much for OPCM? It's a lot of contracts/implementations/deployments but I think it ultimately makes it easier to reason about a lot of chains and should simplify our upgrade process from here a lot

[smartcontracts]

I've been going back and forth on this but I do ultimately think that the Creator pattern is the most flexible while also being the safest. It's much easier to verify things about a contract than it is to verify things about a data blob.

[maurelian]

The more I think about it the more I realize that the Creator pattern is roughly equivalent in complexity to the gameImpl CWIA pattern. Each game has either a gameImpl or a Creator that the DGF uses for game creation.

[clabby]

I don't think the DisputeGameFactory is the right place for these contract addr references to be held.

The factory is meant to be a high-level abstraction that knows nothing about the underlying games. The AnchorStateRegistry + DelayedWETH contracts are too low-level for the factory to know about.

For ex, the AnchorStateRegistry assumes that what is being disputed over is an output root. The DelayedWETH contract assumes a delay in bond payouts. These two things are not always true, especially for extensions to the protocol (i.e. kailua, op-succinct [once Succinct adheres to the IDisputeGame interface...]). The DGF should have no opinions on this, and only perform accounting work for CWIA proxies created.

Holding this abstraction is also particularly useful for us, as we venture into working on interop proofs + multiproofs. What if we want to do something like swap out the AnchorStateRegistry for something more friendly to the super root, and preserve backwards compat while the feature is in progress? The IAnchorStateRegistry interface might be insufficient.

The DGF should continue serving its usecase as a generic middleware for the creation of disputes over arbitrary claims, and allow for arbitrary underlying rulesets, so long as they conform to the IDisputeGame interface.

[ControlCplusControlV]

@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]

@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]

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]

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]

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

164

[smartcontracts]

I've been going back and forth on this but I do ultimately think that the Creator pattern is the most flexible while also being the safest. It's much easier to verify things about a contract than it is to verify things about a data blob.

[clabby]

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]

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

[ajsutton]

LGTM just worth updating the summary.

[ajsutton]

I don't think this got updated to match the creator contract approach - DisputeGameFactory won't contain the arguments need to create each new dispute game now.

changes addressed

[ControlCplusControlV]

Merging with updated changes + some formatting fixes, had approval from design meeting (which is now fully reflected in the doc) + review fixes are now merged in