Promote Shared Singleton Signer Implementation #413
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nlordell
requested review from
akshay-ap,
mmv08 and
remedcu
and removed request for
a team
nlordell
force-pushed
the
408/use-self-storage
branch
from
15a68d1
to
d945f8b
Compare
nlordell
force-pushed
the
407/promote-shared-signer
branch
from
7c87feb
to
cc18f0d
Compare
Pull Request Test Coverage Report for Build 9062104545Details
💛 - Coveralls |
nlordell
force-pushed
the
408/use-self-storage
branch
from
d945f8b
to
dd263d7
Compare
nlordell
force-pushed
the
407/promote-shared-signer
branch
from
cc18f0d
to
8225143
Compare
remedcu
reviewed
May 10, 2024
There was a problem hiding this comment.
How was the address for SAFE_WEBAUTHN_SHARED_SIGNER_ADDRESS and SAFE_SIGNER_LAUNCHPAD_ADDRESS calculated?
I tried with npx hardhat deploy on localhost, and got this:
deploying "SafeSignerLaunchpad" (tx: 0x0840238cfc0f8e1703a410d914e714e36e8aa60445c77e00ea74796319320b70)...: deployed at 0x7a17673482dbB4586A29dBE60BCF20602F5a21b2 with 1265339 gas
deploying "SafeWebAuthnSharedSigner" (tx: 0x7576a804108f00275b06f738d7939c071923b649f4ed55ac77a19604713c3716)...: deployed at 0x7CB04FC7EDc8a701924024F46937b55C971E3156 with 681440 gas
|
I suspect there was a small change in comments or something related to the rebase. I created a GH issue to track this, so we can redeploy the contracts once the audit is complete (as the addresses may change yet again when we incorporate audit comments): |
remedcu
approved these changes
May 10, 2024
nlordell
force-pushed
the
408/use-self-storage
branch
from
dd263d7
to
e9eb4cc
Compare
Base automatically changed from
408/use-self-storage
to
308/single-setup-unit-tests
May 13, 2024 11:09
nlordell
force-pushed
the
308/single-setup-unit-tests
branch
from
bec13b0
to
8a7eab4
Compare
nlordell
changed the base branch from
308/single-setup-unit-tests
to
408/use-self-storage-2
nlordell
force-pushed
the
407/promote-shared-signer
branch
from
8225143
to
a5cf793
Compare
nlordell
force-pushed
the
408/use-self-storage-2
branch
from
23c2459
to
1e8d2d7
Compare
nlordell
force-pushed
the
407/promote-shared-signer
branch
from
a5cf793
to
9173c38
Compare
Fixes #408 This PR reworks the WebAuthn singleton signer to use the account storage instead of its own storage for signer configuration (x, y, and verifiers configuration values). This allows the shared signer to work **without** a staked factory 🎉! We also rename the methods a bit to be more consistent with our signer factory implementation, and adjust the E2E test accordingly (noting that a staked factory is no longer needed). This also required some small adjustments to the `4337-passkeys-singleton-signer` example for the new code (but overall simplification - no need for a staked factory anymore).
This PR promotes the shared singleton signer implementation to the 4337 directory instead of being a "test" contract. This involves: - Adding it to the deployments - Moving and renaming the contract to remove the `Test` prefix I also took this opportunity to rename it to a "shared" signer instead of "singleton" signer, so that it does not get confused with the other singleton contract (the `SafeWebAuthnSignerSingleton`) that already exists in this repository. Additionally, I moved some of the test files around in order to clearly differentiate between E2E tests and unit tests in the `test/4337` directory. Unit tests for 100% coverage of the `SafeWebAuthnSharedSigner` will be added in a follow up PR.
nlordell
force-pushed
the
407/promote-shared-signer
branch
from
9173c38
to
1365f2f
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR promotes the shared singleton signer implementation to the 4337 directory instead of being a "test" contract. This involves:
TestprefixI also took this opportunity to rename it to a "shared" signer instead of "singleton" signer, so that it does not get confused with the other singleton contract (the
SafeWebAuthnSignerSingleton) that already exists in this repository.Additionally, I moved some of the test files around in order to clearly differentiate between E2E tests and unit tests in the
test/4337directory.Unit tests for 100% coverage of the
SafeWebAuthnSharedSignerwill be added in a follow up PR.