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 deployment + management scripts #140

Merged
merged 29 commits into from
Jun 13, 2024
Merged

Add deployment + management scripts #140

merged 29 commits into from
Jun 13, 2024

Conversation

intoverflow
Copy link
Member

@intoverflow intoverflow commented Jun 5, 2024

Adds management scripts for the smart contracts.

Quick link to readme

Deployed to Sepolia:

@intoverflow intoverflow marked this pull request as ready for review June 6, 2024 05:43
@intoverflow intoverflow assigned flaub and nategraf and unassigned flaub and nategraf Jun 6, 2024
console2.log("Using RiscZeroVerifierRouter at address", address(verifierRouter));

// Deploy new contracts
RiscZeroGroth16Verifier verifier =
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: I need RiscZeroGroth16Verifier here. I'd rather use IRiscZeroVerifier, but I need access to the verifier's SELECTOR field. Do we want to add the notion of "selector" to the interface?

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't because it's not 1:1 in general. A router, for instance, has 0 or more selectors.

RiscZeroVerifierEmergencyStop verifierEstop = RiscZeroVerifierEmergencyStop(vm.envAddress("VERIFIER_ESTOP"));
console2.log("Using RiscZeroVerifierEmergencyStop at address", address(verifierEstop));

RiscZeroGroth16Verifier verifier = RiscZeroGroth16Verifier(address(verifierEstop.verifier()));
Copy link
Member Author

Choose a reason for hiding this comment

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

scheduleDelay: 1
```

Save the e-stop contract address:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have thoughts on where we save these addresses more durably?

# Run forge via fireblocks
fireblocks-json-rpc --http -- \
forge script ${SCRIPT_FILE}:${SCRIPT_FUNCTION} \
--slow --broadcast --unlocked ${FORGE_DEPLOY_FLAGS} \
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to make it easy to run a dry-run first. Maybe extracting --broadcast to a flag on manage would be good.

Copy link
Contributor

Choose a reason for hiding this comment

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

Made a suggestion on this in #145

@nategraf
Copy link
Contributor

nategraf commented Jun 7, 2024

I walked through this and made a PR with some suggestions #145. I got through the commands listed in the README, up until "Grant access to the TimelockController" where I hit an error, and I've got to get going for the day.

- rename DeployEstopVerifier and change vm.startBroadcast() and
vm.broadcast()
- move common functions to base contract
- simulate scheduled calls to avoid finding out they'll fail later
- add a codeblock language
- add --broadcast flag
- fix simulation, error on unset env var
@intoverflow
Copy link
Member Author

I walked through this and made a PR with some suggestions #145. I got through the commands listed in the README, up until "Grant access to the TimelockController" where I hit an error, and I've got to get going for the day.

I tested the PR and didn't hit any issues. My hypothesis: the Grant access to the TimelockController flow happens after the Update the TimelockController minimum delay flow. The latter updates the min scheduling delay to 10 seconds. Thus, if you try to execute the former quickly, you'll get a revert because 10 seconds hasn't elapsed.

Just a guess though. I couldn't repro the issue, so I accepted the PR.

@flaub flaub enabled auto-merge (squash) June 12, 2024 23:55
@flaub flaub merged commit 2d5bde8 into main Jun 13, 2024
9 checks passed
@flaub flaub deleted the tcarstens/deploy branch June 13, 2024 00:06
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.

3 participants