search

matter-labs / era-contracts

Smart Contract Submodule For zkSync Era
MIT License
488 stars 347 forks source link

AcceptAdmin deploy script fails at resume #1045

Open Archethect opened 3 weeks ago

Archethect commented 3 weeks ago

🐛 Script Bug Report

📝 Description

The AcceptAdmin.s.sol script does not work well with the --resume flag. This is also flagged inside the accept_ownership.rs file in the zksync-era repo: https://github.com/matter-labs/zksync-era/blob/5161eeda5905d33f4d038a2a04ced3e06f39d593/zkstack_cli/crates/zkstack/src/accept_ownership.rs#L36

This raises problems when initialising an ecosystem (using zkstack ecosystem init --resume) stalls, and one tries to resume it with the --resume flag.

In this cases the AcceptAdmin.s.sol script will be rerun which fails because it tries to re-add already existing governance operations which the Governance contract will prohibit (governanceAcceptOwner and governanceAcceptAdmin). Similarly, chainAdminAcceptAdmin also fails for the same reason.

🔄 Reproduction Steps

Run zkstack ecosystem init and break it off after AcceptAdmin.s.sol has run once Rerun with zkstack ecosystem init --resume

🤔 Expected Behavior

We would expect the initialisation of the ecosystem resumes succesfully.

😯 Current Behavior

The initialisation fails with 

Traces:
  [1201749] → new AcceptAdmin@0x5b73C5498c1E3b4dbA84de0F1833c4a029d90519
    └─ ← [Return] 5892 bytes of code

  [14327] AcceptAdmin::governanceAcceptOwner(Governance: [0x153528e4db24aF907a22063aB0cbc18a1ca45543], 0x363b65738D63757c39C07993F2CeEE6451Cb2e67)
    ├─ [2350] Governance::owner() [staticcall]
    │   └─ ← [Return] 0x6ADa541C05728B7BCA92ab2536E21228fd8e9Ca3
    ├─ [0] VM::startBroadcast(0x6ADa541C05728B7BCA92ab2536E21228fd8e9Ca3)
    │   └─ ← [Return] 
    ├─ [4130] Governance::scheduleTransparent(Operation({ calls: [Call({ target: 0x363b65738D63757c39C07993F2CeEE6451Cb2e67, value: 0, data: 0x79ba5097 })], predecessor: 0x0000000000000000000000000000000000000000000000000000000000000000, salt: 0x0000000000000000000000000000000000000000000000000000000000000000 }), 0)
    │   └─ ← [Revert] OperationExists()
    └─ ← [Revert] OperationExists()

🖥️ Environment

https://github.com/matter-labs/era-contracts/blob/84d5e3716f645909e8144c7d50af9dd6dd9ded62/l1-contracts/deploy-scripts/AcceptAdmin.s.sol

📋 Additional Context

/

📎 Log Output

Traces:
  [1201749] → new AcceptAdmin@0x5b73C5498c1E3b4dbA84de0F1833c4a029d90519
    └─ ← [Return] 5892 bytes of code

  [14327] AcceptAdmin::governanceAcceptOwner(Governance: [0x153528e4db24aF907a22063aB0cbc18a1ca45543], 0x363b65738D63757c39C07993F2CeEE6451Cb2e67)
    ├─ [2350] Governance::owner() [staticcall]
    │   └─ ← [Return] 0x6ADa541C05728B7BCA92ab2536E21228fd8e9Ca3
    ├─ [0] VM::startBroadcast(0x6ADa541C05728B7BCA92ab2536E21228fd8e9Ca3)
    │   └─ ← [Return] 
    ├─ [4130] Governance::scheduleTransparent(Operation({ calls: [Call({ target: 0x363b65738D63757c39C07993F2CeEE6451Cb2e67, value: 0, data: 0x79ba5097 })], predecessor: 0x0000000000000000000000000000000000000000000000000000000000000000, salt: 0x0000000000000000000000000000000000000000000000000000000000000000 }), 0)
    │   └─ ← [Revert] OperationExists()
    └─ ← [Revert] OperationExists()
Archethect commented 3 weeks ago

I have a fix for this ready which tries to check whether the admin has already been set on the contract before proceeding. Will create a PR asap and link here.

Archethect commented 3 weeks ago

PR: https://github.com/matter-labs/era-contracts/pull/1056