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

Add Safe to L2 Setup Contract #759

Merged
merged 15 commits into from
Jul 3, 2024
Merged

Add Safe to L2 Setup Contract #759

merged 15 commits into from
Jul 3, 2024

Conversation

nlordell
Copy link
Collaborator

@nlordell nlordell commented May 7, 2024

This PR introduces a setup contract that can be called from the Safe setup function in order to automatically promote a Safe at setup time if the code is executing on an L2. Namely, this allows the Safe Proxy factory to use a single singleton and initializer for all chains, but end up with different singletons depending on the chain ID.

The expected use of this contract is to use the standard proxy factory:

Safe l1Singleton;
SafeL2 l2Singleton;
SafeToL2Setup l2Setup;

proxyFactory.createProxyWithNonce(
    address(l1Singleton),
    abi.encodeCall(
        l1Singleton.setup,
        (
            owners,
            threshold,
            address(l2Setup),
            abi.encodeCall(l2Setup.setupToL2, address(l2Singleton)),
            fallbackHandler,
            paymentToken,
            payment,
            paymentReceiver
        )
    ),
    saltNonce
)

On L1 (i.e. Ethereum Mainnet where chainId == 1), you would end up with a Safe where safe.singleton == l1Singleton and on any other chains, you would end up with safe.singleton == l2Singleton. This would happen before the first transaction.

This PR introduces a setup contract that can be called from the `Safe`
setup function in order to automatically promote a Safe at setup time if
the code is executing on an L2. Namely, this allows the Safe Proxy
factory to use a single singleton and initializer for all chains, but
end up with different `singleton`s depending on the chain ID.
@coveralls
Copy link

coveralls commented May 7, 2024

Pull Request Test Coverage Report for Build 9780086819

Details

  • 12 of 12 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 93.017%

Totals Coverage Status
Change from base Build 9710052157: 0.1%
Covered Lines: 400
Relevant Lines: 412

💛 - Coveralls

Copy link

@davidaucoin7377 davidaucoin7377 left a comment

Choose a reason for hiding this comment

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

Test run

@@ -71,7 +71,7 @@ jobs:
solidity: ["0.7.6", "0.8.24"]
include:
- solidity: "0.8.24"
settings: '{"viaIR":true,"optimizer":{"enabled":true,"runs":1000000}}'
settings: '{"viaIR":false,"optimizer":{"enabled":true,"runs":1000000}}'
Copy link
Member

Choose a reason for hiding this comment

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

I set this to false because hardhat fails to infer the revert reason in some tests.

test/utils/setup.ts Show resolved Hide resolved
@mmv08 mmv08 self-requested a review July 2, 2024 14:19
@mmv08 mmv08 marked this pull request as ready for review July 2, 2024 14:19
@@ -67,7 +68,7 @@
"eslint-config-prettier": "^9.1.0",
"eslint-plugin-no-only-tests": "^3.1.0",
"eslint-plugin-prettier": "^5.1.3",
"hardhat": "^2.22.3",
"hardhat": "^2.22.6",
Copy link
Member

Choose a reason for hiding this comment

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

hardhat really wanted me to update to the latest version

.env.sample Outdated Show resolved Hide resolved
hardhat.config.ts Outdated Show resolved Hide resolved
test/utils/strings.ts Outdated Show resolved Hide resolved
const { safeToL2SetupLib } = await setupTests();
const randomAddress = ethers.hexlify(ethers.randomBytes(20));

await expect(safeToL2SetupLib.setupToL2(randomAddress)).to.be.rejectedWith("GS900");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should document this new error code somewhere.

@Sulthanmh
Copy link

Sulthanmh commented Jul 2, 2024 via email

Copy link
Collaborator Author

@nlordell nlordell left a comment

Choose a reason for hiding this comment

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

LGTM, only some small nits.

@mmv08 mmv08 self-requested a review July 3, 2024 11:55
Co-authored-by: Nicholas Rodrigues Lordello <n@lordello.net>
expect(sameHexString(value, "0".repeat(64))).to.be.true;
}
}
}
Copy link
Collaborator Author

@nlordell nlordell Jul 3, 2024

Choose a reason for hiding this comment

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

🤯 - generally, well documented and not too difficult to follow.

Copy link
Collaborator Author

@nlordell nlordell left a comment

Choose a reason for hiding this comment

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

LGTM - Approved (can't approve in GH because I opened the PR).

Only open question would be, should we add this to the deployment scripts? Although this can be done in a separate PR. Also left some "take-it-or-leave-it" ubernits.

@mmv08 mmv08 merged commit 8f80a83 into main Jul 3, 2024
20 checks passed
@mmv08 mmv08 deleted the experiment/auto-l2-safe branch July 3, 2024 14:59
@github-actions github-actions bot locked and limited conversation to collaborators Jul 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants