28 add initial forge test for webauthn verification #52
Conversation
JohnGuilding
force-pushed
the
28-add-initial-forge-test-for-webauthn-verification
branch
from
f2814d5 to
d314bc5
Compare
JohnGuilding
force-pushed
the
28-add-initial-forge-test-for-webauthn-verification
branch
from
d314bc5 to
8a2fca9
Compare
|
@jzaki reviewed and happy with the following commits: |
| @@ -0,0 +1,82 @@ | |||
| // SPDX-License-Identifier: MIT | |||
There was a problem hiding this comment.
hmm I'm inclined to think we may want to put these tests into their own folder. Maybe a unit test folder. Or forge-tests. I don't have a strong opinion but I wonder if we should be more explicit these are different then the other tests
There was a problem hiding this comment.
Agreed. I'm actually leaning towards your second suggestion in your PR. Will update to that:
test/hardhat
test/hardhat/integration
test/forge
| @@ -0,0 +1,45 @@ | |||
| // SPDX-License-Identifier: MIT | |||
There was a problem hiding this comment.
Will the WebAuthn.sol file eventually live in the primitives directory? I feel like this could be a good fit for that
There was a problem hiding this comment.
Yeah I think so good shout. Issue to do that - #55
| ]; | ||
| } | ||
|
|
||
| function getUserOpSignature() |
There was a problem hiding this comment.
Is this function specific to a userOp that is using a webAuthN signature? It may be worth updating the name to reflect that for clarity
There was a problem hiding this comment.
Agree, updated in follow up
| ); | ||
| } | ||
|
|
||
| function getPublicKey() |
There was a problem hiding this comment.
Same here, if this is a WebAuthN public key it may be worth reflecting that in the name, since we will eventually have different types of public keys
There was a problem hiding this comment.
Agree, updated in follow up
| import {SafeWebAuthnPlugin} from "../../src/SafeWebAuthnPlugin.sol"; | ||
|
|
||
| /** Helper contract to expose internal functions for testing */ | ||
| contract SafeWebAuthnPluginHarness is SafeWebAuthnPlugin { |
JohnGuilding
deleted the
28-add-initial-forge-test-for-webauthn-verification
branch
This PR:
Webauthn.solSafeWebAuthnPlugin.solTestHelper.solcontract to provide helper functions and abstract common logicSafeWebAuthnPluginHarness.solto expose internal functions for testing. This is the recommended way to test internal functions from the foundry docs.This PR is dependent on #48Merged