Feature/update deployment logic using factory #52
Conversation
wshino
commented
Aug 28, 2024
•
wshino
commented
- Update zkey and wasm
- Integration testing using foundry
- Integration testing using foundry zksync
- Update docs for integration testing using foundry zksync
- Implement EmailAccountRecoveryZkSync
- Implement RecoveryControllerZkSync
- Divide test cases
…update-deployment-logic-using-factory
…logic-using-factory
| // console.log("emailAuthProxy.controller() %s", emailAuthProxy.controller()); | ||
| // vm.stopBroadcast(); | ||
| // } | ||
| // } |
There was a problem hiding this comment.
If this is fully commented out, please remove this file.
There was a problem hiding this comment.
The reason why I commented out is it needs for via-ir option when we compile contracts directory.
This is the script that checks deployed address and computeAddress are the same address. And it checks deployedAddress has been initialized correctly.
I think we should keep this script for future use.
There was a problem hiding this comment.
I don't think EmailAccountRecovery should have a new argument/storage factory and introduce a struct ZKSyncCreate2Factory only for zksync.
How about the following approach?
- The
EmailAccountRecoveryabstract contract has two virtual functionsdeployProxyandcomputeProxyAddress. Their default implementations simply callnew ERC1967ProxyandCreate2.computeAddress, respectively. - We additionally define an abstract contract
EmailAccountRecoveryZksync, which inheritsEmailAccountRecoveryand overwritesdeployProxyandcomputeProxyAddress.
|
@SoraSuegami I agree with your opinion. In that case do we add test cases and deployment scripts for EmailAccountRecoveryZksync? (and RecoveryControllerZksync too) |
|
Yes, we need tests and scripts for both. |
| @@ -124,45 +110,49 @@ abstract contract EmailAccountRecovery { | |||
| /// @param recoveredAccount The address of the account to be recovered. | |||
| /// @param accountSalt A bytes32 salt value defined as a hash of the guardian's email address and an account code. This is assumed to be unique to a pair of the guardian's email address and the wallet address to be recovered. | |||
| /// @return address The computed address. | |||
| function computeEmailAuthAddress( | |||
| function computeProxyAddress( | |||
There was a problem hiding this comment.
If you replace computeEmailAuthAddress with computeProxyAddress , could you name this function computeEmailAuthAddress?
There was a problem hiding this comment.
@SoraSuegami Does that mean we leave the function name as computeEmailAuthAddress ?
| /// @param recoveredAccount The address of the account to be recovered. | ||
| /// @param accountSalt A bytes32 salt value used to ensure the uniqueness of the deployed proxy address. | ||
| /// @return address The address of the newly deployed proxy contract. | ||
| function deployProxy( |
There was a problem hiding this comment.
Also, could you rename it to deployEmailAuthProxy?
This function should be private/internal function.
packages/contracts/README.md
Outdated
| @@ -251,15 +251,22 @@ Next, you should uncomment the following lines in `foundry.toml`. | |||
| # via-ir = true | |||
| ``` | |||
|
|
|||
| And then you should uncomment the commented imports and functions in `src/utils/ZKSyncCreate2Factory.sol` | |||
| Uncomment the following commented-out files. | |||
| These are commented out in the files for ZkSync to avoid problems when testing in foundry. | |||
There was a problem hiding this comment.
Is it possible to avoid commenting out those zksync files by adding an option to the command of yarn build that excludes zksync files?
There was a problem hiding this comment.
@SoraSuegami Maybe we can use --skip flag, let me try it.
* Audit fixes for circuits * Update verifier for the new circuit * Fix maskedSubject attack vector and timestamp bug * Allow lower/uppercases address in the subject * Fix typo. * Add test case to check "invalid masked subject length" and "invalid size of the skipped subject prefix". * Add range check to Digit2Int * Add comments about an email address in the subject. * Update snarkjs version * Add idx validations to the circuit * Change range of LessThan * Update zk-email circuit version * Optimize regex_out in the circuit * Fix lastTimestamp update logic * Fix commented out codes * feat: update api * Fix inactive_guardian api * Fix inactive_guardian * fix: patch db query * Add python logger * Revert "Add python logger" This reverts commit 92be537. * Logger in the prover worked (#54) * Update prover version * Logger in the prover worked * Update dependencies * Fix the versions of circuit dependencies * Fix relayer-utils version * Update verifier * Update proving key * Fix integration test on base sepolia * feat: update api * Update prover address. * Fix requirements * feat: update api * Feature/update deployment logic using factory (#52) * Add ZKSyncCreate2Factory, EmailAccountRecovery test casees split by function. * Failed: update integration testing with foundry-zksync. * Update integration testing command with foundry-zksync. * Failed: update integration testing with foundry. * Add DeployEmailAuthWithCreate2.s.sol. * Disable DeployEmailAuthWIthCreate2.s.sol. * Fix integration test for foundry * Fail: integration testing for foundry zksync. * Fix integration testing for foundry zksync * Tweak README.md * Add EmailAccountRecoveryZkSync * Uncomment some contracts which are used for foundry zksync * make deployEmailAuthProxy as internal function * Update yarn build, yarn test * Remove specific foundry version * Update foundry version * chore: git rm --cached * chore: Update after git rm --cached * chore: git rm --cached * chore: Update after git rm --cached * Fix build-test-fmt on github action * Rename DeployEmailAuthWIthCreate2 * Update deployment files in docs --------- Co-authored-by: wshino <[email protected]> Co-authored-by: Aditya Bisht <[email protected]>
