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

feat: add initial l1-contracts-foundry #267

Merged
merged 93 commits into from
Apr 5, 2024
Merged

Conversation

aon
Copy link
Contributor

@aon aon commented Mar 13, 2024

What ❔

  • This PR aims to begin the transition to move away from hardhat into foundry
  • Adds an initial move for l1-contracts into a specific foundry folder

Why ❔

  • To move from hardhat into foundry

Checklist

  • PR title corresponds to the body of PR (we generate changelog entries from PRs).
  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.

Notes

There are some points of discussion I wanted to address before finalizing this PR.

Preprocessing support

#273 removes the need for preprocessing.

@aon aon requested a review from Deniallugo April 4, 2024 16:07
Copy link
Collaborator

@StanislavBreadless StanislavBreadless left a comment

Choose a reason for hiding this comment

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

@Deniallugo I think we can discuss it later (since it is a rather small change to the PR and it is already somewhat big), but overall we should set the governance to the address rightaway (or at least as soon as possible). For localhost scripts it does not matter, but for the mainnet deployments we should have as little risk of human error as possible (i.e. the error when someone forgets to look onto the etherscan to double check that the governance was set correctly).

l1-contracts-foundry/script/DeployL1.s.sol Outdated Show resolved Hide resolved
console.log("StateTransitionManager set in ValidatorTimelock");
}

function deployDiamondProxy() internal {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I undertand correctly this one is only needed for tests and does not need to be always called

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the approach we took with @Deniallugo for this script was to assume nothing was deployed previously. Therefore we're deploying it here. If necessary an option could be added to the config to pass the address manually. If it's present, then we don't deploy the DiamondProxy, otherwise we deploy it. Do you think this might work?

@Deniallugo Deniallugo merged commit 8ec3cfa into matter-labs:dev Apr 5, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants