Use assembly in WebAuthnVerifier for Base64 encoding #318

akshay-ap · 2024-03-11T14:35:09Z

This PR implements #289

The algorithm is implemented as a library instead of the suggested in-place adjustment from the original issue. Akshay's original implementation was doing it in-place (which can still be found in the commit history). While it was efficient code size-wise, but gas-wise, it consumed more gas than FCL's implementation, probably due to some inefficient stack manipulations. I tested it with Solady's library then, and it turned out to be much more efficient even though it wasn't doing the encoding in place.

The implementation I'm proposing is based on Solady's efficient algorithm. It seems to me that it's not adjustable to in-place encoding due to the fact the algorithm takes advantage of optimizing stack operations by writing X bytes to the scratch space and then writing them to the string at position Y+(I*N) (I=Iteration, Y=start, N=bytes written in an iteration) as a 32-byte word. Because it writes them as a 32-byte word, it’s not suitable for modifying a string in place because it will overwrite the existing string bytes with empty characters. (I'm happy if I'm wrong though)

Changes in PR:

Add codesize task
Add a Base64Url.sol library with a Base64Url encoding algorithm that is optimized for encoding bytes32 data.
Add tests for the base64 library
Edit the typescript configuration to include the hardhat.config.ts so all the type extensions are picked up. Necessary to get the project compiled.

Code size change

This PR:
WebAuthnSigner 2693 bytes (limit is 24576)

Default main branch:
WebAuthnSigner 2817 bytes (limit is 24576)

Gas Usage - Needs additional test for benchmarking

Solady:

  Gas Benchmarking
    WebAuthnSigner
      ⛽ deployment: 550077
      ✔ Benchmark signer deployment cost
      ⛽ verification (FreshCryptoLib): 216563
      ✔ Benchmark signer verification cost with FreshCryptoLib verifier (70ms)
      ⛽ verification (daimo-eth): 347246
      ✔ Benchmark signer verification cost with daimo-eth verifier (116ms)
      ⛽ verification (Dummy): 12713
      ✔ Benchmark signer verification cost with Dummy verifier

FCL:

  Gas Benchmarking
    WebAuthnSigner
      ⛽ deployment: 612929
      ✔ Benchmark signer deployment cost
      ⛽ verification (FreshCryptoLib): 217272
      ✔ Benchmark signer verification cost with FreshCryptoLib verifier (70ms)
      ⛽ verification (daimo-eth): 347636
      ✔ Benchmark signer verification cost with daimo-eth verifier (108ms)
      ⛽ verification (Dummy): 13844
      ✔ Benchmark signer verification cost with Dummy verifier

This PR:

  Gas Benchmarking
    WebAuthnSigner
      ⛽ deployment: 533456
      ✔ Benchmark signer deployment cost
      ⛽ verification (FreshCryptoLib): 210645
      ✔ Benchmark signer verification cost with FreshCryptoLib verifier (64ms)
      ⛽ verification (daimo-eth): 337941
      ✔ Benchmark signer verification cost with daimo-eth verifier (108ms)
      ⛽ verification (Dummy): 12281
      ✔ Benchmark signer verification cost with Dummy verifier

This PR (viaIR):

  Gas Benchmarking
    WebAuthnSigner
      ⛽ deployment: 544844
      ✔ Benchmark signer deployment cost
      ⛽ verification (FreshCryptoLib): 294440
      ✔ Benchmark signer verification cost with FreshCryptoLib verifier (108ms)
      ⛽ verification (daimo-eth): 335453
      ✔ Benchmark signer verification cost with daimo-eth verifier (117ms)
      ⛽ verification (Dummy): 10352
      ✔ Benchmark signer verification cost with Dummy verifier

coveralls · 2024-03-11T14:49:24Z

Pull Request Test Coverage Report for Build 8452699560

Details

2 of 2 (100.0%) changed or added relevant lines in 2 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.1%) to 81.633%

Totals
Change from base Build 8393417595:	0.1%
Covered Lines:	95
Relevant Lines:	103

💛 - Coveralls

modules/4337/contracts/experimental/verifiers/WebAuthnVerifier.sol

modules/passkey/contracts/verifiers/WebAuthnVerifier.sol

nlordell · 2024-03-13T14:23:08Z

The code in question is notable missing unit tests.

akshay-ap · 2024-03-13T15:20:26Z

Gas Usage - Needs additional test for benchmarking

These tests to be added in #287. So, need to close #287 before merging this PR if benchmarking results are required.

yes, I was thinking to add tests and benchmarking tests in #287 or else we can even do it in this

….sol

nlordell · 2024-03-13T16:45:06Z

I think we should add unit tests for the functions to ensure that we have 100% coverage (for example, check that it does not validate a correctly signed signature if the authentication flags aren't correctly set).

This should be different than general benchmarking.

akshay-ap · 2024-03-14T07:48:42Z

I think we should add unit tests for the functions to ensure that we have 100% coverage (for example, check that it does not validate a correctly signed signature if the authentication flags aren't correctly set).

This should be different than general benchmarking.

Already added here: https://github.com/safe-global/safe-modules/pull/301/files

nlordell · 2024-03-19T06:44:50Z

modules/4337/contracts/experimental/verifiers/WebAuthnVerifier.sol

@@ -3,7 +3,6 @@
 pragma solidity >=0.8.0;


The experimental code will be removed, so I wouldn't update it here as well.

nlordell · 2024-03-19T06:45:35Z

modules/4337/contracts/experimental/verifiers/WebAuthnVerifier.sol

            '{"type":"webauthn.get","challenge":"',
-            encodedChallenge,
+            "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA",
            '",',


nit: this can be a single string instead of mutiple strings (not sure how it affects the generated assembly though).

modules/4337/contracts/experimental/verifiers/WebAuthnVerifier.sol

modules/passkey/contracts/verifiers/WebAuthnVerifier.sol

mmv08 · 2024-03-26T16:35:30Z

For some reason, tests fail when run through the coverage command. When run normally, tests pass. I will investigate tomorrow.

nlordell · 2024-03-26T17:25:09Z

modules/passkey/src/types/solc.d.ts

+
+  export function compile(input: string): string
+
+  export function loadRemoteVersion(version: string, callback: (err: Error, solc: any) => void): void


modules/passkey/hardhat.config.ts

nlordell · 2024-03-26T17:28:38Z

modules/passkey/tsconfig.json

@@ -8,5 +8,6 @@
    "strict": true,
    "skipLibCheck": true,
    "resolveJsonModule": true
-  }
+  },
+  "include": ["src/**/*.ts", "hardhat.config.ts"]


Shouldn't tests be included as well?

Have included it now

nlordell · 2024-03-26T17:30:06Z

modules/passkey/src/types/solc.d.ts

@@ -0,0 +1,41 @@
+declare module 'solc' {
+  export interface CompilerInput {


Fairly sure these types aren't used anywhere - and I think are inaccurate (in particular CompilerOutput specifically depends on the outputSelection of the input). I would remove them if they aren't used.

They're used in the codesize task; without type declaration for solc, the project won't compile. They might be inaccurate since I generated them using the perplexity AI tool - however, I believe they're still safer than just declaring everything as any, like here. I'll take one more look and fix the inaccuracies

Ahh, now I see what you mean, you're right

akshay-ap · 2024-03-27T13:24:46Z

LGTM

modules/passkey/contracts/libraries/Base64Url.sol

Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>

akshay-ap self-assigned this Mar 11, 2024

akshay-ap requested a review from a team as a code owner March 11, 2024 14:35

akshay-ap requested review from nlordell, mmv08 and remedcu and removed request for a team March 11, 2024 14:35

akshay-ap marked this pull request as draft March 11, 2024 14:35

[#289] Use assembly in WebAuthnVerifier for Base64 encoding

118428f

akshay-ap force-pushed the feature-289-base64-encoding-optimization branch from eb111a4 to 118428f Compare March 11, 2024 14:46

nlordell reviewed Mar 11, 2024

View reviewed changes

modules/4337/contracts/experimental/verifiers/WebAuthnVerifier.sol Outdated Show resolved Hide resolved

[#289] Store Base64 encoded value without allocating string variable

f4c2e88

akshay-ap requested a review from nlordell March 12, 2024 18:56

akshay-ap marked this pull request as ready for review March 12, 2024 19:20

akshay-ap changed the title ~~WIP: Use assembly in WebAuthnVerifier for Base64 encoding~~ Use assembly in WebAuthnVerifier for Base64 encoding Mar 13, 2024

akshay-ap commented Mar 13, 2024

View reviewed changes

modules/4337/contracts/experimental/verifiers/WebAuthnVerifier.sol Outdated Show resolved Hide resolved

[#289] Update WebAuthnVerifier in passkey package

74ded1f

akshay-ap force-pushed the feature-289-base64-encoding-optimization branch from 530dcb2 to 74ded1f Compare March 13, 2024 10:25

akshay-ap commented Mar 13, 2024

View reviewed changes

modules/passkey/contracts/verifiers/WebAuthnVerifier.sol Outdated Show resolved Hide resolved

akshay-ap added 3 commits March 13, 2024 16:21

Update modules/4337/contracts/experimental/verifiers/WebAuthnVerifier…

e3faff8

….sol

Update modules/passkey/contracts/verifiers/WebAuthnVerifier.sol

70999e1

Fix lint issue

5ab7fac

nlordell reviewed Mar 19, 2024

View reviewed changes

modules/4337/contracts/experimental/verifiers/WebAuthnVerifier.sol Outdated Show resolved Hide resolved

nlordell reviewed Mar 19, 2024

View reviewed changes

modules/passkey/contracts/verifiers/WebAuthnVerifier.sol Outdated Show resolved Hide resolved

Add optimized Base64URL implementation for bytes32

a8928b7

mmv08 force-pushed the feature-289-base64-encoding-optimization branch 2 times, most recently from a52864e to d48bb04 Compare March 26, 2024 16:24

mmv08 marked this pull request as ready for review March 26, 2024 16:25

mmv08 requested a review from nlordell March 26, 2024 16:25

mmv08 force-pushed the feature-289-base64-encoding-optimization branch from d48bb04 to 2fbf4a3 Compare March 26, 2024 16:28

nlordell reviewed Mar 26, 2024

View reviewed changes

modules/passkey/hardhat.config.ts Outdated Show resolved Hide resolved

nlordell reviewed Mar 26, 2024

View reviewed changes

mmv08 force-pushed the feature-289-base64-encoding-optimization branch 3 times, most recently from ce81d03 to 0e87841 Compare March 27, 2024 10:48

mmv08 assigned mmv08 and unassigned akshay-ap Mar 27, 2024

mmv08 force-pushed the feature-289-base64-encoding-optimization branch 2 times, most recently from 94cd100 to 95c51a1 Compare March 27, 2024 11:02

Add codesize task

1952444

mmv08 force-pushed the feature-289-base64-encoding-optimization branch from 95c51a1 to 1952444 Compare March 27, 2024 11:10

mmv08 requested a review from nlordell March 27, 2024 12:21

nlordell reviewed Mar 27, 2024

View reviewed changes

modules/passkey/contracts/libraries/Base64Url.sol Outdated Show resolved Hide resolved

mmv08 and others added 2 commits March 27, 2024 14:28

Update modules/passkey/contracts/libraries/Base64Url.sol

063de78

Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>

Improve the comment on exiting the loop

e526a35

mmv08 requested a review from nlordell March 27, 2024 13:36

nlordell approved these changes Mar 27, 2024

View reviewed changes

mmv08 merged commit 60e609d into main Mar 27, 2024
9 checks passed

mmv08 deleted the feature-289-base64-encoding-optimization branch March 27, 2024 15:47

github-actions bot locked and limited conversation to collaborators Mar 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use assembly in WebAuthnVerifier for Base64 encoding #318

Use assembly in WebAuthnVerifier for Base64 encoding #318

akshay-ap commented Mar 11, 2024 •

edited by mmv08

Loading

coveralls commented Mar 11, 2024 •

edited

Loading

nlordell commented Mar 13, 2024

akshay-ap commented Mar 13, 2024

Gas Usage - Needs additional test for benchmarking

nlordell commented Mar 13, 2024

akshay-ap commented Mar 14, 2024

nlordell Mar 19, 2024

nlordell Mar 19, 2024

mmv08 commented Mar 26, 2024

nlordell Mar 26, 2024

nlordell Mar 26, 2024

mmv08 Mar 27, 2024

nlordell Mar 26, 2024

mmv08 Mar 27, 2024

mmv08 Mar 27, 2024

akshay-ap commented Mar 27, 2024


		export function compile(input: string): string

		export function loadRemoteVersion(version: string, callback: (err: Error, solc: any) => void): void

		@@ -0,0 +1,41 @@
		declare module 'solc' {
		export interface CompilerInput {

Use assembly in WebAuthnVerifier for Base64 encoding #318

Use assembly in WebAuthnVerifier for Base64 encoding #318

Conversation

akshay-ap commented Mar 11, 2024 • edited by mmv08 Loading

Code size change

Gas Usage - Needs additional test for benchmarking

coveralls commented Mar 11, 2024 • edited Loading

Pull Request Test Coverage Report for Build 8452699560

Details

💛 - Coveralls

nlordell commented Mar 13, 2024

akshay-ap commented Mar 13, 2024

Gas Usage - Needs additional test for benchmarking

nlordell commented Mar 13, 2024

akshay-ap commented Mar 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmv08 commented Mar 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshay-ap commented Mar 27, 2024

akshay-ap commented Mar 11, 2024 •

edited by mmv08

Loading

coveralls commented Mar 11, 2024 •

edited

Loading