pulse module functionality #70
Conversation
- Add Pulse class with installPulseModules() and addHook() methods - Define HookType enum and PulseConfig interface - Add HookMultiplexer ABI support - Integrate Pulse into ModularSdk via pulse getter - Update example to use new Pulse methods - Support automated Hook Multiplexer, Credible Account, and Resource Lock Validator installation - Updated network configs for Sepolia to use latest version of Bootstrap that installs the Hooks before Validators - Added new fields to NetworkConfig object to now include "credibleAccountModule" and "ResourceLockValidator"
WalkthroughThis change introduces a new Pulse module management feature to the modular SDK. It adds a Pulse class with methods for installing, uninstalling, and verifying Pulse ecosystem modules. Supporting utilities, interfaces, and constants are added, and a new example script demonstrates module installation. Minor formatting and a new Pulse getter are added to the SDK core. The Sepolia network configuration is updated with new contract addresses, and the package version is bumped. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExampleScript
participant ModularSdk
participant Pulse
participant Blockchain
User->>ExampleScript: Run install-latest-pulse-modules.ts
ExampleScript->>ModularSdk: Initialize with config
ExampleScript->>ModularSdk: Get counterfactual address & balance
ExampleScript->>Pulse: modularSdk.pulse.installPulseModules(config)
Pulse->>ModularSdk: Retrieve network config and addresses
Pulse->>Pulse: (Optional) Uninstall old HookMultiplexer
Pulse->>Pulse: Install HookMultiplexer, CredibleAccountModule, ResourceLockValidator
Pulse->>ModularSdk: Batch and send user operation
ModularSdk->>Blockchain: Submit user operation
Blockchain-->>ModularSdk: Return userOpHash
ExampleScript->>Blockchain: Poll for receipt
Blockchain-->>ExampleScript: Return receipt/status
ExampleScript->>Pulse: isPulseModulesInstalled(config)
Pulse->>ModularSdk: Check installed modules
Pulse-->>ExampleScript: Return install status
ExampleScript->>User: Log results and exit
Estimated code review effort4 (~90 minutes) Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
examples/pulse/install-latest-pulse-modules.tsOops! Something went wrong! :( ESLint: 8.28.0 ReferenceError: Cannot read config file: /.eslintrc.js 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Yooo! You forgot to bump the version in package.json! |
- CircleCI fix (explicit import path change)
|
Yooo! You forgot to bump the version in package.json! |
- Version bump
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/sdk/pulse/interfaces.ts (1)
3-9: Consider usingHextype for address fields for better type safetyThe address fields in
PulseConfigare typed asstring, but using theHextype from viem would provide better type safety and consistency with the rest of the codebase.export interface PulseConfig { - hookMultiplexerAddress?: string; - credibleAccountModuleAddress?: string; - resourceLockValidatorAddress?: string; + hookMultiplexerAddress?: Hex; + credibleAccountModuleAddress?: Hex; + resourceLockValidatorAddress?: Hex; uninstallOldHookMultiplexer?: boolean; - oldHookMultiplexerAddress?: string; + oldHookMultiplexerAddress?: Hex; }examples/pulse/install-latest-pulse-modules.ts (1)
45-47: Remove commented codeThe commented line should be removed if not needed, or uncommented if it's intended to be used.
uninstallOldHookMultiplexer: false, - // oldHookMultiplexerAddress: HOOK_MULTIPLEXER_ADDRESS,src/sdk/sdk.ts (1)
263-263: Use optional chaining for cleaner codeThe condition can be simplified using optional chaining.
- if (version && version.includes('skandha')) return this.bundler.getSkandhaGasPrice(); + if (version?.includes('skandha')) return this.bundler.getSkandhaGasPrice();src/sdk/pulse/pulse.ts (2)
132-132: Remove commented codeThe commented line should be removed if not needed.
const hmpInitData = getHookMultiPlexerInitData([credibleAccountModuleAddress]); - // const config = _makeBootstrapConfig(hookMultiplexerAddress, hmpInitData);
7-7: Unused importThe
_makeBootstrapConfigimport is not used and should be removed.-import { _makeBootstrapConfig } from '../base/Bootstrap.js';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
examples/pulse/install-latest-pulse-modules.ts(1 hunks)package.json(1 hunks)src/sdk/network/constants.ts(36 hunks)src/sdk/network/interfaces.ts(1 hunks)src/sdk/pulse/constants.ts(1 hunks)src/sdk/pulse/index.ts(1 hunks)src/sdk/pulse/interfaces.ts(1 hunks)src/sdk/pulse/pulse.ts(1 hunks)src/sdk/pulse/utils.ts(1 hunks)src/sdk/sdk.ts(13 hunks)
🧬 Code Graph Analysis (2)
src/sdk/pulse/utils.ts (1)
src/sdk/pulse/interfaces.ts (1)
SigHookInit(11-14)
src/sdk/sdk.ts (7)
src/sdk/pulse/pulse.ts (1)
Pulse(12-203)src/sdk/common/utils/viem-utils.ts (2)
getPublicClientByChain(29-35)getPublicClient(21-27)src/sdk/network/constants.ts (1)
Networks(91-649)src/sdk/interfaces.ts (1)
PaymasterApi(4-7)src/sdk/errorHandler/errorHandler.service.ts (1)
ErrorHandler(3-17)src/sdk/base/VerifyingPaymasterAPI.ts (1)
VerifyingPaymasterAPI(27-100)src/sdk/common/interfaces.ts (2)
UserOpsRequest(9-13)BatchUserOpsRequest(3-7)
🪛 Gitleaks (8.27.2)
src/sdk/network/constants.ts
118-118: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Biome (1.9.4)
src/sdk/sdk.ts
[error] 263-263: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/sdk/pulse/utils.ts (1)
src/sdk/pulse/interfaces.ts (1)
SigHookInit(11-14)
src/sdk/sdk.ts (7)
src/sdk/pulse/pulse.ts (1)
Pulse(12-203)src/sdk/common/utils/viem-utils.ts (2)
getPublicClientByChain(29-35)getPublicClient(21-27)src/sdk/network/constants.ts (1)
Networks(91-649)src/sdk/interfaces.ts (1)
PaymasterApi(4-7)src/sdk/errorHandler/errorHandler.service.ts (1)
ErrorHandler(3-17)src/sdk/base/VerifyingPaymasterAPI.ts (1)
VerifyingPaymasterAPI(27-100)src/sdk/common/interfaces.ts (2)
UserOpsRequest(9-13)BatchUserOpsRequest(3-7)
🪛 Gitleaks (8.27.2)
src/sdk/network/constants.ts
118-118: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 Biome (1.9.4)
src/sdk/sdk.ts
[error] 263-263: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (8)
package.json (1)
3-3: LGTM - Appropriate version bump for new feature.The version increment from "6.1.0" to "6.1.1" is appropriate for the addition of new Pulse module functionality.
src/sdk/network/interfaces.ts (1)
20-23: LGTM - Interface properly extended for Pulse modules.The addition of
credibleAccountModuleandresourceLockValidatorproperties to the contracts object correctly supports the new Pulse ecosystem functionality. The interface syntax is also properly formatted.src/sdk/pulse/constants.ts (1)
1-7: LGTM - Well-structured enum for hook types.The
HookTypeenum provides clear, type-safe constants with appropriate hexadecimal values for different hook categories. This will ensure consistent usage across the Pulse module.src/sdk/pulse/index.ts (1)
1-3: LGTM - Clean barrel export pattern.The index file properly centralizes all Pulse module exports, providing a clean public API surface for the new functionality.
src/sdk/network/constants.ts (3)
105-106: LGTM - Consistent addition of new contract fields across all networks.The new
credibleAccountModuleandresourceLockValidatorfields are properly added to all network configurations, maintaining consistency across the SDK.Also applies to: 120-121, 136-137, 151-152, 166-167, 181-182, 196-197, 211-212, 227-228, 242-243, 257-258, 272-273, 287-288, 302-303, 317-318, 332-333, 347-348, 362-363, 377-378, 392-393, 407-408, 422-423, 437-438, 452-453, 467-468, 482-483, 497-498, 512-513, 527-528, 542-543, 557-558, 572-573, 587-588, 602-603, 617-618, 645-646
116-116: LGTM - Bootstrap address update for Sepolia network.The bootstrap contract address update for Sepolia appears to be a legitimate upgrade to use the latest version that supports the new Pulse functionality.
118-118: Static analysis false positive - this is a valid contract address.The static analysis tool flagged this as a potential API key, but this is actually a valid Ethereum contract address for the
erc20SessionKeyValidatoron the Sepolia network. Contract addresses are public information and not sensitive credentials.src/sdk/sdk.ts (1)
52-52: Well-implemented lazy initialization for Pulse moduleThe lazy initialization pattern for the Pulse module is well-implemented, providing good encapsulation and on-demand instantiation.
Also applies to: 133-138
| } = config; | ||
|
|
||
| // Get network config | ||
| const chainId = this.modularSdk['chainId']; |
There was a problem hiding this comment.
Avoid accessing private properties of ModularSdk
The code accesses the private chainId property of ModularSdk in multiple places. This breaks encapsulation and could break if the internal implementation changes.
Consider adding a public getter method to ModularSdk to expose the chainId:
In ModularSdk:
getChainId(): number {
return this.chainId;
}Then update the Pulse class to use:
- const chainId = this.modularSdk['chainId'];
+ const chainId = this.modularSdk.getChainId();Also applies to: 91-91, 166-166
🤖 Prompt for AI Agents
In src/sdk/pulse/pulse.ts at lines 38, 91, and 166, the code accesses the
private property 'chainId' of ModularSdk directly, breaking encapsulation. To
fix this, add a public getter method getChainId() in the ModularSdk class that
returns this.chainId, then update the Pulse class to call this public method
instead of accessing the private property directly.
| throw new ErrorHandler('Hook Multiplexer address not found in network configuration'); | ||
| } | ||
|
|
||
| const address: Hex = (await this.modularSdk.getCounterFactualAddress()) as Hex; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused variable
The address variable is retrieved but never used in the addHook method.
- const address: Hex = (await this.modularSdk.getCounterFactualAddress()) as Hex;
-
// Clear existing UserOps from batch
await this.modularSdk.clearUserOpsFromBatch();📝 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.
| const address: Hex = (await this.modularSdk.getCounterFactualAddress()) as Hex; | |
| // src/sdk/pulse/pulse.ts | |
| // … existing code … | |
| // Clear existing UserOps from batch | |
| await this.modularSdk.clearUserOpsFromBatch(); | |
| // … following code … |
🤖 Prompt for AI Agents
In src/sdk/pulse/pulse.ts at line 179, the variable `address` is assigned but
never used in the addHook method. Remove the declaration and assignment of the
`address` variable to clean up the code and avoid unused variable warnings.
There was a problem hiding this comment.
agree with AI here
There was a problem hiding this comment.
@nikhilkumar1612 I'll help you remove that unused variable from the addHook method.
Task: Remove unused variable from addHook method
- Remove the unused
addressvariable declaration in theaddHookmethod - Clean up the code to avoid unused variable warnings
The change will be implemented by another agent. This removes the unused variable and cleans up the code as suggested.
⏳ Preparing changes...
There was a problem hiding this comment.
A summary of the changes CodeRabbit can apply:
- Remove the unused
addressvariable declaration from theaddHookmethod insrc/sdk/pulse/pulse.tsto clean up the code.
- ✅ Create PR with these edits
- 📌 Create commit in current branch
- 📋 Get copyable edits
| const encodedData = encodeAbiParameters(abiType, [ | ||
| globalHooks, | ||
| valueHooks, | ||
| delegatecallHooks, | ||
| sigHooks, | ||
| targetSigHooks, | ||
| ] as any); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using 'any' type casting
The as any type casting defeats TypeScript's type safety. Consider properly typing the parameters.
The type casting to any appears to be used to bypass TypeScript's type checking for the tuple array parameters. This should be addressed by properly typing the function parameters to match the expected ABI types.
🤖 Prompt for AI Agents
In src/sdk/pulse/utils.ts around lines 58 to 64, the use of 'as any' to cast the
array passed to encodeAbiParameters bypasses TypeScript's type safety. To fix
this, define the array with the correct tuple type that matches the expected ABI
parameter types instead of using 'any'. Adjust the types of globalHooks,
valueHooks, delegatecallHooks, sigHooks, and targetSigHooks or create a properly
typed tuple to pass to encodeAbiParameters, ensuring full type safety without
casting.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
3-14: Consider bumping the minor version instead of patch & fix naming inconsistencies– According to SemVer, introducing new public-facing functionality (
Pulseclass, SDK getter, new enum/interface, etc.) is a MINOR change, so6.2.0would be more appropriate than6.1.1.
– The bullet list mixes camelCase (resourceLockValidator) with Pascal Case (Resource Lock Validator) and inconsistent capitalisation ofcredibleAccountModule. Align the field names with the exact identifiers used in code to avoid confusion.Example diff:
-## [6.1.1] - 2025-07-22 +## [6.2.0] - 2025-07-22 ... -- Support for automated Hook Multiplexer, Credible Account, and Resource Lock Validator installation -- Added `credibleAccountModule` and `resourceLockValidator` fields to `NetworkConfig` object +- Support for automated Hook Multiplexer, *CredibleAccount* module, and *ResourceLockValidator* installation +- Added `credibleAccountModule` and `resourceLockValidator` fields to `NetworkConfig`
- Removed logs from pulse/constants.ts - Increased polling on example for getting UserOpReceipt to reduce unnecessary load
cryptonoyaiba
assigned
ch4r10t33r,
kanthgithub and
nikhilkumar1612 and unassigned
ch4r10t33r,
kanthgithub and
nikhilkumar1612
src/sdk/network/constants.ts
Outdated
| entryPoint: '0x0000000071727De22E5E9d8BAf0edAc6f37da032', | ||
| walletFactory: '0x38CC0EDdD3a944CA17981e0A19470d2298B8d43a', | ||
| bootstrap: '0xCF2808eA7d131d96E5C73Eb0eCD8Dc84D33905C7', | ||
| bootstrap: '0x6e5E238F8cA164CDe6a8C88407FB242b05524658', |
There was a problem hiding this comment.
@cryptonoyaiba i dont think we have to change the bootstrap address as it will result in a different counterfactual wallet address
There was a problem hiding this comment.
Sorry yes, you are right. I change this back and also I noticed im using an outdated contract addresses for modules. I'll deploy and update
- Updated Pulse contract module addresses on Sepolia to latest. - Reverted Bootstrap contract address on Sepolia back to original. - Adjusted latest Pulse module example script to use new addresses.
…spot/etherspot-modular-sdk into feature/hook-multiplexer-migration
- Fix for incorrect HookType assignment
| throw new ErrorHandler('Hook Multiplexer address not found in network configuration'); | ||
| } | ||
|
|
||
| const address: Hex = (await this.modularSdk.getCounterFactualAddress()) as Hex; |
There was a problem hiding this comment.
agree with AI here
- Updated PulseConfig to no longer use hookMultiPlexer addresses as taking from NetworkConfigs and made credibleAccountModule and resourceLockValidator addresses mandatory - Removed credibleAccountModule and resourceLockValidator fields from NetworkConfig as these will be passed in via intent-sdk - Updated pulse functions to match this. - Updated install-latest-pulse-modules example to match this.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/sdk/pulse/pulse.ts (4)
32-32: Avoid accessing private properties of ModularSdkThe code accesses the private
chainIdproperty of ModularSdk. This breaks encapsulation and could break if the internal implementation changes.
85-85: Avoid accessing private properties of ModularSdkThe code accesses the private
chainIdproperty of ModularSdk. This breaks encapsulation.
160-160: Avoid accessing private properties of ModularSdkThe code accesses the private
chainIdproperty of ModularSdk. This breaks encapsulation.
173-173: Remove unused variableThe
addressvariable is retrieved but never used in theaddHookmethod.
🧹 Nitpick comments (1)
src/sdk/pulse/pulse.ts (1)
126-126: Remove commented code or provide explanation.There's commented code referencing
_makeBootstrapConfig. Either remove it if it's no longer needed or add a comment explaining why it's kept for future reference.const hmpInitData = getHookMultiPlexerInitData([credibleAccountModuleAddress]); -// const config = _makeBootstrapConfig(hookMultiplexerAddress, hmpInitData);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
examples/pulse/install-latest-pulse-modules.ts(1 hunks)src/sdk/network/constants.ts(35 hunks)src/sdk/network/interfaces.ts(1 hunks)src/sdk/pulse/interfaces.ts(1 hunks)src/sdk/pulse/pulse.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/sdk/network/constants.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/sdk/network/interfaces.ts
- examples/pulse/install-latest-pulse-modules.ts
🔇 Additional comments (2)
src/sdk/pulse/interfaces.ts (1)
1-12: LGTM! Clean and well-structured interface definitions.The interfaces are properly typed and follow TypeScript best practices. The
PulseConfiginterface appropriately uses optional properties where needed, and the type imports are correct.src/sdk/pulse/pulse.ts (1)
149-149: No security issue with EOA address in ResourceLockValidator initialization
TheinstallResourceLockValidatormethod encodes the wallet’s EOA address into the module’s initData—this is a public identifier (not a secret key) and must be supplied so the validator knows which address is authorized to unlock resources. Exposing it in transaction calldata is expected and safe (all calldata is on-chain, public). This matches existing patterns elsewhere in the SDK.No changes required.
| async installPulseModules(config: PulseConfig): Promise<string> { | ||
| const { credibleAccountModuleAddress, resourceLockValidatorAddress, uninstallOldHookMultiplexer = false } = config; | ||
|
|
||
| // Get network config | ||
| const chainId = this.modularSdk['chainId']; | ||
| const networkConfig = Networks[chainId]; | ||
| if (!networkConfig) { | ||
| throw new ErrorHandler('Network configuration not found for chain ID: ' + chainId); | ||
| } | ||
|
|
||
| // Use provided addresses or fall back to network defaults | ||
| const HOOK_MULTIPLEXER_ADDRESS_V2 = networkConfig.contracts.hookMultiPlexerV2 as Hex; | ||
| const CREDIBLE_ACCOUNT_MODULE_ADDRESS = (credibleAccountModuleAddress || | ||
| networkConfig.contracts.credibleAccountModule) as Hex; | ||
| const RESOURCE_LOCK_VALIDATOR_ADDRESS = (resourceLockValidatorAddress || | ||
| networkConfig.contracts.resourceLockValidator) as Hex; | ||
| const HOOK_MULTIPLEXER_ADDRESS = networkConfig.contracts.hookMultiPlexer as Hex; | ||
|
|
||
| if (!HOOK_MULTIPLEXER_ADDRESS_V2 || !CREDIBLE_ACCOUNT_MODULE_ADDRESS || !RESOURCE_LOCK_VALIDATOR_ADDRESS) { | ||
| throw new ErrorHandler('Required contract addresses not found in network configuration'); | ||
| } | ||
|
|
||
| // Get wallet address | ||
| const address: Hex = (await this.modularSdk.getCounterFactualAddress()) as Hex; | ||
|
|
||
| // Clear existing UserOps from batch | ||
| await this.modularSdk.clearUserOpsFromBatch(); | ||
|
|
||
| // Uninstall old hook multiplexer if requested and installed | ||
| if (uninstallOldHookMultiplexer && HOOK_MULTIPLEXER_ADDRESS) { | ||
| await this.uninstallOldHookMultiplexer(address, HOOK_MULTIPLEXER_ADDRESS); | ||
| } | ||
|
|
||
| // Install Hook Multiplexer with Credible Account Module as subhook | ||
| await this.installHookMultiplexer(address, HOOK_MULTIPLEXER_ADDRESS_V2, CREDIBLE_ACCOUNT_MODULE_ADDRESS); | ||
|
|
||
| // Install Credible Account Module as Validator | ||
| await this.installCredibleAccountValidator(address, CREDIBLE_ACCOUNT_MODULE_ADDRESS); | ||
|
|
||
| // Install Resource Lock Validator | ||
| await this.installResourceLockValidator(address, RESOURCE_LOCK_VALIDATOR_ADDRESS); | ||
|
|
||
| // Estimate and send the UserOp | ||
| const op = await this.modularSdk.estimate(); | ||
| const uoHash = await this.modularSdk.send(op); | ||
|
|
||
| return uoHash; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding transaction confirmation and error handling.
The installPulseModules method performs multiple critical operations but lacks robust error handling and transaction confirmation. If any intermediate step fails, the user operation may be in an inconsistent state.
Consider wrapping the installation process in try-catch blocks and adding transaction confirmation:
async installPulseModules(config: PulseConfig): Promise<string> {
+ try {
const { credibleAccountModuleAddress, resourceLockValidatorAddress, uninstallOldHookMultiplexer = false } = config;
// ... existing code ...
// Estimate and send the UserOp
const op = await this.modularSdk.estimate();
const uoHash = await this.modularSdk.send(op);
+ // Wait for transaction confirmation
+ await this.waitForTransaction(uoHash);
return uoHash;
+ } catch (error) {
+ // Clear any pending operations on failure
+ await this.modularSdk.clearUserOpsFromBatch();
+ throw new ErrorHandler(`Failed to install Pulse modules: ${error.message}`);
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/sdk/pulse/pulse.ts between lines 28 and 75, the installPulseModules
method lacks error handling and transaction confirmation, risking inconsistent
states if steps fail. Wrap the entire installation process in a try-catch block
to catch and handle errors gracefully. After sending the user operation, add
logic to confirm the transaction completion before returning, ensuring each step
completes successfully or errors are properly managed.
| const addHookCalldata = encodeFunctionData({ | ||
| abi: HookMultiplexer, | ||
| functionName: 'addHook', | ||
| args: [hookAddress as Hex, hookType as Hex], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate hook address format before encoding.
The method casts hookAddress to Hex without validation. This could lead to runtime errors if an invalid address format is provided.
Add address validation before encoding:
+// Validate hook address format
+if (!hookAddress.match(/^0x[a-fA-F0-9]{40}$/)) {
+ throw new ErrorHandler('Invalid hook address format');
+}
+
// Encode the addHook function call
const addHookCalldata = encodeFunctionData({
abi: HookMultiplexer,
functionName: 'addHook',
args: [hookAddress as Hex, hookType as Hex],
});📝 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.
| args: [hookAddress as Hex, hookType as Hex], | |
| // Validate hook address format | |
| if (!hookAddress.match(/^0x[a-fA-F0-9]{40}$/)) { | |
| throw new ErrorHandler('Invalid hook address format'); | |
| } | |
| // Encode the addHook function call | |
| const addHookCalldata = encodeFunctionData({ | |
| abi: HookMultiplexer, | |
| functionName: 'addHook', | |
| args: [hookAddress as Hex, hookType as Hex], | |
| }); |
🤖 Prompt for AI Agents
In src/sdk/pulse/pulse.ts at line 182, the code casts hookAddress to Hex without
validating its format, which risks runtime errors. Add a validation step before
this line to check if hookAddress is a valid address format, and only proceed
with encoding if it passes validation. If invalid, handle the error
appropriately, such as throwing an error or returning early.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/sdk/pulse/pulse.ts (4)
78-101: Fix private property access in status check method.The method correctly checks module installation status but accesses the private
chainIdproperty directly, breaking encapsulation.async isPulseModulesInstalled(config: PulseConfig): Promise<{ hookMultiPlexer: boolean; credibleAccountValidator: boolean; resourceLockValidator: boolean; }> { - const chainId = this.modularSdk['chainId']; + const chainId = this.modularSdk.getChainId(); // Assumes public getter exists const networkConfig = Networks[chainId]; // ... rest of the method remains the same }
32-32: Fix encapsulation violation by accessing private chainId property.The code accesses the private
chainIdproperty of ModularSdk in multiple places, breaking encapsulation and potentially causing issues if the internal implementation changes.Verify if a public getter method exists for chainId in ModularSdk:
#!/bin/bash # Check if ModularSdk has a public chainId getter method ast-grep --pattern $'class ModularSdk { $$$ getChainId() { $$$ } $$$ }' # Also check for any public chainId property or method rg -A 3 "chainId.*:" --include="*.ts" | head -20Also applies to: 83-83, 156-156
28-73: Add comprehensive error handling and transaction confirmation.The
installPulseModulesmethod performs multiple critical operations but lacks robust error handling. If any intermediate step fails, the user operation may be in an inconsistent state. Additionally, there's still the issue of accessing privatechainIdproperty directly.Consider adding comprehensive error handling:
async installPulseModules(config: PulseConfig): Promise<string> { + try { const { credibleAccountModuleAddress, resourceLockValidatorAddress, uninstallOldHookMultiplexer = false } = config; // Get network config - const chainId = this.modularSdk['chainId']; + const chainId = this.modularSdk.getChainId(); // Assumes public getter exists const networkConfig = Networks[chainId]; if (!networkConfig) { throw new ErrorHandler('Network configuration not found for chain ID: ' + chainId); } // ... rest of the method ... // Estimate and send the UserOp const op = await this.modularSdk.estimate(); const uoHash = await this.modularSdk.send(op); + // Wait for transaction confirmation if needed + // await this.waitForTransaction(uoHash); return uoHash; + } catch (error) { + // Clear any pending operations on failure + await this.modularSdk.clearUserOpsFromBatch(); + throw new ErrorHandler(`Failed to install Pulse modules: ${error.message}`); + } }
154-192: Multiple issues need to be addressed in addHook method.This method has several issues that were identified in previous reviews: unused variable, private property access, and missing input validation.
async addHook(hookAddress: string, hookType: HookType, hookMultiplexerAddress?: string): Promise<string> { // Get network config - const chainId = this.modularSdk['chainId']; + const chainId = this.modularSdk.getChainId(); // Assumes public getter exists const networkConfig = Networks[chainId]; if (!networkConfig) { throw new ErrorHandler('Network configuration not found for chain ID: ' + chainId); } // Use provided address or fall back to network default const HOOK_MULTIPLEXER_ADDRESS = (hookMultiplexerAddress || networkConfig.contracts.hookMultiPlexer) as Hex; if (!HOOK_MULTIPLEXER_ADDRESS) { throw new ErrorHandler('Hook Multiplexer address not found in network configuration'); } + // Validate hook address format + if (!hookAddress.match(/^0x[a-fA-F0-9]{40}$/)) { + throw new ErrorHandler('Invalid hook address format'); + } - const address: Hex = (await this.modularSdk.getCounterFactualAddress()) as Hex; - // Clear existing UserOps from batch await this.modularSdk.clearUserOpsFromBatch(); // Encode the addHook function call const addHookCalldata = encodeFunctionData({ abi: HookMultiplexer, functionName: 'addHook', args: [hookAddress as Hex, hookType as Hex], }); // Add the call to the batch (calling addHook on the Hook Multiplexer) await this.modularSdk.addUserOpsToBatch({ to: HOOK_MULTIPLEXER_ADDRESS, data: addHookCalldata, }); // Estimate and send the UserOp const op = await this.modularSdk.estimate(); const uoHash = await this.modularSdk.send(op); return uoHash; }
🧹 Nitpick comments (1)
src/sdk/pulse/pulse.ts (1)
116-130: Remove commented code and improve readability.The method is functionally correct but contains commented-out code that should be removed for cleaner code.
private async installHookMultiplexer( address: Hex, hookMultiplexerAddress: Hex, credibleAccountModuleAddress: Hex, ): Promise<void> { const hmpInitData = getHookMultiPlexerInitData([credibleAccountModuleAddress]); - // const config = _makeBootstrapConfig(hookMultiplexerAddress, hmpInitData); const hmpInstallCalldata = encodeFunctionData({ abi: parseAbi(accountAbi), functionName: 'installModule', args: [BigInt(MODULE_TYPE.HOOK), hookMultiplexerAddress, hmpInitData], }); await this.modularSdk.addUserOpsToBatch({ to: address, data: hmpInstallCalldata }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/sdk/network/constants.ts(36 hunks)src/sdk/pulse/pulse.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sdk/network/constants.ts
🔇 Additional comments (5)
src/sdk/pulse/pulse.ts (5)
1-11: LGTM! Clean imports and dependencies.The import statements are well-organized, importing necessary modules from the SDK ecosystem including viem utilities, network configurations, and Pulse-specific interfaces.
12-17: LGTM! Simple and clean class structure.The Pulse class constructor properly initializes with a ModularSdk instance, following good encapsulation practices.
103-114: LGTM! Well-structured uninstall logic.The method properly checks if the old module is installed before attempting to uninstall it, preventing unnecessary operations. The ABI encoding and function call setup is correct.
132-142: LGTM! Clean validator installation implementation.The Credible Account Validator installation logic is well-structured with proper ABI encoding and parameter handling.
144-152: LGTM! Resource Lock Validator setup is correct.The method properly initializes the Resource Lock Validator with the EOA address, which is the expected configuration for this validator type.
Description
Types of changes
What types of changes does your code introduce?
Further comments (optional)
Summary by CodeRabbit
New Features
Enhancements
Chores
Style
Bug Fixes