Skip to content

replace usage of Node's Buffer with browser-friendly code#72

Merged
jeffsmale90 merged 2 commits into
MetaMask:mainfrom
gabspeck:replace-buffer
Oct 1, 2025
Merged

replace usage of Node's Buffer with browser-friendly code#72
jeffsmale90 merged 2 commits into
MetaMask:mainfrom
gabspeck:replace-buffer

Conversation

@gabspeck

@gabspeck gabspeck commented Sep 3, 2025

Copy link
Copy Markdown
Contributor

📝 Description

This PR makes the delegation-toolkit package compatible with environments where Node.js's Buffer global is not available.

🔄 What Changed?

List the specific changes made:

  • packages/delegation-toolkit/src/webAuthn.ts: parseAuthenticatorFlags changed to use Viem's hexToBytes in combination with Uint8Array

🚀 Why?

Explain the motivation behind these changes:

  • I'm developing a vanilla JS browser-side application that calls toMetaMaskSmartAccount and ran into an error because Buffer is not defined on the browser.

🧪 How to Test?

Describe how to test these changes:

  • Manual testing steps:
  1. Create a vanilla JS script that creates a MetaMask smart account instance with toMetamaskSmartAccount, using the hybrid implementation
  2. Use viem's "sendUserOperation" with that account
  3. After signing the operation with your passkey, you'll get an error similar to:
Uncaught (in promise) ReferenceError: Buffer is not defined
    parseAuthenticatorFlags webAuthn.ts:150
    encodeDeleGatorSignature webAuthn.ts:108
    encodeSignature signatory.ts:78
    promise callback*signTypedData signatory.ts:92
    signUserOperation2 toMetaMaskSmartAccount.ts:159
    sendUserOperation sendUserOperation.ts:138
    sendUserOperation bundler.ts:292
    createAccount index.js:57
    async* index.html:10
  • Automated tests added/updated
  • All existing tests pass

⚠️ Breaking Changes

List any breaking changes:

  • No breaking changes
  • Breaking changes (describe below):

📋 Checklist

Check off completed items:

  • Code follows the project's coding standards
  • Self-review completed
  • Documentation updated (if needed)
  • Tests added/updated
  • Changelog updated (if needed)
  • All CI checks pass

🔗 Related Issues

Link to related issues:
Closes #
Related to #

📚 Additional Notes

Any additional information, concerns, or context:


Note

Switches parseAuthenticatorFlags to viem hexToBytes/Uint8Array with a bounds check, and adds a unit test for missing flags.

  • Delegation Toolkit:
    • webAuthn.ts: Replace Node Buffer usage in parseAuthenticatorFlags with hexToBytes + Uint8Array; add error when flags byte is missing.
  • Tests:
    • webAuthn.test.ts: Add test asserting an error when authenticator flags are not found at the expected offset.

Written by Cursor Bugbot for commit 688f14c. This will update automatically on new commits. Configure here.

@gabspeck gabspeck requested a review from a team as a code owner September 3, 2025 16:42

@jeffsmale90 jeffsmale90 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gabspeck for this, and sorry for taking so long to get to it. We've been heads down on some other things. If I'm not mistaken, this is our first external contribution to the Delegation Toolkit 🎉

This all looks good - can you please just add a test for the "Authenticator flags not found in authenticator data"?

@gabspeck

Copy link
Copy Markdown
Contributor Author

Thanks @gabspeck for this, and sorry for taking so long to get to it. We've been heads down on some other things. If I'm not mistaken, this is our first external contribution to the Delegation Toolkit 🎉

This all looks good - can you please just add a test for the "Authenticator flags not found in authenticator data"?

Thanks for the review, test added

@gabspeck gabspeck requested a review from jeffsmale90 October 1, 2025 08:43

@jeffsmale90 jeffsmale90 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gabspeck! We will get this out in an update shortly.

@jeffsmale90 jeffsmale90 merged commit b861534 into MetaMask:main Oct 1, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants