Skip to content

feat: update design doc for the dispute game creator pattern #275

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

stevennevins
Copy link

@stevennevins stevennevins commented Apr 28, 2025

Description

Updating the design doc for the update to the DisputeGames to remove the immutable arguements from their constructor and instead have the values be passed as part of the CWIA payload for each proxy. This allows the chain specific data to be abstracted from the deployed implementation and for different chains to share the same implementation

Tests

Additional context

Metadata

@stevennevins stevennevins changed the title Feat update creator dgf design feat: update design doc for the dispute game creator pattern Apr 28, 2025
Copy link
Contributor

@ControlCplusControlV ControlCplusControlV left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me, we should have more direct language imo, I also don't think there are that many additional risks with regards to deployment issues across a lot of different chains


# Alternatives Considered

One alternative considered was to pass all chain-specific configuration purely within the `_extraData` field during game creation. This is similar to the proposed approach but would rely solely on `_extraData` instead of utilizing the structured `gameArgs` stored in the factory alongside the CWIA mechanism. While feasible, encoding all configuration into `_extraData` was deemed less explicit and potentially harder to manage compared to storing base configuration per `GameType` in the factory's `gameArgs` and combining it with instance-specific `_extraData` during cloning.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the big problem with this is that it would be completely insecure. Since game creation is permissionless and the extraData is specified when creating the game it would allow permissionless updating of the prestate and other critical inputs.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, definitely a good call out. I might have left out a security consideration in this, but it's definitely a pretty flimsy option

```solidity
// Example _args construction (conceptual, order based on implementation tests)
bytes memory exampleArgs = abi.encodePacked(
GameType gameType, // uint8 (e.g., uint8(0))
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't expect game type to be passed to the dispute game. Would be better to include this in the original deployment - deploying one implementation contract per game type seems reasonable and doesn't make it chain specific.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah agree one game type to one implementation seems reasonable 4184ccb

Comment on lines 76 to 79
uint256 maxGameDepth, // uint256 (e.g., uint256(2 ** 3))
uint256 splitDepth, // uint256 (e.g., uint256(2 ** 2))
Duration clockExtension, // uint64/Duration (e.g., Duration.wrap(3 hours))
Duration maxClockDuration, // uint64/Duration (e.g., Duration.wrap(3.5 days))
Copy link
Contributor

Choose a reason for hiding this comment

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

These are constants in the standard config. Can we just include them in the implementation contract since they will be the same for every L2 we support?

Copy link
Author

Choose a reason for hiding this comment

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

yeah makes sense!

a4f020a

IBigStepper vm, // address
IDelayedWETH weth, // address
IAnchorStateRegistry registry, // address
uint256 l2ChainId // uint256 (e.g., uint256(10))
Copy link
Contributor

@ajsutton ajsutton May 6, 2025

Choose a reason for hiding this comment

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

l2ChainId is not needed for super games. Given this will ship after interop we probably should only update the super contracts and leave FaultDisputeGame and PermissionedDisputeGame as is since they'll be deleted. We can then remove this chain ID arg.

Copy link
Author

Choose a reason for hiding this comment

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

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.

3 participants