fix/add-authorization-param-delegatesEoa-userOp - #194
Conversation
WalkthroughThe PR adds optional authorization parameter support to transaction methods (estimate, send, estimateBatches, sendBatches) in delegatedEoa mode, enabling atomic delegation-and-execution flows via EIP-7702 authorization. Includes Kernel v3.3 validation, per-chain batch handling, comprehensive documentation, tests, and UI examples. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🧹 Nitpick comments (10)
lib/TransactionKit.ts (2)
2518-2556: Consider moving Kernel validation after chainId check.The current code validates Kernel authorization for every chain group, even if the authorization won't be used for that chain (due to chainId mismatch). This could cause confusing error messages.
Consider validating Kernel authorization only when it will actually be used:
- // Validate authorization matches Kernel v3.3 implementation if provided - // Note: Authorization will only be used for chain groups where chainId matches - // (enforced by the conditional `authorization && !isDesignated` when calling bundler) - if (authorization) { + // Only use authorization if chain IDs match (for multi-chain batch compatibility) + const authChainId = authorization?.chainId; + const shouldUseAuthorization = + authorization && + !isDesignated && + authChainId !== undefined && + authChainId === groupChainId; + + // Validate authorization matches Kernel v3.3 implementation if it will be used + if (shouldUseAuthorization) { const isValidAuthorization = this.validateKernelAuthorization( authorization, groupChainId ); if (!isValidAuthorization) { const expectedKernelAddress = KernelVersionToAddressesMap[ KERNEL_V3_3 ].accountImplementationAddress as `0x${string}`; const errorMessage = `Invalid authorization: The provided authorization does not match Kernel v3.3 implementation. ` + `Only Kernel v3.3 (${expectedKernelAddress}) authorizations are supported. ` + `Please use delegateSmartAccountToEoa() to get a valid Kernel authorization.`; groupTxs.forEach((tx) => { const resultObj = { to: tx.to || '', value: tx.value?.toString(), data: tx.data, chainId: tx.chainId || groupChainId, errorMessage, errorType: 'VALIDATION_ERROR' as const, isEstimatedSuccessfully: false, }; groupEstimated.push(resultObj); estimatedTransactions.push(resultObj); }); chainGroups[groupChainId] = { transactions: groupEstimated, errorMessage, isEstimatedSuccessfully: false, }; batchAllGroupsSuccessful = false; continue; } } // Log if using authorization for delegation - if (authorization && !isDesignated) { + if (shouldUseAuthorization) { log( `estimateBatches(): Using provided authorization to delegate EOA during batch estimation (chain ${groupChainId})`, undefined, this.debugMode ); } // ... (gas estimation code) - // Only use authorization if chain IDs match (for multi-chain batch compatibility) - // Require authChainId to be defined and match groupChainId for security - const authChainId = authorization?.chainId; - const shouldUseAuthorization = - authorization && - !isDesignated && - authChainId !== undefined && - authChainId === groupChainId; const gasEstimate = await bundlerClient.estimateUserOperationGas({ account: delegatedEoaAccount, calls, ...(shouldUseAuthorization ? { authorization } : {}), });This reduces duplication and makes the logic clearer by checking shouldUseAuthorization once.
3356-3396: Consider moving Kernel validation after chainId check (same as estimateBatches).Same issue as in
estimateBatches- the Kernel validation runs for all chain groups even when the authorization won't be used for that chain due to chainId mismatch.Apply the same refactoring suggested for
estimateBatchesto avoid redundant validation and confusing error messages for multi-chain batches.README.md (1)
294-369: Good addition; add a brief security caveat and scope reminder.
- Add a one‑liner: “Treat authorization like a signature; do not log or persist it in client apps.”
- Consider amending the heading to “Using the authorization parameter (delegatedEoa mode only)” to reduce misuse.
example/src/App.tsx (3)
667-674: Stop usingas any; types now acceptauthorization.Interfaces include
authorization?: SignAuthorizationReturnType, so the casts are unnecessary.- const result = await named.estimate({ authorization: auth } as any); + const result = await named.estimate({ authorization: auth }); - const result = await named.send({ authorization: auth } as any); + const result = await named.send({ authorization: auth }); - const result = await kit.estimateBatches({ onlyBatchNames: ['sameAuthEstimate'], authorization: auth } as any); + const result = await kit.estimateBatches({ onlyBatchNames: ['sameAuthEstimate'], authorization: auth }); - const result = await kit.sendBatches({ onlyBatchNames: ['sameAuthSend'], authorization: auth } as any); + const result = await kit.sendBatches({ onlyBatchNames: ['sameAuthSend'], authorization: auth }); - const result = await named.estimate({ authorization: auth } as any); + const result = await named.estimate({ authorization: auth });Also applies to: 742-747, 812-816, 974-979, 1133-1141
395-404: Avoid logging full authorization material.Authorization contains signature parts (r/s/yParity). Log only non‑sensitive fields.
- logAndUpdateState( - `📝 Authorization: ${JSON.stringify(result.authorization, bigIntReplacer)}` - ); + const safeAuth = result.authorization + ? { address: result.authorization.address, chainId: result.authorization.chainId, nonce: result.authorization.nonce } + : undefined; + logAndUpdateState(`📝 Authorization (redacted): ${JSON.stringify(safeAuth)}`);And when logging after signing:
- `📝 Authorization signed: address=${auth.address}, chainId=${auth.chainId}, nonce=${auth.nonce}` + `📝 Authorization signed: address=${auth.address}, chainId=${auth.chainId}, nonce=${auth.nonce}` + // Do not log r/s/yParityAlso applies to: 473-481, 650-653
1111-1144: Clarify modular‑mode failure handling in UI button.This scenario label says “Should Fail”. Depending on core impl, it may either return a VALIDATION_ERROR result or throw. Wrap in try/catch and log both paths.
- const result = await named.estimate({ authorization: auth }); - logAndUpdateState(`✅ Modular estimate with auth (validation error expected): ${JSON.stringify(result, bigIntReplacer)}`); + try { + const result = await named.estimate({ authorization: auth }); + logAndUpdateState(`ℹ️ Modular estimate with auth: ${JSON.stringify(result, bigIntReplacer)}`); + } catch (err) { + logAndUpdateState(`✅ Expected failure: ${(err as Error).message}`); + }lib/interfaces/index.ts (1)
231-232: Type additions look correct; add a short note indicating delegatedEoa‑only.Consider JSDoc on these fields to state they are only supported in
delegatedEoamode and must match the transaction chainId and Kernel v3.3.- authorization?: SignAuthorizationReturnType; + /** delegatedEoa mode only; must match tx chainId and Kernel v3.3 */ + authorization?: SignAuthorizationReturnType;Also applies to: 237-238, 344-344, 349-350
__tests__/EtherspotTransactionKit.test.ts (3)
2425-2466: Mirror viem’s authorization shape in tests (optional).Your test fixtures include
vin addition tor/s/yParity. To better reflectSignAuthorizationReturnType, you can dropvand keepr/s/yParity. Not a blocker.Also applies to: 2651-2671, 2822-2834, 3014-3026
2913-3011: Add a sendBatches multi‑chain + authorization test for symmetry.You covered multi‑chain in estimateBatches; consider mirroring it for sendBatches to assert selective application of authorization per chain group.
2895-2911: Unify error semantics for modular-mode + authorization.Single-tx methods (estimate/send at lines 2561, 2732) return structured error result objects with
errorType: 'VALIDATION_ERROR', while batch methods (estimateBatches/sendBatches at lines 2894, 3101) throw errors viathrowError(). For API consistency and predictable developer experience, adopt one approach across all four methods (estimate, send, estimateBatches, sendBatches)—either consistently return structured errors or consistently throw.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
example/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)README.md(2 hunks)__tests__/EtherspotTransactionKit.test.ts(4 hunks)example/src/App.tsx(2 hunks)lib/TransactionKit.ts(28 hunks)lib/interfaces/index.ts(2 hunks)package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
example/src/App.tsx (1)
lib/interfaces/index.ts (1)
INamedTransaction(125-145)
lib/TransactionKit.ts (1)
lib/utils/index.ts (1)
log(70-75)
🔇 Additional comments (6)
lib/TransactionKit.ts (3)
290-351: LGTM: Solid authorization validation logic.The validation logic correctly:
- Enforces delegatedEoa mode only
- Validates the authorization address matches Kernel v3.3 implementation
- Uses case-insensitive comparison for addresses
- Provides detailed logging for debugging
- Returns false for missing or invalid authorization without throwing
1332-1334: LGTM: Correct conditional authorization usage.The conditional spread
...(authorization && !isDelegateSmartAccountToEoaDelegated ? { authorization } : {})correctly applies authorization only when:
- Authorization is provided
- EOA is not yet designated
This enables atomic delegation during estimation while avoiding redundant authorization for already-designated EOAs.
2577-2589: Excellent multi-chain authorization safety.The
shouldUseAuthorizationlogic provides strong defense-in-depth for multi-chain batches:const authChainId = authorization?.chainId; const shouldUseAuthorization = authorization && !isDesignated && authChainId !== undefined && authChainId === groupChainId;This prevents authorization leakage across chains by requiring:
- Authorization exists
- EOA not already designated
- Authorization has explicit chainId
- ChainId exactly matches current chain group
This is a solid security pattern that prevents accidental cross-chain authorization application.
CHANGELOG.md (1)
3-8: Changelog entry reads well and matches the intent.No issues from my side.
__tests__/EtherspotTransactionKit.test.ts (1)
2-5: Switch to real Kernel constants is a win.Removes brittle stubs and validates against real addresses. LGTM.
package.json (1)
4-4: Version bump verified; ready for release.package.json (2.1.1) and CHANGELOG.md (2.1.1 - 2025-01-27) are in sync. Create git tag v2.1.1 during the release workflow.
| if (authorization && transactionChainId) { | ||
| // Validate authorization chain ID matches transaction chain ID | ||
| const authChainId = authorization?.chainId; | ||
| if ( | ||
| authChainId !== undefined && | ||
| authChainId !== transactionChainId | ||
| ) { | ||
| return setErrorAndReturn( | ||
| `Invalid authorization: Authorization chain ID (${authChainId}) does not match transaction chain ID (${transactionChainId}). ` + | ||
| `Please use an authorization signed for chain ${transactionChainId}.`, | ||
| 'VALIDATION_ERROR', | ||
| {} | ||
| ); | ||
| } |
There was a problem hiding this comment.
Validate authorization chainId more strictly.
The current validation allows authChainId to be undefined, which means an authorization without a chainId could be accepted. This is potentially unsafe as the authorization might be intended for a different chain.
Consider requiring authChainId to be defined:
// Validate authorization chain ID matches transaction chain ID
const authChainId = authorization?.chainId;
if (
- authChainId !== undefined &&
- authChainId !== transactionChainId
+ authChainId === undefined
) {
return setErrorAndReturn(
+ `Invalid authorization: Authorization is missing chainId. ` +
+ `Authorization must include a chainId to ensure it's intended for chain ${transactionChainId}.`,
+ 'VALIDATION_ERROR',
+ {}
+ );
+ }
+ if (authChainId !== transactionChainId) {
+ return setErrorAndReturn(
`Invalid authorization: Authorization chain ID (${authChainId}) does not match transaction chain ID (${transactionChainId}). ` +
`Please use an authorization signed for chain ${transactionChainId}.`,
'VALIDATION_ERROR',
{}
);
}🤖 Prompt for AI Agents
In lib/TransactionKit.ts around lines 1271 to 1284, the authorization chain ID
check currently allows undefined authChainId which could accept authorizations
lacking a chainId; change the validation to require authChainId to be defined
and to equal the transactionChainId (i.e. treat undefined as invalid), and
update the error message to reflect both cases (missing chainId or mismatched
chainId) so the function returns a VALIDATION_ERROR when authChainId is
undefined or does not equal transactionChainId.
| // Validate authorization chain ID matches transaction chain ID | ||
| const authChainId = authorization?.chainId; | ||
| if ( | ||
| authChainId !== undefined && | ||
| authChainId !== transactionChainId | ||
| ) { | ||
| return setErrorAndReturn( | ||
| `Invalid authorization: Authorization chain ID (${authChainId}) does not match transaction chain ID (${transactionChainId}). ` + | ||
| `Please use an authorization signed for chain ${transactionChainId}.`, | ||
| 'VALIDATION_ERROR', | ||
| {} | ||
| ); | ||
| } |
There was a problem hiding this comment.
Validate authorization chainId more strictly (same issue as estimate).
Same issue as in the estimate method - the validation allows authChainId to be undefined, which could allow cross-chain authorization misuse.
Apply the same fix as suggested for the estimate method:
// Validate authorization chain ID matches transaction chain ID
const authChainId = authorization?.chainId;
if (
- authChainId !== undefined &&
- authChainId !== transactionChainId
+ authChainId === undefined
) {
return setErrorAndReturn(
+ `Invalid authorization: Authorization is missing chainId. ` +
+ `Authorization must include a chainId to ensure it's intended for chain ${transactionChainId}.`,
+ 'VALIDATION_ERROR',
+ {}
+ );
+ }
+ if (authChainId !== transactionChainId) {
+ return setErrorAndReturn(
`Invalid authorization: Authorization chain ID (${authChainId}) does not match transaction chain ID (${transactionChainId}). ` +
`Please use an authorization signed for chain ${transactionChainId}.`,
'VALIDATION_ERROR',
{}
);
}📝 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.
| // Validate authorization chain ID matches transaction chain ID | |
| const authChainId = authorization?.chainId; | |
| if ( | |
| authChainId !== undefined && | |
| authChainId !== transactionChainId | |
| ) { | |
| return setErrorAndReturn( | |
| `Invalid authorization: Authorization chain ID (${authChainId}) does not match transaction chain ID (${transactionChainId}). ` + | |
| `Please use an authorization signed for chain ${transactionChainId}.`, | |
| 'VALIDATION_ERROR', | |
| {} | |
| ); | |
| } | |
| // Validate authorization chain ID matches transaction chain ID | |
| const authChainId = authorization?.chainId; | |
| if ( | |
| authChainId === undefined | |
| ) { | |
| return setErrorAndReturn( | |
| `Invalid authorization: Authorization is missing chainId. ` + | |
| `Authorization must include a chainId to ensure it's intended for chain ${transactionChainId}.`, | |
| 'VALIDATION_ERROR', | |
| {} | |
| ); | |
| } | |
| if (authChainId !== transactionChainId) { | |
| return setErrorAndReturn( | |
| `Invalid authorization: Authorization chain ID (${authChainId}) does not match transaction chain ID (${transactionChainId}). ` + | |
| `Please use an authorization signed for chain ${transactionChainId}.`, | |
| 'VALIDATION_ERROR', | |
| {} | |
| ); | |
| } |
🤖 Prompt for AI Agents
In lib/TransactionKit.ts around lines 1808 to 1820, the current check permits
authChainId to be undefined which can allow cross-chain authorization misuse;
change the validation to require a present authorization.chainId and reject when
it's missing or does not exactly equal transactionChainId. Concretely, treat
undefined authChainId as an error (call setErrorAndReturn with a clear message
about missing authorization chain ID) and keep the mismatch branch for non-equal
values, mirroring the stricter check implemented in the estimate method.
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Documentation