feat/owner-account-param #195
Conversation
WalkthroughAdds support for passing a viem LocalAccount ( Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Provider
participant Config
Note over Caller,Provider: Request requiring owner account (delegatedEoa)
Caller->>Provider: getOwnerAccount()
Provider->>Config: read delegatedEoa config
alt viemLocalAccount provided
Provider->>Provider: return provided viemLocalAccount (cached)
Provider-->>Caller: LocalAccount (same object)
else privateKey provided
Provider->>Provider: create LocalAccount from privateKey
Provider-->>Caller: created LocalAccount
else neither or both provided
Provider-->>Caller: throw validation error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 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: 0
🧹 Nitpick comments (3)
README.md (1)
112-123: Consider adding an ownerAccount example.Thanks for calling out that either
privateKeyorownerAccountcan be supplied, but the sample still only shows the private key path. A short code block demonstratingownerAccount: someLocalAccount(and omittingprivateKey) would make the new workflow immediately copy‑pastable and prevent readers from assuming the flag is still mandatory.lib/interfaces/index.ts (1)
55-65: Tighten the delegatedEoa config type.Runtime now rejects configs that pass both
privateKeyandownerAccount, but the interface still allows it at compile time. We can align the type with the runtime guard by switching to a discriminated union, so consumers get an immediate TypeScript error if they try to provide both (or neither) fields.-export interface DelegatedEoaModeConfig { - chainId: number; - bundlerApiKey?: string; - bundlerUrl?: string; - bundlerApiKeyFormat?: string; - debugMode?: boolean; - walletMode: 'delegatedEoa'; - // Either privateKey or ownerAccount must be provided (but not both) - privateKey?: string; - ownerAccount?: LocalAccount; -} +type DelegatedEoaCredentials = + | { privateKey: string; ownerAccount?: never } + | { ownerAccount: LocalAccount; privateKey?: never }; + +export type DelegatedEoaModeConfig = { + chainId: number; + bundlerApiKey?: string; + bundlerUrl?: string; + bundlerApiKeyFormat?: string; + debugMode?: boolean; + walletMode: 'delegatedEoa'; +} & DelegatedEoaCredentials;__tests__/EtherspotProvider.test.ts (1)
1066-1083: Refactor: Test is misplaced or redundant.This test is located in the
getOwnerAccountdescribe block but actually testsupdateConfigvalidation. It's also redundant with the test at lines 463-480, which already covers the same scenario of throwing when both parameters are cleared.Consider either:
- Moving this test to the "updateConfig with delegatedEoa mode and ownerAccount" describe block (lines 359-481), or
- Removing it as redundant, or
- Refactoring it to actually test
getOwnerAccount's error handling (though that would require the provider to be in an invalid state, which the constructor/updateConfig prevent)Apply this diff to remove the redundant test:
- it('should throw when neither ownerAccount nor privateKey is available', () => { - const delegated = new EtherspotProvider({ - chainId: 1, - walletMode: 'delegatedEoa', - privateKey: - '0xaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', - } as EtherspotTransactionKitConfig); - - expect(() => { - delegated.updateConfig({ - walletMode: 'delegatedEoa', - privateKey: undefined, - ownerAccount: undefined, - }); - }).toThrow( - 'Either privateKey or ownerAccount is required when walletMode is "delegatedEoa"' - ); - }); -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)README.md(8 hunks)__tests__/EtherspotProvider.test.ts(13 hunks)lib/EtherspotProvider.ts(7 hunks)lib/interfaces/index.ts(3 hunks)lib/utils/index.ts(1 hunks)package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
__tests__/EtherspotProvider.test.ts (2)
lib/EtherspotProvider.ts (1)
EtherspotProvider(50-824)lib/interfaces/index.ts (1)
EtherspotTransactionKitConfig(68-70)
lib/EtherspotProvider.ts (2)
lib/interfaces/index.ts (2)
EtherspotTransactionKitConfig(68-70)DelegatedEoaModeConfig(55-65)lib/utils/index.ts (1)
log(70-75)
🔇 Additional comments (10)
__tests__/EtherspotProvider.test.ts (10)
8-8: LGTM: Import addition is appropriate.The
privateKeyToAccountimport from viem/accounts is correctly added to support the new ownerAccount test scenarios.
193-254: LGTM: Comprehensive constructor validation tests.The new tests thoroughly validate the mutual exclusivity requirement for
privateKeyandownerAccountin delegatedEoa mode:
- Correctly rejects when neither is provided
- Correctly rejects when both are provided
- Correctly accepts when only ownerAccount is provided
- Error messages are accurate and helpful
359-481: LGTM: Excellent coverage of updateConfig with ownerAccount.The new test suite comprehensively covers all updateConfig scenarios:
- Switching between privateKey and ownerAccount correctly clears the opposite field
- Correctly verifies object identity when ownerAccount is provided (line 400:
expect(owner).toBe(newOwnerAccount))- Correctly verifies address changes when switching to privateKey (lines 422-426)
- Validates all error conditions (neither, both, clearing both)
The tests properly distinguish between the two modes of operation and verify the expected behavior in each case.
1042-1054: LGTM: Correct verification of ownerAccount passthrough.This test properly verifies that when an
ownerAccountis provided,getOwnerAccount()returns the exact same object (not a copy). The use oftoBeon line 1053 is essential to confirm object identity.
1110-1134: LGTM: Excellent concurrent access test.This test correctly verifies that concurrent calls to
getOwnerAccount()with a providedownerAccountall return the exact same object. The use oftoBe(lines 1131-1133) is crucial to verify object identity rather than just equality.
1085-1108: LGTM: Correct test for privateKey concurrent access.The test correctly verifies that concurrent calls to
getOwnerAccount()withprivateKeyall return accounts with the same address (lines 1105-1107). The comment on line 1104 accurately describes the expected behavior.Note: This appropriately contrasts with lines 1110-1134, where the ownerAccount case verifies object identity using
toBe. With privateKey, each call creates a new LocalAccount object (viaprivateKeyToAccount), so only address equality is verified.
1176-1190: LGTM: Good integration test for ownerAccount support.This test correctly verifies that
getDelegatedEoaAccountworks properly when anownerAccountis provided directly. The verification at line 1189 (expect(owner).toBe(ownerAccount)) confirms that the same object is used throughout the flow.
1423-1439: LGTM: Cache clearing with ownerAccount is properly tested.This test correctly verifies that switching to delegatedEoa mode with an
ownerAccount(instead of privateKey) also clears caches appropriately. It mirrors the existing test at lines 1407-1421, providing symmetric coverage for both parameter types.
1472-1483: LGTM: Error message consistently updated.The test correctly verifies the updated error message that reflects the dual-parameter support (either privateKey or ownerAccount). The error message is consistent with other validation tests throughout the file.
888-897: LGTM: Test correctly updated for new validation.The test now explicitly provides a
privateKeywhen switching to delegatedEoa mode, as required by the new validation logic. This maintains the test's original intent while complying with the enhanced parameter requirements.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/EtherspotProvider.ts (1)
172-178: Minor: Comment and type assertion could be clearer.The comment on line 172 says "Store viemLocalAcocunt address as string for comparison (not in DelegatedEoaModeConfig type)" but line 178 includes a type assertion adding
ownerAccountAddressto the type. This is slightly confusing.Consider either:
- Documenting that we're extending the type for internal comparison purposes, or
- Creating a separate internal type for this comparison object
- // Store viemLocalAcocunt address as string for comparison (not in DelegatedEoaModeConfig type) + // Store viemLocalAccount address for comparison (extended type for internal use) viemLocalAcocuntAddress: delegatedEoaConfig.viemLocalAcocunt?.address, bundlerUrl: delegatedEoaConfig.bundlerUrl, bundlerApiKey: delegatedEoaConfig.bundlerApiKey, bundlerApiKeyFormat: delegatedEoaConfig.bundlerApiKeyFormat, walletMode: 'delegatedEoa', - } as DelegatedEoaModeConfig & { ownerAccountAddress?: string }; + } as DelegatedEoaModeConfig & { viemLocalAcocuntAddress?: string };Note: The property name in the type assertion should match the actual property name used on line 173.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)README.md(9 hunks)__tests__/EtherspotProvider.test.ts(13 hunks)lib/EtherspotProvider.ts(7 hunks)lib/interfaces/index.ts(3 hunks)lib/utils/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/utils/index.ts
- lib/interfaces/index.ts
🧰 Additional context used
🧬 Code graph analysis (2)
__tests__/EtherspotProvider.test.ts (2)
lib/EtherspotProvider.ts (1)
EtherspotProvider(50-825)lib/interfaces/index.ts (1)
EtherspotTransactionKitConfig(68-70)
lib/EtherspotProvider.ts (2)
lib/interfaces/index.ts (2)
EtherspotTransactionKitConfig(68-70)DelegatedEoaModeConfig(55-65)lib/utils/index.ts (1)
log(70-75)
🪛 LanguageTool
README.md
[grammar] ~449-~449: Ensure spelling is correct
Context: ... from viem) directly, but not both. The viemLocalAcocunt option is useful when you already have ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~612-~612: Ensure spelling is correct
Context: ... 7. Owner Account Usage: When using viemLocalAcocunt, ensure the underlying private key is h...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (5)
README.md (1)
1-670: Skip: Duplicate issue already flagged.The typo
viemLocalAcocunt→viemLocalAccountappears throughout this file and has already been flagged in the CHANGELOG.md review. Please apply the same fix to all occurrences in this file.__tests__/EtherspotProvider.test.ts (1)
1-1629: Skip: Duplicate issue already flagged.The typo
viemLocalAcocunt→viemLocalAccountappears throughout this test file. This has already been flagged in the CHANGELOG.md review. Please apply the same fix to all test names, variables, and assertions in this file.lib/EtherspotProvider.ts (3)
1-825: Skip: Duplicate issue already flagged.The typo
viemLocalAcocunt→viemLocalAccountappears throughout this implementation file. This has already been flagged in the CHANGELOG.md review. Please apply the same fix to all occurrences including variable names, parameter names, JSDoc comments, error messages, and log messages.
213-246: Approve the mutual exclusivity logic.The validation logic correctly ensures that:
- In delegatedEoa mode, at least one of
privateKeyorviemLocalAccountmust be provided- Both cannot be provided simultaneously
- The clearing logic properly handles switching between the two options
The implementation correctly accounts for the clearing behavior when calculating
willHavePrivateKeyandwillHaveViemLocalAccount, preventing invalid states.
568-596: Approve the getOwnerAccount implementation.The implementation correctly:
- Returns the provided
viemLocalAccountdirectly if available (avoiding unnecessary conversion)- Falls back to creating an account from
privateKeyif noviemLocalAccountis provided- Includes appropriate logging for both paths
- Throws a clear error if neither is available
This design is efficient and aligns with the PR objectives.
CHANGELOG.md
Outdated
|
|
||
| ### Added Changes | ||
|
|
||
| - **viemLocalAcocunt Support**: Added support for passing a `LocalAccount` (from viem) directly to the Transaction Kit via the `viemLocalAcocunt` parameter in `delegatedEoa` mode. This allows users to bypass `privateKeyToAccount` conversion when they already have a `LocalAccount` object. The `viemLocalAcocunt` parameter works equivalently to `privateKey` and provides the same functionality - users can provide either `privateKey` or `viemLocalAcocunt`, but not both. |
There was a problem hiding this comment.
CRITICAL: Fix typo in parameter name before release.
The parameter name viemLocalAcocunt contains a typo and should be viemLocalAccount (note the "cc" vs "cc" → should be "ccount"). This typo appears throughout the entire codebase including public API, documentation, tests, and error messages.
This is critical because:
- It's a public API parameter - fixing it after release would be a breaking change
- The correct spelling aligns with viem's
LocalAccounttype - It affects developer experience and code professionalism
Apply this fix across the entire codebase by replacing all instances of viemLocalAcocunt with viemLocalAccount:
-- **viemLocalAcocunt Support**: Added support for passing a `LocalAccount` (from viem) directly to the Transaction Kit via the `viemLocalAcocunt` parameter in `delegatedEoa` mode. This allows users to bypass `privateKeyToAccount` conversion when they already have a `LocalAccount` object. The `viemLocalAcocunt` parameter works equivalently to `privateKey` and provides the same functionality - users can provide either `privateKey` or `viemLocalAcocunt`, but not both.
+- **viemLocalAccount Support**: Added support for passing a `LocalAccount` (from viem) directly to the Transaction Kit via the `viemLocalAccount` parameter in `delegatedEoa` mode. This allows users to bypass `privateKeyToAccount` conversion when they already have a `LocalAccount` object. The `viemLocalAccount` parameter works equivalently to `privateKey` and provides the same functionality - users can provide either `privateKey` or `viemLocalAccount`, but not both.This needs to be fixed in:
- CHANGELOG.md
- README.md (multiple occurrences)
- lib/EtherspotProvider.ts (implementation)
- lib/interfaces/index.ts (type definitions)
- lib/utils/index.ts (SENSITIVE_KEYS)
- tests/EtherspotProvider.test.ts (all test cases)
📝 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.
| - **viemLocalAcocunt Support**: Added support for passing a `LocalAccount` (from viem) directly to the Transaction Kit via the `viemLocalAcocunt` parameter in `delegatedEoa` mode. This allows users to bypass `privateKeyToAccount` conversion when they already have a `LocalAccount` object. The `viemLocalAcocunt` parameter works equivalently to `privateKey` and provides the same functionality - users can provide either `privateKey` or `viemLocalAcocunt`, but not both. | |
| - **viemLocalAccount Support**: Added support for passing a `LocalAccount` (from viem) directly to the Transaction Kit via the `viemLocalAccount` parameter in `delegatedEoa` mode. This allows users to bypass `privateKeyToAccount` conversion when they already have a `LocalAccount` object. The `viemLocalAccount` parameter works equivalently to `privateKey` and provides the same functionality - users can provide either `privateKey` or `viemLocalAccount`, but not both. |
🤖 Prompt for AI Agents
In CHANGELOG.md around line 7, the public parameter name `viemLocalAcocunt` is
misspelled and must be corrected to `viemLocalAccount`; update this line and
then perform a project-wide rename of the identifier (code, docs, tests, types,
and SENSITIVE_KEYS) replacing all occurrences of `viemLocalAcocunt` with
`viemLocalAccount` (including README.md, lib/EtherspotProvider.ts,
lib/interfaces/index.ts, lib/utils/index.ts, and
__tests__/EtherspotProvider.test.ts), update any exported names/signatures and
error messages to the correct spelling, and run/update tests to reflect the new
name; if you need to avoid a breaking change immediately, add a temporary alias
or deprecation wrapper that maps the old misspelled name to the corrected one
and mark it for removal in the next major release.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (1)
449-458: Consider adding a viemLocalAccount example.The note explains the option, but every snippet still passes
privateKey. A tiny code sample showingviemLocalAccount: yourLocalAccountwould make the new flow discoverable without guesswork.lib/EtherspotProvider.ts (1)
172-183: Fix the type assertion for the viem local account address.We emit a
viemLocalAccountAddressproperty but assert the shape as{ ownerAccountAddress?: string }. That mismatch defeats the compiler’s protection—future code could readownerAccountAddressand always getundefined. Please align the asserted type (or rename the property) so the shape is truthful.Apply this diff to align the type:
- } as DelegatedEoaModeConfig & { ownerAccountAddress?: string }; + } as DelegatedEoaModeConfig & { viemLocalAccountAddress?: string };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)README.md(9 hunks)__tests__/EtherspotProvider.test.ts(14 hunks)lib/EtherspotProvider.ts(7 hunks)lib/interfaces/index.ts(3 hunks)lib/utils/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/interfaces/index.ts
🧰 Additional context used
🧬 Code graph analysis (2)
lib/EtherspotProvider.ts (2)
lib/interfaces/index.ts (2)
EtherspotTransactionKitConfig(68-70)DelegatedEoaModeConfig(55-65)lib/utils/index.ts (1)
log(70-75)
__tests__/EtherspotProvider.test.ts (2)
lib/EtherspotProvider.ts (1)
EtherspotProvider(50-829)lib/interfaces/index.ts (1)
EtherspotTransactionKitConfig(68-70)
🔇 Additional comments (2)
lib/utils/index.ts (1)
81-86: Sensitive key list update looks correct.Adding
viemLocalAccountensures LocalAccount objects are redacted alongside the other confidential fields.__tests__/EtherspotProvider.test.ts (1)
191-252: Great coverage for delegatedEoa credential validation.The new test cases pin the “neither” and “both” scenarios and prove
viemLocalAccountworks interchangeably withprivateKey, guarding the new config requirements.
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chore