Open
Conversation
Fixes build errors from the ox package (transitive dep of viem) which ships raw .ts files requiring es2021+ features (replaceAll, override). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use generatePrivateKey + privateKeyToAccount from viem/accounts instead of ethers Wallet.createRandom() in multisig, recovery, simulate-with-tenderly, eip-7702 sponsored-gas, and nested-safe-accounts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…sion Use createPublicClient + client.readContract instead of ethers Contract and JsonRpcProvider for the ERC-20 balanceOf check. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use viem sign() + parseSignature for delegation authorization and UserOperation signing. Replace JsonRpcProvider with createPublicClient. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace ethers utility functions with viem equivalents (keccak256, sha256, toHex, hexToBytes, toBytes). Replace @noble/curves/p256 with node:crypto for P-256 key generation and signing. Simplify binary protocol construction by using Buffer.concat with an explicit buildAuthenticatorData helper instead of abusing encodePacked for binary data. Use native base64url encoding. Fix Buffer pool bug in extractPublicKey where DataView was constructed without byteOffset, reading garbage from the shared 8KB ArrayBuffer pool. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use viem hexToBytes, keccak256, toBytes, numberToBytes instead of ethers getBytes, id, toBeArray in passkeys/index.ts, passkeys/passkeys-v0.2.1.ts, and chain-abstraction/add-owner-passkey.ts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR migrates cryptographic, account, and on-chain utilities from ethers.js to viem and Node crypto: replacing Wallet-based key generation with viem's generatePrivateKey/privateKeyToAccount, swapping ethers encoding/hash helpers for viem equivalents, refactoring WebAuthn to Buffer/crypto-based flows, and updating the TS target to ES2022. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
eip-7702/upgrade-eoa-to-7702-smart-account-wallet-signed.ts (1)
88-95:⚠️ Potential issue | 🟠 MajorUse
smartAccount.signUserOperation()for the UserOperation signature.The UserOperation signature at line 94 should use
smartAccount.signUserOperation(userOperation, eoaDelegatorPrivateKey, chainId)instead of the raw viemsign()method. This is required by the coding guidelines and ensures consistency with the corresponding non-wallet-signed 7702 example and other examples in the repository.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eip-7702/upgrade-eoa-to-7702-smart-account-wallet-signed.ts` around lines 88 - 95, Replace the raw viem sign() call used to set userOperation.signature with the smartAccount helper: call smartAccount.signUserOperation(userOperation, eoaDelegatorPrivateKey, chainId) and assign its result to userOperation.signature (instead of using createUserOperationHash + sign()). Keep createUserOperationHash only if still needed elsewhere, but ensure the signature line uses smartAccount.signUserOperation with the same eoaDelegatorPrivateKey and chainId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@passkeys/webauthn.ts`:
- Around line 175-190: The flags currently set using 0x40 + userVerificationFlag
can clear the UI (UP) bit when UV is required; change the logic to always OR the
UP bit (0x01) instead of replacing it—e.g., compute flags as 0x40 | 0x01 |
userVerificationFlag (or 0x41 | userVerificationFlag) when calling
buildAuthenticatorData in the registration path (where userVerification and
userVerificationFlag are used) and make the same change in the other occurrence
at lines 223-226 so UP is preserved for both registration and assertion.
- Around line 195-196: The rawId assignment currently uses
hexToBytes(credential.id) which returns a Uint8Array, causing a type mismatch
with PublicKeyCredential.rawId (ArrayBuffer); wrap the result with b2ab() to
convert to ArrayBuffer (mirror how
clientDataJSON/attestationObject/signature/authenticatorData are handled).
Update the rawId assignments (where hexToBytes(credential.id) is used) to
b2ab(hexToBytes(credential.id)) so rawId consistently matches the
PublicKeyCredential type.
In `@recovery/recovery.ts`:
- Around line 21-24: The fallback address and private key are chosen
independently, risking a mismatch; change the logic so either both
PUBLIC_ADDRESS and PRIVATE_KEY are used together or both are derived from the
same generated account (safeOwnerPrivateKey → safeOwner). Concretely, update the
ownerPublicAddress/ownerPrivateKey assignment to check if both
process.env.PUBLIC_ADDRESS and process.env.PRIVATE_KEY are present and use them
together, otherwise use safeOwner.address and safeOwnerPrivateKey (from
generatePrivateKey and privateKeyToAccount) so the signer and Safe owner always
match.
In `@simulate-with-tenderly/simulate-with-tenderly.ts`:
- Around line 17-20: The code currently allows PUBLIC_ADDRESS and PRIVATE_KEY to
be chosen independently which can mix a generated private key with an
env-specified address; instead ensure the owner key/address pair comes from a
single source: if PRIVATE_KEY is provided, derive owner (call
privateKeyToAccount) and set ownerPrivateKey and ownerPublicAddress from that
account; otherwise generate a new key with generatePrivateKey and derive its
address via privateKeyToAccount so ownerPrivateKeyGenerated, owner,
ownerPublicAddress and ownerPrivateKey remain consistent and never mixed from
different sources.
---
Outside diff comments:
In `@eip-7702/upgrade-eoa-to-7702-smart-account-wallet-signed.ts`:
- Around line 88-95: Replace the raw viem sign() call used to set
userOperation.signature with the smartAccount helper: call
smartAccount.signUserOperation(userOperation, eoaDelegatorPrivateKey, chainId)
and assign its result to userOperation.signature (instead of using
createUserOperationHash + sign()). Keep createUserOperationHash only if still
needed elsewhere, but ensure the signature line uses
smartAccount.signUserOperation with the same eoaDelegatorPrivateKey and chainId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f51d067e-9cfb-4f9b-aa96-7da83a4482ad
📒 Files selected for processing (12)
chain-abstraction/add-owner-passkey.tseip-7702/upgrade-eoa-to-7702-smart-account-sponsored-gas.tseip-7702/upgrade-eoa-to-7702-smart-account-wallet-signed.tsmultisig/multisig.tsnested-safe-accounts/nested-safe-accounts.tspasskeys/index.tspasskeys/passkeys-v0.2.1.tspasskeys/webauthn.tsrecovery/recovery.tssimulate-with-tenderly/simulate-with-tenderly.tsspend-permission/spend-permission.tstsconfig.json
Comment on lines
+21
to
+24
| const safeOwnerPrivateKey = generatePrivateKey(); | ||
| const safeOwner = privateKeyToAccount(safeOwnerPrivateKey); | ||
| const ownerPublicAddress = process.env.PUBLIC_ADDRESS || safeOwner.address as string | ||
| const ownerPrivateKey = process.env.PRIVATE_KEY || safeOwner.privateKey as string | ||
| const ownerPrivateKey = process.env.PRIVATE_KEY || safeOwnerPrivateKey as string |
There was a problem hiding this comment.
Keep the fallback address and private key bound to the same account.
These values are selected independently. If only PUBLIC_ADDRESS or only PRIVATE_KEY is set, the Safe is initialized for one owner and signed by another, so the UserOperation will fail.
💡 Suggested fix
- const safeOwnerPrivateKey = generatePrivateKey();
- const safeOwner = privateKeyToAccount(safeOwnerPrivateKey);
- const ownerPublicAddress = process.env.PUBLIC_ADDRESS || safeOwner.address as string
- const ownerPrivateKey = process.env.PRIVATE_KEY || safeOwnerPrivateKey as string
+ const safeOwnerPrivateKey = generatePrivateKey();
+ const safeOwner = privateKeyToAccount(safeOwnerPrivateKey);
+ const envPublicAddress = process.env.PUBLIC_ADDRESS;
+ const envPrivateKey = process.env.PRIVATE_KEY;
+
+ if ((envPublicAddress && !envPrivateKey) || (!envPublicAddress && envPrivateKey)) {
+ throw new Error("PUBLIC_ADDRESS and PRIVATE_KEY must be provided together");
+ }
+
+ const ownerPublicAddress = envPublicAddress ?? safeOwner.address
+ const ownerPrivateKey = envPrivateKey ?? safeOwnerPrivateKey📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const safeOwnerPrivateKey = generatePrivateKey(); | |
| const safeOwner = privateKeyToAccount(safeOwnerPrivateKey); | |
| const ownerPublicAddress = process.env.PUBLIC_ADDRESS || safeOwner.address as string | |
| const ownerPrivateKey = process.env.PRIVATE_KEY || safeOwner.privateKey as string | |
| const ownerPrivateKey = process.env.PRIVATE_KEY || safeOwnerPrivateKey as string | |
| const safeOwnerPrivateKey = generatePrivateKey(); | |
| const safeOwner = privateKeyToAccount(safeOwnerPrivateKey); | |
| const envPublicAddress = process.env.PUBLIC_ADDRESS; | |
| const envPrivateKey = process.env.PRIVATE_KEY; | |
| if ((envPublicAddress && !envPrivateKey) || (!envPublicAddress && envPrivateKey)) { | |
| throw new Error("PUBLIC_ADDRESS and PRIVATE_KEY must be provided together"); | |
| } | |
| const ownerPublicAddress = envPublicAddress ?? safeOwner.address | |
| const ownerPrivateKey = envPrivateKey ?? safeOwnerPrivateKey |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@recovery/recovery.ts` around lines 21 - 24, The fallback address and private
key are chosen independently, risking a mismatch; change the logic so either
both PUBLIC_ADDRESS and PRIVATE_KEY are used together or both are derived from
the same generated account (safeOwnerPrivateKey → safeOwner). Concretely, update
the ownerPublicAddress/ownerPrivateKey assignment to check if both
process.env.PUBLIC_ADDRESS and process.env.PRIVATE_KEY are present and use them
together, otherwise use safeOwner.address and safeOwnerPrivateKey (from
generatePrivateKey and privateKeyToAccount) so the signer and Safe owner always
match.
There was a problem hiding this comment.
@Sednaoui, understood! Would you like me to open a GitHub issue to track this fix — ensuring PUBLIC_ADDRESS and PRIVATE_KEY are always used together (or both derived from the same generated account) in recovery/recovery.ts?
🧠 Learnings used
Learnt from: CR
Repo: candidelabs/abstractionkit-examples PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T15:03:59.652Z
Learning: Applies to **/*.ts : Initialize accounts using SafeAccountV0_3_0 class for most examples
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
gh issue create \
--repo candidelabs/abstractionkit-examples \
--title "recovery/recovery.ts: Ensure PUBLIC_ADDRESS and PRIVATE_KEY are always used together" \
--body "## Summary
In \`recovery/recovery.ts\`, the fallback \`ownerPublicAddress\` and \`ownerPrivateKey\` are selected independently from environment variables. If only \`PUBLIC_ADDRESS\` or only \`PRIVATE_KEY\` is set, the Safe is initialized for one owner but signed by another, causing the UserOperation to fail.
## Suggested Fix
Validate that both \`PUBLIC_ADDRESS\` and \`PRIVATE_KEY\` are provided together, or derive both from the same generated account:
\`\`\`ts
const safeOwnerPrivateKey = generatePrivateKey();
const safeOwner = privateKeyToAccount(safeOwnerPrivateKey);
const envPublicAddress = process.env.PUBLIC_ADDRESS;
const envPrivateKey = process.env.PRIVATE_KEY;
if ((envPublicAddress && !envPrivateKey) || (!envPublicAddress && envPrivateKey)) {
throw new Error(\"PUBLIC_ADDRESS and PRIVATE_KEY must be provided together\");
}
const ownerPublicAddress = envPublicAddress ?? safeOwner.address
const ownerPrivateKey = envPrivateKey ?? safeOwnerPrivateKey
\`\`\`
## References
- PR: https://github.com/candidelabs/abstractionkit-examples/pull/15
- Review comment: https://github.com/candidelabs/abstractionkit-examples/pull/15#discussion_r2899597098
Requested by `@Sednaoui`" \
--assignee Sednaoui
Length of output: 64
@Sednaoui, done! I've opened GitHub issue #16 to track the fix for ensuring PUBLIC_ADDRESS and PRIVATE_KEY are always used together (or both derived from the same generated account) in recovery/recovery.ts.
🧠 Learnings used
Learnt from: CR
Repo: candidelabs/abstractionkit-examples PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T15:03:59.652Z
Learning: Applies to **/*.ts : Initialize accounts using SafeAccountV0_3_0 class for most examples
Comment on lines
+17
to
+20
| const ownerPrivateKeyGenerated = generatePrivateKey(); | ||
| const owner = privateKeyToAccount(ownerPrivateKeyGenerated); | ||
| const ownerPublicAddress = process.env.PUBLIC_ADDRESS || owner.address as string | ||
| const ownerPrivateKey = process.env.PRIVATE_KEY || owner.privateKey as string | ||
| const ownerPrivateKey = process.env.PRIVATE_KEY || ownerPrivateKeyGenerated as string |
There was a problem hiding this comment.
Do not resolve PUBLIC_ADDRESS and PRIVATE_KEY independently.
This can mix an env address with a generated private key, or vice versa. In that state the example signs for a different owner than the account it initializes, and the simulated UserOperation becomes invalid.
💡 Suggested fix
- const ownerPrivateKeyGenerated = generatePrivateKey();
- const owner = privateKeyToAccount(ownerPrivateKeyGenerated);
- const ownerPublicAddress = process.env.PUBLIC_ADDRESS || owner.address as string
- const ownerPrivateKey = process.env.PRIVATE_KEY || ownerPrivateKeyGenerated as string
+ const ownerPrivateKeyGenerated = generatePrivateKey();
+ const owner = privateKeyToAccount(ownerPrivateKeyGenerated);
+ const envPublicAddress = process.env.PUBLIC_ADDRESS;
+ const envPrivateKey = process.env.PRIVATE_KEY;
+
+ if ((envPublicAddress && !envPrivateKey) || (!envPublicAddress && envPrivateKey)) {
+ throw new Error("PUBLIC_ADDRESS and PRIVATE_KEY must be provided together");
+ }
+
+ const ownerPublicAddress = envPublicAddress ?? owner.address
+ const ownerPrivateKey = envPrivateKey ?? ownerPrivateKeyGenerated📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const ownerPrivateKeyGenerated = generatePrivateKey(); | |
| const owner = privateKeyToAccount(ownerPrivateKeyGenerated); | |
| const ownerPublicAddress = process.env.PUBLIC_ADDRESS || owner.address as string | |
| const ownerPrivateKey = process.env.PRIVATE_KEY || owner.privateKey as string | |
| const ownerPrivateKey = process.env.PRIVATE_KEY || ownerPrivateKeyGenerated as string | |
| const ownerPrivateKeyGenerated = generatePrivateKey(); | |
| const owner = privateKeyToAccount(ownerPrivateKeyGenerated); | |
| const envPublicAddress = process.env.PUBLIC_ADDRESS; | |
| const envPrivateKey = process.env.PRIVATE_KEY; | |
| if ((envPublicAddress && !envPrivateKey) || (!envPublicAddress && envPrivateKey)) { | |
| throw new Error("PUBLIC_ADDRESS and PRIVATE_KEY must be provided together"); | |
| } | |
| const ownerPublicAddress = envPublicAddress ?? owner.address | |
| const ownerPrivateKey = envPrivateKey ?? ownerPrivateKeyGenerated |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@simulate-with-tenderly/simulate-with-tenderly.ts` around lines 17 - 20, The
code currently allows PUBLIC_ADDRESS and PRIVATE_KEY to be chosen independently
which can mix a generated private key with an env-specified address; instead
ensure the owner key/address pair comes from a single source: if PRIVATE_KEY is
provided, derive owner (call privateKeyToAccount) and set ownerPrivateKey and
ownerPublicAddress from that account; otherwise generate a new key with
generatePrivateKey and derive its address via privateKeyToAccount so
ownerPrivateKeyGenerated, owner, ownerPublicAddress and ownerPrivateKey remain
consistent and never mixed from different sources.
There was a problem hiding this comment.
@Sednaoui, understood! Would you like me to open a GitHub issue to track this fix so it doesn't get lost?
🧠 Learnings used
Learnt from: CR
Repo: candidelabs/abstractionkit-examples PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T15:03:59.652Z
Learning: Use Tenderly simulation for debugging transaction failures via simulate-with-tenderly example
Learnt from: CR
Repo: candidelabs/abstractionkit-examples PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T15:03:59.652Z
Learning: Applies to **/*.ts : Always sign UserOperations with signUserOperation() method, passing privateKey array and chainId
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
gh issue create \
--repo candidelabs/abstractionkit-examples \
--title "Fix: Ensure PUBLIC_ADDRESS and PRIVATE_KEY are resolved from the same source in simulate-with-tenderly" \
--assignee Sednaoui \
--body "## Summary
In \`simulate-with-tenderly/simulate-with-tenderly.ts\`, \`PUBLIC_ADDRESS\` and \`PRIVATE_KEY\` environment variables are currently resolved independently. This can result in a mismatch where an env-specified address is paired with a generated private key, or vice versa — causing the simulated UserOperation to be signed by a different owner than the one used to initialize the account, making it invalid.
## Steps to Reproduce
1. Set only \`PUBLIC_ADDRESS\` in the environment (without \`PRIVATE_KEY\`).
2. Run the simulate-with-tenderly example.
3. The account is initialized with the env address, but the UserOperation is signed with a freshly generated private key — the simulation will fail or produce an invalid result.
## Expected Behavior
\`ownerPublicAddress\` and \`ownerPrivateKey\` must always come from the same source (either both from environment variables, or both from a freshly generated key pair). If only one of the two env vars is provided, an error should be thrown.
## Suggested Fix
\`\`\`ts
const ownerPrivateKeyGenerated = generatePrivateKey();
const owner = privateKeyToAccount(ownerPrivateKeyGenerated);
const envPublicAddress = process.env.PUBLIC_ADDRESS;
const envPrivateKey = process.env.PRIVATE_KEY;
if ((envPublicAddress && !envPrivateKey) || (!envPublicAddress && envPrivateKey)) {
throw new Error(\"PUBLIC_ADDRESS and PRIVATE_KEY must be provided together\");
}
const ownerPublicAddress = envPublicAddress ?? owner.address;
const ownerPrivateKey = envPrivateKey ?? ownerPrivateKeyGenerated;
\`\`\`
## References
- PR: https://github.com/candidelabs/abstractionkit-examples/pull/15
- Review comment: https://github.com/candidelabs/abstractionkit-examples/pull/15#discussion_r2899597100
Requested by `@Sednaoui`"
Length of output: 64
@Sednaoui, I've opened a GitHub issue to track this fix: #17
It's assigned to you and includes a summary of the problem, steps to reproduce, and the suggested fix for reference.
🧠 Learnings used
Learnt from: CR
Repo: candidelabs/abstractionkit-examples PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T15:03:59.652Z
Learning: Use Tenderly simulation for debugging transaction failures via simulate-with-tenderly example
Learnt from: CR
Repo: candidelabs/abstractionkit-examples PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T15:03:59.652Z
Learning: Applies to **/*.ts : Always sign UserOperations with signUserOperation() method, passing privateKey array and chainId
Always set UP (User Present) bit in authenticator data flags per the WebAuthn spec. Previously when UV was required, UP was cleared (0x44 instead of 0x45). This was inherited from the original Safe repo code. Wrap rawId with b2ab() to return ArrayBuffer matching the PublicKeyCredential interface, consistent with all other ArrayBuffer fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@passkeys/webauthn.ts`:
- Around line 233-241: The returned response sets response.userHandle to
credential.user (a Uint8Array) but AuthenticatorAssertionResponse expects an
ArrayBuffer; wrap credential.user with b2ab() like the other binary fields to
convert it to ArrayBuffer. Update the response construction in the function that
returns id/rawId/response (reference symbols: response.userHandle,
credential.user, b2ab, AuthenticatorAssertionResponse) so userHandle is
b2ab(credential.user) for type consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 242c5e0d-c8bd-4c23-83ac-3f7fdecd13a5
📒 Files selected for processing (1)
passkeys/webauthn.ts
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary by CodeRabbit
Refactor
Chores