feat: Abstracting proof verifier in account recovery #84
Conversation
… verifier abstraction
…rict added guardianVerifier
feat: Abstracting proof verifier in account recovery
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
There was a problem hiding this comment.
Thanks Dhruv. Let me get back to you tomorrow about where this should be merged. Some thoughts on those questions:
Who should choose between different verifiers?
The end user chooses the guardian type e.g. ethereum address, email, JWT. But the developer/client side code makes any lower level decisions e.g. circom or noir
How should we choose between different verifiers?
Not sure yet. When choosing between circom and noir, maybe a guardian is on a phone when handling acceptance but is on their computer when when handling recovery, in which case, would you want to dynamically choose between client side (noir) or server side (circom) proving?
Can keep this as an open question. For now lets keep it simple and pass something to indicate a guardian type in when adding a guardian. All of the guardian types can be supported by default
How do we handle nullifiers/replay protection?
haven't had enough time to think about this. Will keep you posted
| /// @param accountSalt A bytes32 salt value used to ensure the uniqueness of the deployed proxy address. | ||
| /// @param verifierInitData The initialization data for the guardian verifier. | ||
| /// @return address The computed address. | ||
| function computeGuardianVerifierAddress( |
There was a problem hiding this comment.
Maybe we want to support non-upgradeable guardian verifiers, in which case this logic would change. Hmm let’s leave it for now, but will think more on this
There was a problem hiding this comment.
Leaning towards non-upgradeable guardians
There was a problem hiding this comment.
we were having similar conversations when designing the EmailSigner, and came to the conclusion we would remove upgradeability by default and only use it where it is needed (email signer would not be upgradeable, but the underlying verifier implementation is used would be)
For this PR, if you have time, would simplify and switch to non-upgradeable guardians
There was a problem hiding this comment.
There was a problem hiding this comment.
I understand, but I would still want to use proxy-clone method for guardian verifiers, so that the deployment cost is minimal.
I can sure remove the upgradeability part here. Let me know if clones work according to you - https://docs.openzeppelin.com/contracts/4.x/api/proxy#Clones
There was a problem hiding this comment.
Yeah that's fair, Clones work
There was a problem hiding this comment.
I checked this further post our conversation on the call, the current implementation with ERC1967 is in fact not upgradeable by default, it's just a proxy, it become upgradeable if you were to use ERC1967Upgrade or the implementation contract ( that being guardian verifier) follow an upgradeable pattern. So it's safe to leave it as such
src/interfaces/IGuardianVerifier.sol
Outdated
| * @return isVerified if the proof is valid | ||
| * | ||
| */ | ||
| function verifyProofStrict( |
There was a problem hiding this comment.
Can you explain the rationale again for having verifyProofStrict and verifyProof
There was a problem hiding this comment.
verifyProofStrict - Recommended to use when proof verification is done on-chain or when called from another contract. This avoids extra gas costs from handling failure cases manually.
verifyProof - Recommended to use when proof verification is done off-chain, saves gas cost. This allows clients to check errors without using extra gas.
There was a problem hiding this comment.
Understood, in that case, would propose 2 changes:
- Not against code duplication necessarily, but because the duplicate code is relatively large and complex, we could implement a design similar to OZ's ECDSA library where we have single function for the core logic which returns error strings, then that can be wrapped in the function that is called onchain which reverts with the appropriate errors like so
- rename
verifyProof->tryVerifyProof& renameverifyProofStrict->verifyProof
This avoids extra gas costs from handling failure cases manually.
Not sure I get this, can you elaborate? You mean gas costs from doing something like _throwError?
saves gas cost. This allows clients to check errors without using extra gas.
verifyProofStrict is view so we wouldn't have to spend gas. The benefit is UX here? (which I'm in favour of)
There was a problem hiding this comment.
Not sure I get this, can you elaborate? You mean gas costs from doing something like _throwError?
yeah user wouldn't have to revert conditionally, but in an ideal scenario revert is always better from the verifier itself
Actually, now when I think more about it, It is not making much sense to return the error string instead of revert. Both offer similar UX and there is no gas benefit of tryVerifyProof
There was a problem hiding this comment.
update the structure as per our discussion and handling the nullifier temporarily in the verifier as well, let me know your thoughts on it
|
If you bump modulekit to latest that should resolve the CI failure |
After update, the CI failures are because of the incompleted tests for now, will work on them. |
Introduction
The current account-recovery flow is specifically design for email based guardians, using email proofs to recover the accounts.
Solution
The idea is to abstract the proof verfier in the account recovery flow. It will support different verifier like zk-email, zk-jwt, OAuth, zk-email.nr
Mutliple guardian verifier implementations are supported for the guardians using the same module, i.e. guardians with different verifier can recover a single account
Key changes
IGuardianVerifierinterface for implementation of verifiers based on different schemesNotes