Add universal email recovery module #100
Conversation
JohnGuilding
force-pushed
the
feat/add-universal-email-recovery-module
branch
from
909e616 to
453ba3c
Compare
There was a problem hiding this comment.
I haven't added tests for the following actions:
getHandleAcceptanceAction,
getHandleRecoveryAction,
getCompleteRecoveryAction,
getCancelRecoveryAction,
getCancelExpiredRecoveryAction,
This is because we would have to generate a valid proof to pass here, I haven't had time to do this. Module developers would not be directly calling the main recovery functions initially, so I don't think it's super critical that these are covered in the e2e tests right now. This is because these functions are currently called by our relayer infrastructure instead:
handleAcceptance
handleRecovery
completeRecovery
For the cancel actions, would need start a recovery request so again would need to generate a proof to get to that state I guess.
ofc feel free to block on this if you think this is important. I do think we should add these eventually especially when we have client-side proving in 2025, and then it is more likely devs would be calling these functions directly
There was a problem hiding this comment.
yep I think that makes sense - happy to revisit this down the line
|
Question on the unit tests, when I used a deployed module on Sepolia for the e2e tests. Some of my unit tests started failing. I assume they were grabbing state from Sepolia so the assertions became invalid - see commit Update unit tests. So should we keep things like this? Is there a risk that state will change on Sepolia and then the unit tests will start failing again? |
|
ah so you ran the e2e on actual sepolia not the forked one? yeah this is annoying but I think we can leave it like this for now |
…email-recovery-module
I ran:
This runs tests on the fork? |
This PR:
Open questions
UniversalEmailRecoveryModuleis currently only deployed on Base, so this module would only be installable on Base. We do plan on supporting more evm chains but probably not quite as many as Rhinestone and no immediate plans to make more deployments. Are there any additional changes we need to make to account for this?^I added a helper function that uses the correct address based on chain ID. We will likely have differing addresses for different chains initially (Base and testnets). Future deployments will be more consistent with deterministic addresses