-
Notifications
You must be signed in to change notification settings - Fork 74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor WebAuthn Implementation #320
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add the bounty email before this is audited? Or adding the email doesn't signify anything until we explcitly say somewhere that these files are also available for bounty hunting?
Above and small nit, rest LGTM 👍🏾
X, | ||
Y | ||
); | ||
function _verifySignature(bytes32 message, bytes calldata signature) internal view virtual override returns (bool success) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subjective nit: I resonate more with isValid
than success
in this case.
A similar change in SignatureValidator
as well.
Nice catch, yes we should. |
ebdc3c0
to
124f1fc
Compare
73b5d25
to
3667e8a
Compare
Pull Request Test Coverage Report for Build 8328648516Details
💛 - Coveralls |
This PR refactors the WebAuthn implementation into a library instead of split into multiple contracts. This removes the amount of
CALL
s required for verifying a signature.I did some analysis on the deployment vs. signature verification costs comparing this code,
main
and this change incorporating optimizations from #289 and found this:Meaning that after 7 signatures, even with the slightly larger deployment costs, we would break even by avoiding the extra
CALL
(noting that ABI-encoding the call to theWebAuthnVerifier
is not particularly efficient in the first place).