fix: Make contract deployer hook EVM-friendly#538
Conversation
|
We should check that this change is compatible with our upgrade flow. Alternatively we can on a higher level pretend that ContractDeployer isn't a hook if caller isn't X. But still we have to figure the exact upgrade flow first. Iiuc, we want to deploy proxy there and then initialize it, so I guess it's already a bit complex (huh): https://github.com/matter-labs/era-contracts/blob/84e5c59386b6b324d6547c9ad551e43f9a22fc93/l1-contracts/contracts/l2-upgrades/L2ComplexUpgrader.sol#L39 UPD: it was decided to keep ContractDeployer hook and remove ContractDeployer contract for now |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
system_hooks/src/call_hooks/contract_deployer_temp.rs:49
- The constant
L2_COMPLEX_UPGRADER_ADDRESSdefined on line 114 duplicatesCOMPLEX_UPGRADER_ADDRESSalready present inaddresses_constants.rs(line 63). Both have the valueB160::from_limbs([0x800f, 0, 0]). The locally defined constant should be removed and the one fromaddresses_constants.rsused instead (it is accessible via the existing wildcard import), to avoid having two constants for the same address that could diverge in the future.
Additionally, the redundant authorization check at line 149 (if caller != L2_COMPLEX_UPGRADER_ADDRESS) inside contract_deployer_hook_inner is now dead code: the outer contract_deployer_temp_hook function already returns early for unauthorized callers before ever calling into this inner function. The inner check should be removed.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
system_hooks/src/call_hooks/contract_deployer_temp.rs:147
- The comment on line 147 contains a typo: "anauthorized" should be "unauthorized".
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| [dependencies] | ||
| ruint = { workspace = true, default-features = false } | ||
| ruint = { workspace = true, default-features = false, features = ["serde"] } |
There was a problem hiding this comment.
The ruint/serde feature is now added unconditionally to the ruint dependency. The existing comment on line 27 says "serde derivation has to be disabled for risc-v target until we update ruint version". Since the workspace now uses ruint 1.16 (which presumably resolved the risc-v issue), the comment on line 27 is now stale and misleading. It should either be updated to reflect the current state (i.e., that ruint 1.16 fixed the risc-v issue), or removed. Additionally, adding the feature unconditionally (rather than only via the serde feature) makes the serde feature's "ruint/serde" entry on line 28 a no-op, since ruint/serde is already always enabled.
Benchmark report
|
What ❔
Ppretend to be an empty account if caller is unauthorized
Why ❔
Is this a breaking change?
Checklist