feat(ctb): Pass msg.sender to dispute games from the factory

[clabby]

Overview

Passes msg.sender from the factory to the dispute game as an immutable argument. This change allows for smart wallets to participate in the protocol as first-class citizens.

Metadata
closes https://github.com/ethereum-optimism/client-pod/issues/732
closes https://github.com/ethereum-optimism/client-pod/issues/731

[clabby]

• feat(ctb): Relax re-proving constraints in OptimismPortal2 #10154
• chore(ctb): Add test for LibClone improvement #10153
• fix(ctb): LibGameType downcast fix #10152
• chore(ctb): Remove payable from resolveClaim #10151
• fix(ctb): LibPosition downcasting #10150
• feat(ctb): Pass msg.sender to dispute games from the factory #10149 👈
• feat(ctb): Subgame rez changes #10148
• develop

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @clabby and the rest of your teammates on Graphite

[coderabbitai]

Walkthrough

Walkthrough

The updates encompass a version increment and modifications across several smart contracts and interfaces within the contracts-bedrock package, focusing on dispute management. Key changes include the addition of new functions, renaming of existing ones, and adjustments in data handling and error definitions to enhance functionality and error clarity in dispute games.

Changes

File Path	Changes Summary
.../src/dispute/DisputeGameFactory.sol	Updated version to "0.5.0", modified clone function to include msg.sender.
.../src/dispute/FaultDisputeGame.sol	Updated version, adjusted data reading, added new functions, and modified calldata size check.
.../src/dispute/interfaces/IDisputeGame.sol	Renamed function, added new getters for enhanced data access.
.../src/dispute/interfaces/IFaultDisputeGame.sol	Removed l1Head() function.
.../src/libraries/DisputeErrors.sol	Renamed error for clarity in data validation.
.../test/dispute/FaultDisputeGame.t.sol	Updated test logic and error handling to align with changes in data validation.

---

Recent Review Details

Configuration used: .coderabbit.yml

Commits

Files that changed from the base of the PR and between 2d354ac and 909d996.

Files ignored due to path filters (7)

• op-bindings/bindings/disputegamefactory.go is excluded by !op-bindings/bindings/**
• op-bindings/bindings/disputegamefactory_more.go is excluded by !op-bindings/bindings/**
• op-bindings/bindings/faultdisputegame.go is excluded by !op-bindings/bindings/**
• op-bindings/bindings/faultdisputegame_more.go is excluded by !op-bindings/bindings/**
• packages/contracts-bedrock/semver-lock.json is excluded by !**/*.json
• packages/contracts-bedrock/snapshots/abi/FaultDisputeGame.json is excluded by !**/*.json
• packages/contracts-bedrock/snapshots/abi/PermissionedDisputeGame.json is excluded by !**/*.json

Files selected for processing (6)

• packages/contracts-bedrock/src/dispute/DisputeGameFactory.sol (2 hunks)
• packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol (5 hunks)
• packages/contracts-bedrock/src/dispute/interfaces/IDisputeGame.sol (1 hunks)
• packages/contracts-bedrock/src/dispute/interfaces/IFaultDisputeGame.sol (1 hunks)
• packages/contracts-bedrock/src/libraries/DisputeErrors.sol (1 hunks)
• packages/contracts-bedrock/test/dispute/FaultDisputeGame.t.sol (4 hunks)

Additional Context Used

Learnings (1)

packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol (1)

User: clabby"
PR: ethereum-optimism/optimism#8351
File: packages/contracts-bedrock/test/dispute/OutputBisectionGame.t.sol:197-200
Timestamp: 2023-12-04T18:04:13.562Z
Learning: The user has indicated that the manual manipulation of storage in tests is an accepted strategy due to the stability of the storage layout and the complexity of introspecting the contract artifact.

Additional comments not posted (14)

packages/contracts-bedrock/src/dispute/interfaces/IDisputeGame.sol (4)

33-37: Renaming gameType to gameCreator and its updated functionality align well with the PR's objectives.

---

38-39: Addition of rootClaim function enhances data handling capabilities in dispute games.

---

43-46: Introduction of l1Head function supports better traceability of the dispute game's creation context.

---

49-49: Addition of extraData function enhances the flexibility of dispute games by allowing additional data association.

packages/contracts-bedrock/src/libraries/DisputeErrors.sol (1)

39-40: Renaming ExtraDataTooLong to BadExtraData improves clarity and specificity in error reporting.

packages/contracts-bedrock/src/dispute/DisputeGameFactory.sol (2)

26-27: Update of semantic version to "0.5.0" is appropriate given the significant changes in contract functionalities.

---

106-116: Inclusion of msg.sender in the clone function enhances traceability and aligns with the PR's objectives to treat smart wallets as first-class entities.

Verification successful

---

The changes to the clone function in DisputeGameFactory.sol have been correctly implemented, and there are no outdated references in other Solidity files. The inclusion of msg.sender in the clone function is verified as per the PR's objectives.

Scripts Executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Search for any remaining references to the old `clone` function signature in the codebase.
rg --type sol 'clone\(.*\)'

Length of output: 57

---

---

Script:

#!/bin/bash
# Search for any remaining references to the old `clone` function signature in the codebase without specifying file type.
rg 'clone\(.*\)'

Length of output: 804

packages/contracts-bedrock/src/dispute/FaultDisputeGame.sol (6)

91-92: Updated version constant to "0.12.0" aligns with the PR's objective to enhance the dispute game mechanism.

---

449-452: The gameCreator function has been added to retrieve the address of the game creator from calldata. This is a new feature that supports the PR's goal of treating smart wallets as first-class entities.

---

456-457: The rootClaim function has been added to retrieve the root claim from calldata. This function supports the enhanced data handling capabilities mentioned in the PR summary.

---

460-461: The l1Head function has been added to retrieve the L1 head hash from calldata. This addition is part of the adjustments to improve data management in the dispute game mechanism.

---

468-468: The extraData function has been adjusted to retrieve 32 bytes of extra data starting from the second word within the calldata. Verify that the offset 0x54 and length 0x20 are correctly used to fetch the intended extra data.

Verification successful

---

The search results confirm that the offset 0x54 and length 0x20 are used in the FaultDisputeGame.sol file to fetch extra data, as specified in the review comment. This is the only occurrence found in the codebase, which aligns with the intended usage described in the review comment. Therefore, the implementation of the extraData_ fetching mechanism using these parameters is verified to be correct.

Scripts Executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Verify that the offset 0x54 and length 0x20 are correctly used to fetch extra data across the codebase.
rg --type solidity $'0x54, 0x20'

Length of output: 66

---

---

Script:

#!/bin/bash
# Verify that the offset 0x54 and length 0x20 are correctly used to fetch extra data across the codebase.
rg '0x54, 0x20' *.sol

Length of output: 71

---

---

Script:

#!/bin/bash
# Verify that the offset 0x54 and length 0x20 are correctly used to fetch extra data across the codebase.
rg '0x54, 0x20' --glob '*.sol' -uu

Length of output: 143

---

521-537: Modified calldata size check to prevent incorrect game UUID creation. This change is crucial for ensuring the integrity of game instance creation. Verify that the expected calldata size 0x7A is consistent with the structure of the calldata used across related contracts.

packages/contracts-bedrock/test/dispute/FaultDisputeGame.t.sol (1)

178-194: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [182-201]

Renamed test function and adjusted logic for _extraDataLen handling.

The renaming of the test function from testFuzz_initialize_extraDataTooLong_reverts to testFuzz_initialize_badExtraData_reverts aligns with the error name change in the contract. The logic adjustment in setting _extraDataLen to skip the exact 32 bytes length by incrementing it ensures that the test covers the scenario where the extra data is just above the valid threshold, which is a good edge case to test.

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 2

[laizy]

@smartcontracts, Sorry to bother you, but I reported this same issue in sherlock-audit/2024-02-optimism-2024-judging#140 as sherlock-audit/2024-02-optimism-2024-judging#194 and didn't receive the reward. hope you could take a look, and contact to the Sherlock team. I'm not specialized in auditing, and this is my first time participating. I joined this competition because I'm passionate about optimism. I've been studying optimism contract code for years and have learned a lot from you!