making the plonk verifier library external to the light client contract #1718
Conversation
alysiahuggins
requested review from
nomaxg,
philippecamacho,
ImJeremyHe,
sveitser,
jbearer and
tbro
as code owners
.env
Outdated
| DEFENDER_KEY= | ||
| DEFENDER_SECRET= | ||
| DEFENDER_KEY=3vKMu7Dwajrg8GMV3B4qVZpspdocj1Pt | ||
| DEFENDER_SECRET=4TJWVRbw6kWGqR3bTrAQQTBcBQiNduvZvEqUv9o7fAChWZW5pcgqNkkj1RgKjg1b |
There was a problem hiding this comment.
Why are we adding the DEFENDER_SECRET and DEFENDER_KEY to .env ?
.env
Outdated
| @@ -137,3 +137,6 @@ SAFE_ORCHESTRATOR_PRIVATE_KEY= | |||
| # Light Client | |||
| LIGHT_CLIENT_PROXY_CONTRACT_ADDRESS= | |||
| APPROVED_PROVER_ADDRESS= | |||
|
|
|||
| # Plonk Verification Library Deployment with Defender | |||
| PLONK_VERIFIER_SALT=3 | |||
There was a problem hiding this comment.
Just a suggestion - but I think we should not push .env to main as its generally not advised and rather we should have a .env.example which can be converted to .env locally when needed. The reason for this is that someone can by accident add a private key to .env file and push it and bots will extract all the assets from that key - this has happened with a lot of people in the industry. Let me know what your thoughts are on this!
There was a problem hiding this comment.
Thanks for calling this out so we can revise this. Historically, the .env file wasn't used for secrets but for config. The openzeppelin related config/secrets have been removed and placed into a gitignored env file called .env.contracts with a .env.contracts.example file provided as the example. This also resolves the comment above 0e7ab30
contracts/script/Utils.s.sol
Outdated
| ) external returns (string memory filePath, string memory data) { | ||
| string memory outputDir = string.concat( | ||
| vm.projectRoot(), | ||
| "/contracts/script/output/defenderDeployments/", |
There was a problem hiding this comment.
Should we allow this to be a .env variables instead of being hard coded here to make it more configurable?
There was a problem hiding this comment.
Thanks Sneh. Currently, There's an open issue to deal with defender deployment logs #1286 but currently when we deploy with openzeppelin defender, the deployment logs aren't written locally (whereas foundry normally does this). So this is a workaround and is not expected to be used more dynamically. I've put that string into a variable since it has been referenced multiple times in the script. c80c26e
… and removed oppenzeppelin related secrets to the another secret env file which you can replicate via the example file
Bumps [openssl](https://github.com/sfackler/rust-openssl) from 0.10.64 to 0.10.66. - [Release notes](https://github.com/sfackler/rust-openssl/releases) - [Commits](sfackler/rust-openssl@openssl-v0.10.64...openssl-v0.10.66) --- updated-dependencies: - dependency-name: openssl dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
… and removed oppenzeppelin related secrets to the another secret env file which you can replicate via the example file
alysiahuggins
force-pushed
the
1533-make-plonkverifier-an-external-library-again
branch
from
8418d41
to
0e7ab30
Compare
…ttps://github.com/EspressoSystems/espresso-sequencer into 1533-make-plonkverifier-an-external-library-again
alysiahuggins
deleted the
1533-make-plonkverifier-an-external-library-again
branch
Closes #1533
This PR:
In #999 (comment), we changed the visibility of verify() function in library PlonkVerifier from external to internal to placate OZ's defender script. However that includes the PlonkVerifier bytecode in the LightClient contract which makes the file large. Instead, we have changed the visibility to external again and followed the following steps to be able to deploy the upgradeable light client contract via open zeppelin
This PR does not:
forge scriptcommand for deploying or upgrading the LightClient contractKey places to review:
How to test this PR:
contracts/src/READMEto set up the environment to deploy both the PlonkVerifier contract and the LightClient contractThings tested
forge scriptwhich involves OpenZeppelin Defender & Multisig Safe WalletThings not tested