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

[Superchain Ops] Integration Testing Fork Test #13960

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

ElliotFriedman
Copy link

Description

This PR paves the way for fork testing using the state diff result of a task from superchain ops.

Additional context

Superchain ops integration testing will allow us to run the integration tests after the superchain ops task state changes are applied.

Metadata

Include a link to any github issues that this may close in the following form:

@ElliotFriedman ElliotFriedman requested a review from a team as a code owner January 24, 2025 00:29
Copy link
Contributor

@blmalone blmalone left a comment

Choose a reason for hiding this comment

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

first pass.

packages/contracts-bedrock/test/setup/ForkLive.s.sol Outdated Show resolved Hide resolved
packages/contracts-bedrock/test/setup/ForkLive.s.sol Outdated Show resolved Hide resolved
bool useOpsRepo = bytes(vm.envOr("STATE_PATH", string(""))).length > 0;
if (useOpsRepo) {
console.log("ForkLive: loading state from %s", vm.envString("STATE_PATH"));
vm.loadAllocs(vm.envString("STATE_PATH"));
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 it would be useful to include a comment here to say why the order of commands matters here e.g. why don't we 'loadAllocs' after '_readSuperchainRegistry'?

Copy link
Author

Choose a reason for hiding this comment

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

superchain ops might do an upgrade and so we want to be able to pull the logic contracts from the newly updated values instead of the stale values in the current registry.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I believe @blmalone is suggesting to add that as a code comment which is a good idea :)

Also one clarification would be "...from the newly updated values instead of reading the current implementation addresses from the forked chain", since the implementations are read via calls here, not read from the superchain registry

@mds1
Copy link
Contributor

mds1 commented Jan 24, 2025

/ci authorize f505a15

@blmalone
Copy link
Contributor

/ci authorize f505a15

1 similar comment
@blmalone
Copy link
Contributor

/ci authorize f505a15

@blmalone
Copy link
Contributor

/ci authorize ed0e121

Signed-off-by: Elliot <[email protected]>
@blmalone
Copy link
Contributor

/ci authorize 3a7df1e

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