Deploy latest modules to testnets #16
Conversation
- Deployed v2 RLV, CAM and IM to testnets. - Fxied HookMultiPlexer code after last merge as missing adjustment after merging of older branch.
- Added missing GasTankPaymaster deployments.
WalkthroughUpdated deployment documentation and broadcast artifacts with new contract addresses and transaction traces; added GasTankPaymasters table; adjusted script defaults for Pulse (whitelist, health checks); changed expected HookMultiPlexer address constant; and reversed the post-install iteration order in HookMultiPlexer implementation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Deployer
participant CAM as CredibleAccountModule
participant HMP as HookMultiPlexer
participant Hook as Hook
rect rgba(230,240,255,0.6)
note right of HMP: postInstall now iterates in reverse order
Deployer->>CAM: installHooks(hooks[])
CAM->>HMP: postInstall(hooks[])
loop reverse i = hooks.length-1 down to 0
HMP->>Hook: hooks[i].hook.postInstall(hooks[i].context)
Hook-->>HMP: result/event
end
HMP-->>CAM: postInstall complete
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
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: 1
🧹 Nitpick comments (1)
script/pulse/IM_WhitelistTokenManagement.s.sol (1)
7-9: Consider removing unused imports.The imports still include
USDC_GNOSISandUSDC_OPTIMISM, but these are no longer used in the default arrays (lines 16 and 20).Apply this diff to clean up unused imports:
-import { - EXPECTED_INVOICE_MANAGER_ADDRESS, USDC_GNOSIS, USDC_OPTIMISM, TEST_DAI_SEPOLIA -} from "./utils/PulseConstants.sol"; +import {EXPECTED_INVOICE_MANAGER_ADDRESS, TEST_DAI_SEPOLIA} from "./utils/PulseConstants.sol";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
DEPLOYMENTS.md(3 hunks)broadcast/IM_SolverOnboard.s.sol/11155111/run-1759747598.json(1 hunks)broadcast/IM_SolverOnboard.s.sol/11155111/run-latest.json(1 hunks)broadcast/IM_WhitelistTokenManagement.s.sol/11155111/run-1759749625.json(1 hunks)broadcast/IM_WhitelistTokenManagement.s.sol/11155111/run-latest.json(2 hunks)broadcast/PULSE_DeployContracts.s.sol/84532/run-1759744955.json(1 hunks)broadcast/PULSE_DeployContracts.s.sol/84532/run-1759745077.json(1 hunks)broadcast/PULSE_DeployContracts.s.sol/84532/run-1759747090.json(1 hunks)broadcast/PULSE_DeployContracts.s.sol/84532/run-latest.json(1 hunks)script/HookMultiPlexer.s.sol(1 hunks)script/pulse/IM_WhitelistTokenManagement.s.sol(1 hunks)script/pulse/PULSE_DeployContracts.s.sol(1 hunks)script/pulse/PULSE_HealthCheck.s.sol(1 hunks)src/modules/hooks/HookMultiPlexer.sol(2 hunks)
🔇 Additional comments (13)
src/modules/hooks/HookMultiPlexer.sol (2)
13-22: LGTM!The updated header properly attributes the original author (rhinestone.wtf) and clearly indicates that modifications were made by etherspot. This is appropriate for a modified/derived work under AGPL-3.0.
326-329: Verify reversed hook iteration order
In src/modules/hooks/HookMultiPlexer.sol (lines 326–329), the loop now processes hooks from last to first (reverse registration order). There are no tests covering postCheck ordering—confirm this reversal is intentional and doesn’t introduce ordering-dependent or shared-state issues.DEPLOYMENTS.md (2)
244-247: LGTM!The sepolia deployment addresses are properly updated with unique addresses for each contract, which is correct.
260-269: LGTM!The new GasTankPaymasters section is well-structured and provides clear documentation of the paymaster contracts and supported tokens across networks.
script/pulse/PULSE_HealthCheck.s.sol (2)
27-27: LGTM!The updated wallet address for health checks is properly formatted with correct checksum capitalization.
30-30: LGTM!The updated solver address for health checks is properly formatted with correct checksum capitalization. This appears to align with the "PULSE_SOLVER" onboarded in the broadcast traces.
script/HookMultiPlexer.s.sol (1)
24-24: EXPECTED_HMP matches documented deployments
The constant aligns with all HookMultiPlexer addresses listed in DEPLOYMENTS.md across networks.script/pulse/IM_WhitelistTokenManagement.s.sol (1)
16-20: LGTM! Testnet token defaults are appropriate.The updated defaults using
TEST_DAI_SEPOLIAfor adding tokens and an empty array for removing tokens align with testnet configuration needs.broadcast/PULSE_DeployContracts.s.sol/84532/run-latest.json (1)
1-116: Deployment trace looks correct.This broadcast file successfully documents two role grant transactions on Base Sepolia:
grantSessionKeyDisablerRoleto address 0xb63D98DC1e8095a45791549A8326504b26e4A21cgrantSettlerRoleto the same addressBoth transactions completed successfully with proper receipts and event logs.
broadcast/PULSE_DeployContracts.s.sol/84532/run-1759745077.json (1)
1-188: Deployment trace with pending transactions looks correct.This broadcast file documents the complete deployment sequence including CREATE2 deployments and configuration calls. The pending transaction hashes indicate successful submission to Base Sepolia.
broadcast/PULSE_DeployContracts.s.sol/84532/run-1759747090.json (1)
1-116: Execution trace with receipts looks correct.This broadcast file contains the complete execution details including receipts for the two role grant transactions. The event logs (line 72-89) confirm successful role assignments on Base Sepolia.
script/pulse/PULSE_DeployContracts.s.sol (1)
18-23: LGTM! Testnet token configuration is appropriate. Verified that TEST_USDC_BASE_SEPOLIA (0x5A3e81fBF26EE80eB1031fBc52663C1A57457B13) and TEST_USDT_BASE_SEPOLIA (0xDeDf3B40f8c44b1fF195F37F35c6b8199C7ee443) match the definitions in PulseConstants.sol.broadcast/PULSE_DeployContracts.s.sol/84532/run-1759744955.json (1)
119-121: Whitelist addresses verified Whitelisted token addresses matchTEST_USDC_BASE_SEPOLIAandTEST_USDT_BASE_SEPOLIAinPulseConstants.sol.
- Added correct module address for Pulse modules for Base Sepolia
- Added IM_UpdateFeeReceiver script for updating InvoiceManager Pulse fee receiver. - Added IM_UpdateSolverPulseFee script for updating a solver's Pulse fee.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
script/pulse/IM_UpdateSolverPulseFee.s.sol (2)
22-25: Verify InvoiceManager contract exists at the expected address.The script blindly casts to
InvoiceManagerwithout checking if a contract exists at that address. If the address is incorrect or the contract hasn't been deployed yet, subsequent calls will fail with confusing errors.Add an existence check after binding:
invoiceManager = InvoiceManager(EXPECTED_INVOICE_MANAGER_ADDRESS); + require(address(invoiceManager).code.length > 0, "InvoiceManager not deployed at expected address"); console2.log("Starting updating solver's pulse fee process..."); console2.log("InvoiceManager address:", address(invoiceManager));
17-20: Verify deployer has FEE_MANAGER_ROLE before attempting update.The comment assumes the deployer has
FEE_MANAGER_ROLE, but this isn't verified. If the assumption is incorrect, the transaction will fail on-chain after gas is spent. Consider adding a pre-flight check to fail early with a clear error message.Add a role check after validation:
// After line 34, add: bytes32 FEE_MANAGER_ROLE = keccak256("FEE_MANAGER_ROLE"); require( invoiceManager.hasRole(FEE_MANAGER_ROLE, vm.addr(deployerPrivateKey)), "Deployer does not have FEE_MANAGER_ROLE" ); console2.log("Deployer has FEE_MANAGER_ROLE - proceeding with update");Note: This requires
InvoiceManagerto expose ahasRolefunction (e.g., via AccessControl).script/pulse/IM_UpdateFeeReceiver.s.sol (2)
14-14: Consider reading fee receiver from environment variable.The fee receiver address is hardcoded despite the comment suggesting it should be updated before running. For better operational flexibility and to avoid accidental deployments with wrong addresses, consider reading this from an environment variable like other configuration values.
Apply this diff:
- // Configuration for updating pulse fee receiver - // Update these values before running the script - address public newFeeReceiver = 0x24D6c5aFBfe04E3231115D9f496DEe837D7F8233; + // Configuration for updating pulse fee receiver + address public newFeeReceiver;Then in the
run()function after line 21:newFeeReceiver = vm.envAddress("NEW_FEE_RECEIVER");
34-36: Incorrect comment section header.The comment says "Update Solver's Pulse Fee" but the operation being performed is updating the fee receiver address, not the pulse fee amount. This appears to be a copy-paste error from another script.
Apply this diff:
/*////////////////////////////////////////////////////////////// - Update Solver's Pulse Fee + Update Fee Receiver Address //////////////////////////////////////////////////////////////*/
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
broadcast/IM_UpdateFeeReceiver.s.sol/1/run-1761575498.json(1 hunks)broadcast/IM_UpdateFeeReceiver.s.sol/1/run-latest.json(1 hunks)broadcast/IM_UpdateFeeReceiver.s.sol/10/run-1761575588.json(1 hunks)broadcast/IM_UpdateFeeReceiver.s.sol/10/run-latest.json(1 hunks)broadcast/IM_UpdateFeeReceiver.s.sol/100/run-1761575540.json(1 hunks)broadcast/IM_UpdateFeeReceiver.s.sol/100/run-latest.json(1 hunks)broadcast/IM_UpdateFeeReceiver.s.sol/11155111/run-1761575701.json(1 hunks)broadcast/IM_UpdateFeeReceiver.s.sol/11155111/run-latest.json(1 hunks)broadcast/IM_UpdateFeeReceiver.s.sol/137/run-1761575617.json(1 hunks)broadcast/IM_UpdateFeeReceiver.s.sol/137/run-latest.json(1 hunks)broadcast/IM_UpdateFeeReceiver.s.sol/42161/run-1761575108.json(1 hunks)broadcast/IM_UpdateFeeReceiver.s.sol/42161/run-latest.json(1 hunks)broadcast/IM_UpdateFeeReceiver.s.sol/56/run-1761575449.json(1 hunks)broadcast/IM_UpdateFeeReceiver.s.sol/56/run-latest.json(1 hunks)broadcast/IM_UpdateFeeReceiver.s.sol/8453/run-1761575349.json(1 hunks)broadcast/IM_UpdateFeeReceiver.s.sol/8453/run-latest.json(1 hunks)broadcast/IM_UpdateFeeReceiver.s.sol/84532/run-1761575663.json(1 hunks)broadcast/IM_UpdateFeeReceiver.s.sol/84532/run-latest.json(1 hunks)broadcast/IM_UpdateSolverPulseFee.s.sol/1/run-1761575524.json(1 hunks)broadcast/IM_UpdateSolverPulseFee.s.sol/1/run-latest.json(1 hunks)broadcast/IM_UpdateSolverPulseFee.s.sol/10/run-1761575602.json(1 hunks)broadcast/IM_UpdateSolverPulseFee.s.sol/10/run-latest.json(1 hunks)broadcast/IM_UpdateSolverPulseFee.s.sol/100/run-1761575565.json(1 hunks)broadcast/IM_UpdateSolverPulseFee.s.sol/100/run-latest.json(1 hunks)broadcast/IM_UpdateSolverPulseFee.s.sol/11155111/run-1761575724.json(1 hunks)broadcast/IM_UpdateSolverPulseFee.s.sol/11155111/run-latest.json(1 hunks)broadcast/IM_UpdateSolverPulseFee.s.sol/137/run-1761575645.json(1 hunks)broadcast/IM_UpdateSolverPulseFee.s.sol/137/run-latest.json(1 hunks)broadcast/IM_UpdateSolverPulseFee.s.sol/42161/run-1761575249.json(1 hunks)broadcast/IM_UpdateSolverPulseFee.s.sol/42161/run-latest.json(1 hunks)broadcast/IM_UpdateSolverPulseFee.s.sol/56/run-1761575472.json(1 hunks)broadcast/IM_UpdateSolverPulseFee.s.sol/56/run-latest.json(1 hunks)broadcast/IM_UpdateSolverPulseFee.s.sol/8453/run-1761575375.json(1 hunks)broadcast/IM_UpdateSolverPulseFee.s.sol/8453/run-latest.json(1 hunks)broadcast/IM_UpdateSolverPulseFee.s.sol/84532/run-1761575679.json(1 hunks)broadcast/IM_UpdateSolverPulseFee.s.sol/84532/run-latest.json(1 hunks)script/pulse/IM_UpdateFeeReceiver.s.sol(1 hunks)script/pulse/IM_UpdateSolverPulseFee.s.sol(1 hunks)
✅ Files skipped from review due to trivial changes (16)
- broadcast/IM_UpdateFeeReceiver.s.sol/10/run-latest.json
- broadcast/IM_UpdateSolverPulseFee.s.sol/42161/run-1761575249.json
- broadcast/IM_UpdateFeeReceiver.s.sol/84532/run-1761575663.json
- broadcast/IM_UpdateFeeReceiver.s.sol/100/run-1761575540.json
- broadcast/IM_UpdateFeeReceiver.s.sol/1/run-1761575498.json
- broadcast/IM_UpdateSolverPulseFee.s.sol/1/run-latest.json
- broadcast/IM_UpdateFeeReceiver.s.sol/8453/run-1761575349.json
- broadcast/IM_UpdateFeeReceiver.s.sol/137/run-1761575617.json
- broadcast/IM_UpdateFeeReceiver.s.sol/8453/run-latest.json
- broadcast/IM_UpdateSolverPulseFee.s.sol/11155111/run-latest.json
- broadcast/IM_UpdateFeeReceiver.s.sol/42161/run-latest.json
- broadcast/IM_UpdateSolverPulseFee.s.sol/56/run-latest.json
- broadcast/IM_UpdateSolverPulseFee.s.sol/84532/run-1761575679.json
- broadcast/IM_UpdateSolverPulseFee.s.sol/84532/run-latest.json
- broadcast/IM_UpdateSolverPulseFee.s.sol/8453/run-latest.json
- broadcast/IM_UpdateSolverPulseFee.s.sol/56/run-1761575472.json
🔇 Additional comments (20)
broadcast/IM_UpdateSolverPulseFee.s.sol/100/run-latest.json (1)
1-65: Verify deployment chain alignment with PR objectives.The artifact shows a successful transaction on Gnosis Chain (chainId 100), but the PR description indicates deployments to "Base Sepolia and Sepolia." This appears to be either a deployment to an unmentioned chain or an artifact from prior work.
Confirm whether:
- This Gnosis Chain deployment is intended and within the PR scope
- The PR description accurately lists all deployment targets
- Other broadcast artifacts exist for the mentioned testnet chains
broadcast/IM_UpdateFeeReceiver.s.sol/56/run-1761575449.json (1)
1-65: Chain ID mismatch: verify this artifact belongs in this PR.The file shows a successful deployment to chain 56 (BSC mainnet), but the PR description specifies deployments to Base Sepolia and Sepolia testnets. Confirm whether:
- This BSC artifact should be included in a "Deploy to testnets" PR
- Broadcast artifacts for all target testnets (Base Sepolia chain 84532, Sepolia chain 11155111) are also present
- The broadcast artifacts are intentionally version-controlled (they're often .gitignored in projects using Forge)
If this is from a staging or production deployment rather than testnet, consider whether it should be in this PR or if there's a separate channel/process for those deployments.
broadcast/IM_UpdateSolverPulseFee.s.sol/42161/run-latest.json (1)
1-68: Broadcast artifact is well‑formed and transaction executed successfully.This is a Foundry broadcast artifact recording the successful execution of an
updateSolverFeetransaction on Arbitrum (chain 42161). The artifact structure is valid:
- Transaction and receipt hashes are consistent
- Chain ID and contract address align with deployment context
- Status code
0x1confirms successful execution- All required metadata (timestamp, commit hash) is present for reproducibility
No issues identified.
broadcast/IM_UpdateSolverPulseFee.s.sol/10/run-1761575602.json (1)
1-72: Broadcast artifact confirms successful solver fee update on chain 10 (Optimism).This auto-generated Foundry artifact documents a successful on-chain transaction (status 0x1) updating the solver fee via
updateSolverFee(address,uint256). The transaction metadata, receipt, and event logs appear properly structured and consistent with similar artifacts across other networks in this PR.broadcast/IM_UpdateSolverPulseFee.s.sol/100/run-1761575565.json (1)
1-65: Broadcast artifact confirms successful solver fee update on chain 100 (Gnosis).This auto-generated artifact documents successful transaction execution on chain 100. Metadata and receipt structure are consistent with other chain-specific artifacts in the PR.
broadcast/IM_UpdateSolverPulseFee.s.sol/10/run-latest.json (1)
1-72: Symlink artifact for latest execution on chain 10.The
run-latest.jsonfile matches the timestamped run artifact, confirming it's a record of the most recent script execution on Optimism.broadcast/IM_UpdateSolverPulseFee.s.sol/11155111/run-1761575724.json (1)
1-66: Broadcast artifact confirms successful solver fee update on chain 11155111 (Sepolia).This auto-generated artifact documents successful transaction execution on Sepolia testnet, aligning with the PR's stated testing on Sepolia.
broadcast/IM_UpdateSolverPulseFee.s.sol/137/run-latest.json (1)
1-81: Broadcast artifact confirms successful solver fee update on chain 137 (Polygon).This auto-generated artifact documents successful transaction execution on Polygon, including two event logs in the receipt (one from the target contract, one from the protocol fee mechanism).
broadcast/IM_UpdateSolverPulseFee.s.sol/137/run-1761575645.json (1)
1-81: Broadcast artifact mirrors run-latest for chain 137.This timestamped artifact matches the
run-latest.jsonrecord, confirming consistent tracking of the most recent Polygon execution.broadcast/IM_UpdateSolverPulseFee.s.sol/8453/run-1761575375.json (1)
1-72: Broadcast artifact confirms successful solver fee update on chain 8453 (Base).This auto-generated artifact documents successful transaction execution on Base, aligning with the PR's stated deployment to Base Sepolia. Receipt includes L1 fee metrics typical of L2 networks.
broadcast/IM_UpdateSolverPulseFee.s.sol/1/run-1761575524.json (1)
1-66: Broadcast artifact confirms successful solver fee update on chain 1 (Mainnet).This auto-generated artifact documents the mainnet execution. All broadcast artifacts across the 8 networks show consistent structure, successful execution (status 0x1), and identical transaction parameters, confirming the script executed correctly across all target networks.
broadcast/IM_UpdateFeeReceiver.s.sol/56/run-latest.json (1)
1-65: Broadcast artifact looks consistent.This is a generated artifact documenting a successful fee receiver update on BSC (chain 56). The transaction successfully called
setFeeReceiver(0x24D6c5aFBfe04E3231115D9f496DEe837D7F8233)on the InvoiceManager contract.broadcast/IM_UpdateFeeReceiver.s.sol/100/run-latest.json (1)
1-65: Broadcast artifact looks consistent.This is a generated artifact documenting a successful fee receiver update on Gnosis (chain 100). Transaction executed successfully.
broadcast/IM_UpdateFeeReceiver.s.sol/11155111/run-latest.json (1)
1-66: Broadcast artifact looks consistent.This is a generated artifact documenting a successful fee receiver update on Sepolia testnet (chain 11155111). Transaction executed successfully.
broadcast/IM_UpdateFeeReceiver.s.sol/137/run-latest.json (1)
1-81: Broadcast artifact looks consistent.This is a generated artifact documenting a successful fee receiver update on Polygon (chain 137). The additional log from address
0x1010is expected Polygon network behavior for gas fee tracking.broadcast/IM_UpdateFeeReceiver.s.sol/1/run-latest.json (1)
1-66: Broadcast artifact looks consistent.This is a generated artifact documenting a successful fee receiver update on Ethereum Mainnet (chain 1). Transaction executed successfully.
broadcast/IM_UpdateFeeReceiver.s.sol/42161/run-1761575108.json (1)
1-68: Broadcast artifact looks consistent.This is a generated artifact documenting a successful fee receiver update on Arbitrum One (chain 42161). The additional L1 gas tracking fields are expected behavior for Arbitrum transactions.
broadcast/IM_UpdateFeeReceiver.s.sol/84532/run-latest.json (1)
1-72: Broadcast artifact looks consistent.This is a generated artifact documenting a successful fee receiver update on Base Sepolia testnet (chain 84532). The L1 fee tracking fields are expected behavior for Base transactions.
broadcast/IM_UpdateFeeReceiver.s.sol/10/run-1761575588.json (1)
1-72: Broadcast artifact structure is valid and complete.The JSON artifact is well-formed with proper transaction and receipt fields, including Optimism-specific L1 fee data. Status 0x1 confirms successful execution. However, clarify the project's policy on committing broadcast artifacts to version control—these are typically generated build outputs. If intentional, add context to explain why these specific transactions are being permanently recorded (e.g., as verification of critical deployments).
broadcast/IM_UpdateFeeReceiver.s.sol/11155111/run-1761575701.json (1)
1-66: Cross-chain artifact consistency verified; testnet configuration correct.Both broadcast artifacts record identical fee receiver updates to the same contract across Optimism and Sepolia with matching transaction parameters and successful outcomes. The Sepolia artifact correctly omits L1 fee fields. However, the same caveat applies: confirm that permanently committing generated broadcast artifacts aligns with the project's deployment verification and CI/CD practices.
| try invoiceManager.setFeeReceiver(newFeeReceiver) { | ||
| console2.log("Successfully updated the pulse fee receiver address to:", newFeeReceiver); | ||
| } catch { | ||
| console2.log("Updating pulse fee receiver failed for:", newFeeReceiver); | ||
| } |
There was a problem hiding this comment.
Silent error handling masks deployment failures.
The try/catch logs failures but allows the script to continue and exit successfully even when the fee receiver update fails. This can mask operational issues and lead to confusion about whether the deployment succeeded. The script should revert on failure to ensure visibility of errors in CI/CD pipelines.
Apply this diff to revert on failure:
try invoiceManager.setFeeReceiver(newFeeReceiver) {
console2.log("Successfully updated the pulse fee receiver address to:", newFeeReceiver);
- } catch {
- console2.log("Updating pulse fee receiver failed for:", newFeeReceiver);
+ } catch Error(string memory reason) {
+ console2.log("Updating pulse fee receiver failed for:", newFeeReceiver);
+ revert(reason);
+ } catch (bytes memory lowLevelData) {
+ console2.log("Updating pulse fee receiver failed for:", newFeeReceiver);
+ revert("Low-level error during fee receiver update");
}📝 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.
| try invoiceManager.setFeeReceiver(newFeeReceiver) { | |
| console2.log("Successfully updated the pulse fee receiver address to:", newFeeReceiver); | |
| } catch { | |
| console2.log("Updating pulse fee receiver failed for:", newFeeReceiver); | |
| } | |
| try invoiceManager.setFeeReceiver(newFeeReceiver) { | |
| console2.log("Successfully updated the pulse fee receiver address to:", newFeeReceiver); | |
| } catch Error(string memory reason) { | |
| console2.log("Updating pulse fee receiver failed for:", newFeeReceiver); | |
| revert(reason); | |
| } catch (bytes memory lowLevelData) { | |
| console2.log("Updating pulse fee receiver failed for:", newFeeReceiver); | |
| revert("Low-level error during fee receiver update"); | |
| } |
🤖 Prompt for AI Agents
In script/pulse/IM_UpdateFeeReceiver.s.sol around lines 38-42, the try/catch
silently swallows failures and only logs them; change this so failures surface
to CI/CD by removing the try/catch and calling
invoiceManager.setFeeReceiver(newFeeReceiver) directly (so any revert/exception
bubbles up and causes the script to fail), keep the success console2.log after
the call; alternatively if you must catch, rethrow the error immediately inside
catch so the script exits non-zero (e.g., revert with a clear message) instead
of allowing continued execution.
| address public solverAddress = 0x7C84F10502FcDea2E403b70feA96a4aE990a34DF; // Update with actual solver address | ||
| uint256 public newPulseFee = 10; // 0 = use default 5 cents, >0 = custom fee amount in cents |
There was a problem hiding this comment.
Replace hardcoded configuration with environment variables or command-line arguments.
Hardcoded values in deployment scripts create operational risks—accidentally running the script without updating these values would update the wrong solver or apply an incorrect fee. The comment reminder is insufficient protection.
Consider using environment variables or Foundry's script arguments:
- address public solverAddress = 0x7C84F10502FcDea2E403b70feA96a4aE990a34DF; // Update with actual solver address
- uint256 public newPulseFee = 10; // 0 = use default 5 cents, >0 = custom fee amount in cents
+ address public solverAddress = vm.envAddress("SOLVER_ADDRESS");
+ uint256 public newPulseFee = vm.envUint("NEW_PULSE_FEE"); // 0 = use default 5 cents, >0 = custom fee amount in cents📝 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.
| address public solverAddress = 0x7C84F10502FcDea2E403b70feA96a4aE990a34DF; // Update with actual solver address | |
| uint256 public newPulseFee = 10; // 0 = use default 5 cents, >0 = custom fee amount in cents | |
| address public solverAddress = vm.envAddress("SOLVER_ADDRESS"); | |
| uint256 public newPulseFee = vm.envUint("NEW_PULSE_FEE"); // 0 = use default 5 cents, >0 = custom fee amount in cents |
🤖 Prompt for AI Agents
In script/pulse/IM_UpdateSolverPulseFee.s.sol around lines 14-15, the
solverAddress and newPulseFee are hardcoded which is unsafe; change them to read
from environment variables or script arguments (e.g., use Foundry's
vm.envAddress("SOLVER_ADDRESS") and vm.envUint("NEW_PULSE_FEE") or parse script
args) and validate inputs before using—if env/arg is missing or invalid,
revert/require with a clear message and optionally provide a default-not-allowed
behavior; add a short comment showing the expected env variable names and update
any README/usage docs accordingly.
| require(solverAddress != address(0), "Solver address must be set"); | ||
|
|
||
| console2.log("Solver address:", solverAddress); | ||
| console2.log("Pulse fee (0 = default):", newPulseFee); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add validation for pulse fee bounds.
While the solver address is validated, there's no check that newPulseFee is within a reasonable range. Given the comment states fees are in cents, consider adding an upper bound to prevent accidentally setting an extremely high fee (e.g., due to unit confusion or typos).
require(solverAddress != address(0), "Solver address must be set");
+ require(newPulseFee <= 1000, "Pulse fee exceeds reasonable maximum (1000 cents = $10)");🤖 Prompt for AI Agents
In script/pulse/IM_UpdateSolverPulseFee.s.sol around lines 31 to 34, there is no
validation that newPulseFee is within acceptable bounds; add a check to ensure
newPulseFee is non-negative and does not exceed a sensible upper limit (e.g.,
define a constant MAX_PULSE_FEE_CENTS near the top of the file or contract, such
as 1_000_000 (representing $10,000) or an agreed project limit) and use
require(newPulseFee <= MAX_PULSE_FEE_CENTS, "Pulse fee exceeds max allowed") so
attempts to set absurdly large fees revert; keep the existing console2 logs but
ensure the require runs before any state-changing operations or logs if desired.
| try invoiceManager.updateSolverFee(solverAddress, newPulseFee) { | ||
| console2.log("Successfully updated solver's pulse fee to:", newPulseFee); | ||
| } catch { | ||
| console2.log("Updating solver fee failed for:", solverAddress); | ||
| } |
There was a problem hiding this comment.
Revert on failure instead of silently catching errors.
The catch block logs the failure but allows the script to complete successfully, which means CI/CD pipelines and operators won't be notified that the update failed. This creates a false sense of security.
Apply this diff to fail loudly:
- try invoiceManager.updateSolverFee(solverAddress, newPulseFee) {
- console2.log("Successfully updated solver's pulse fee to:", newPulseFee);
- } catch {
- console2.log("Updating solver fee failed for:", solverAddress);
- }
+ try invoiceManager.updateSolverFee(solverAddress, newPulseFee) {
+ console2.log("Successfully updated solver's pulse fee to:", newPulseFee);
+ } catch Error(string memory reason) {
+ console2.log("Updating solver fee failed:", reason);
+ revert(reason);
+ } catch (bytes memory lowLevelData) {
+ console2.log("Updating solver fee failed with low-level error");
+ revert("updateSolverFee failed");
+ }🤖 Prompt for AI Agents
In script/pulse/IM_UpdateSolverPulseFee.s.sol around lines 40 to 44, the
try/catch silently logs failures and lets the script succeed; change the catch
to revert so the script fails loudly. Replace the generic catch with appropriate
catch clauses that surface the error: catch Error(string memory reason) {
revert(reason); } and catch (bytes memory lowLevelData) {
revert(string(lowLevelData)); } (or otherwise revert with a descriptive message
including solverAddress and newPulseFee) so CI/operators see the failure.
Description
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate)
Types of changes
Summary by CodeRabbit
Documentation
New Features
Refactor
Chores