Skip to content
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

Draft: OPUpdatePrestate #13998

Draft
wants to merge 47 commits into
base: develop
Choose a base branch
from
Draft

Draft: OPUpdatePrestate #13998

wants to merge 47 commits into from

Conversation

maurelian
Copy link
Contributor

No description provided.

@maurelian maurelian requested a review from a team as a code owner January 27, 2025 16:19
@maurelian maurelian requested a review from mslipper January 27, 2025 16:19
@maurelian maurelian marked this pull request as draft January 27, 2025 16:44
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is looking pretty good to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this file LGTM

packages/contracts-bedrock/src/L1/OPPrestateUpdater.sol Outdated Show resolved Hide resolved
);
}

function test_updatePrestate_succeeds() public {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

even if they aren't written yet, it will speed up the process if you could add empty test functions for other cases that we'll add here.

Ie. just:

function test_updatePrestate_emptyPrestate_fails(){}

that way we can make sure we're on the same page about what tests are needed.

Copy link
Contributor

@JosepBove JosepBove Feb 6, 2025

Choose a reason for hiding this comment

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

Added my tests, let's talk about the ones that you think are missing on the meeting. Probably testing that 2 inputs work and get correctly reordered if we have fdg and pdg

Comment on lines +93 to +97
bool hasFDG = Claim.unwrap(_prestateUpdateInputs[i].absolutePrestate) != bytes32(0);
AddGameInput[] memory inputs = new AddGameInput[](hasFDG ? 2 : 1);
AddGameInput memory pdgInput;
AddGameInput memory fdgInput;

if (Claim.unwrap(_prestateUpdateInputs[i].absolutePrestate) == bytes32(0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How can we use Claim.unwrap(_prestateUpdateInputs[i].absolutePrestate) != bytes32(0) to determine if there is a FDG when we also revert if Claim.unwrap(_prestateUpdateInputs[i].absolutePrestate) == bytes32(0)?

I'd have expected we determine if there's a FDG by seeing if DisputeGameFactory.gameImpls(GameTypes.CANNON) != address(0).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, that check looks wrong. Should be similar to here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I see. This line dates to when we were accepting an absolutePrestate for each game, and so providing one for the fdg was just taken as an indication that it exists.

Accepting a single absolutePrestate, then querying to see if there is an FDG is a better approach.

@JosepBove JosepBove force-pushed the opcm/updatePrestateHash branch from 9a145b8 to b326e30 Compare February 6, 2025 03:31
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.

4 participants