feat: monolithic+modular openrouter#2
Merged
Conversation
…lithic' into openocean-across-modular-test
Optimized modular
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new V2 router implementation (signature-checked and unchecked) with byte-level splice semantics, a modular action engine, manipulators/Math helper, extensive E2E TypeScript tooling and scripts, Foundry PoOCs, and project scaffolding (.env.example, Hardhat, package, tsconfig, remappings, .gitignore). ChangesArbitrum-native E2E & npm config
Sequence Diagram(s)sequenceDiagram
actor Wallet
participant AH as AllowanceHolder
participant Router as BungeeOpenRouterV2Unchecked
participant OpenOcean as OpenOcean
participant Inbox as ArbitrumInbox
Wallet->>AH: approve(Router, MaxUint256)
Wallet->>AH: AH.exec(caller=Wallet, target=Router, calldata=performExecution/performModularExecution)
AH-->>Router: transferFrom(token,user,router,amount)
Router->>OpenOcean: swap call (AAVE→ETH)
OpenOcean-->>Router: returndata (estimated output)
Router->>Router: compute fee / splice bridge value
Router->>Inbox: depositEth(msg.value or spliced value)
Inbox-->>Router: success
Router-->>Wallet: emit/return results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (9)
src/manipulators/MathManipulator.sol (1)
16-18: ⚡ Quick winConsider validating the bps parameter.
The
percentfunction accepts anyuint256bps value, allowing callers to compute percentages > 100% (e.g.,bps = 50_000would compute 500%). While this might be intentional for some use cases, it's non-obvious behavior.This should either:
- Add a check
if (bps > BPS_DENOMINATOR) revert InvalidBps();, or- Document that bps values > 10_000 are supported for computing multiples
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/manipulators/MathManipulator.sol` around lines 16 - 18, The percent(uint256 amount, uint256 bps) function currently allows bps greater than BPS_DENOMINATOR which yields percentages >100%; add validation in percent to revert on invalid bps (e.g., if (bps > BPS_DENOMINATOR) revert InvalidBps();) or alternatively update the function comment to document that bps > BPS_DENOMINATOR is intentionally supported for multiples—locate the percent function and either insert the validation using the InvalidBps revert or add a clear NatSpec comment above percent referencing BPS_DENOMINATOR to explain supported ranges.src/manipulators/AcrossERC20AmountManipulator.sol (1)
30-32: 💤 Low valueInteger division truncates fractional amounts.
Line 32 divides
amountAfterFeeby10 ** decimalDifffor downscaling. This truncates any fractional part. For example, converting 1 wei from an 18-decimal token to a 6-decimal token results in 0.This is standard behavior for token amount conversions, but it means small amounts may round down to zero when crossing decimal boundaries. If this is the intended behavior, the code is correct. Otherwise, consider documenting this truncation behavior or adding a minimum output check.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/manipulators/AcrossERC20AmountManipulator.sol` around lines 30 - 32, The downscale division in the conversion (decimalDiff and amountAfterFee in AcrossERC20AmountManipulator) truncates fractions so small inputs can become zero; either document this truncation behavior or prevent accidental zero outputs by adding a guard or using rounding-up. Concretely, after computing decimalDiff (and checking MAX_SAFE_DECIMAL_DIFF and DecimalDiffTooLarge), either add a comment clarifying that return amountAfterFee / (10 ** decimalDiff) intentionally truncates, or enforce a minimum non-zero output (e.g., require(amountAfterFee >= 10 ** decimalDiff, "OutputUnderflow") or revert with a new SmallOutputZero error) or perform ceiling division (compute (amountAfterFee + 10**decimalDiff - 1) / (10**decimalDiff)) before returning.tsconfig.json (1)
10-11: 💤 Low valueRedundant file inclusion.
hardhat.config.tsis listed in bothinclude(line 10) andfiles(line 11). Sinceincludealready covers it, thefilesentry is redundant.This doesn't break anything but adds unnecessary duplication. Remove line 11 or clarify why explicit inclusion is needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tsconfig.json` around lines 10 - 11, The tsconfig currently lists "hardhat.config.ts" twice (once inside the "include" array and again in the "files" array); remove the redundant explicit entry by deleting "hardhat.config.ts" from the "files" array (or alternatively remove the "include" entry if you intentionally want explicit file listing), ensuring the change targets the tsconfig.json keys "include" and "files" and leaves other entries unchanged.test/poc/OpenOceanAcrossOpenRouterPoC.t.sol (2)
116-117: ⚡ Quick winHard-coded splice offsets are brittle.
The offsets
132and164in the deposit splices depend on the exact ABI encoding ofISpokePool.deposit. If the interface changes or calldata structure shifts, these offsets break silently.Document why these specific offsets map to
inputAmountandoutputAmountin the deposit calldata, or add constants with clear names:uint256 constant ACROSS_DEPOSIT_INPUT_AMOUNT_OFFSET = 132; uint256 constant ACROSS_DEPOSIT_OUTPUT_AMOUNT_OFFSET = 164;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/poc/OpenOceanAcrossOpenRouterPoC.t.sol` around lines 116 - 117, The depositSplices use hard-coded offsets (132, 164) which are brittle; replace them by named constants and/or document why those offsets correspond to ISpokePool.deposit's inputAmount and outputAmount to make the intent robust and maintainable. Define descriptive constants such as ACROSS_DEPOSIT_INPUT_AMOUNT_OFFSET and ACROSS_DEPOSIT_OUTPUT_AMOUNT_OFFSET and use those in the calls to _splice (i.e., where depositSplices[0] and depositSplices[1] are set), and add a short comment explaining how the calldata layout produces those offsets from ISpokePool.deposit's ABI so future edits won’t silently break.
145-161: 💤 Low valueHardcoded 3-hour deadline may be insufficient.
Line 157 sets
fillDeadlinetoblock.timestamp + 3 hours. This is fine for a PoC but may be too short if running against a fork with warped timestamps or during network congestion.Consider making the deadline configurable via environment variable for flexibility in different test scenarios.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/poc/OpenOceanAcrossOpenRouterPoC.t.sol` around lines 145 - 161, The hardcoded 3-hour extension used to set fillDeadline in _emptyAcrossDepositCalldata (the expression block.timestamp + 3 hours) should be made configurable; change _emptyAcrossDepositCalldata to accept a deadline extension parameter (or read a test env variable like ACROSS_DEADLINE_HOURS) and use that value to compute fillDeadline instead of the literal 3 hours, updating any callers/tests to pass the desired extension or ensure the env var is set; keep the selector and other encoded args unchanged and ensure the new parameter is converted to seconds before adding to block.timestamp.scripts/e2e/utils/routerAbi.ts (1)
1-21: ⚡ Quick winConsider using typechain-generated ABI.
The inline ABI definition is clear but duplicates the Solidity interface. If struct layouts change in the router contract, this ABI becomes stale.
Since
package.jsonincludes thetypechainscript, generated types would provide type safety and automatic sync with Solidity changes.However, for a minimal PoC setup, inline ABI is acceptable. Document that this must stay in sync with router contracts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/e2e/utils/routerAbi.ts` around lines 1 - 21, ROUTER_ABI is an inline, hand-written ABI for performExecution and performModularExecution that can drift from the Solidity router; replace it by importing the TypeChain-generated ABI/types (or the JSON artifact) produced by the project's typechain build and use those types for calls to performExecution/performModularExecution to keep struct layouts in sync, or if you must keep the inline fragment add a clear TODO comment above ROUTER_ABI stating it must be maintained alongside the Solidity router and point to the TypeChain artifact that should be used instead (reference symbols: ROUTER_ABI, performExecution, performModularExecution).scripts/e2e/utils/modularActionsBuilder/index.js (1)
56-59: 💤 Low valueRedundant storeResult marking.
Lines 56-59 set
storeResult = trueon splice sources during action construction. Then_markSpliceSources()(lines 129-135) does the same thing again when callingtoActions(),toLogicalActions(),toJSON(), ortoCalldata().The double-marking is defensive but unnecessary. Since line 58 already handles it during construction,
_markSpliceSources()only needs to handle the edge case where actions are built without splices initially, then splices are added via_splice()later (line 125).The current implementation is safe but slightly redundant. If this is intentional for robustness, keep it; otherwise consolidate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/e2e/utils/modularActionsBuilder/index.js` around lines 56 - 59, The code redundantly sets storeResult on splice sources both during construction (in the loop over action.splices where validateSpliceForAction is called and this._actions[splice.sourceActionIndex].storeResult = true) and again in _markSpliceSources(), so remove the duplicate by having the constructor-side marking be the primary path and change _markSpliceSources() to only mark splice sources when actions have had splices added later (i.e., detect actions with empty splices at construction or when _splice() is used) — keep validateSpliceForAction and the loop over action.splices as-is, and modify _markSpliceSources() to iterate only over actions that currently lack storeResult but now contain splices (or to be a no-op if all splices were handled at construction).scripts/deploy/deployBungeeOpenRouterV2.ts (1)
34-43: 💤 Low valueCommented-out code should be removed or tracked separately.
Multiple blocks of commented-out code for
BungeeOpenRouterV2deployment remain. If this is intended for future use, track it in a separate branch or issue. If it's obsolete, remove it to reduce noise.Also applies to: 60-60, 69-71
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/deploy/deployBungeeOpenRouterV2.ts` around lines 34 - 43, Remove the dead commented-out deployment code for BungeeOpenRouterV2 (the blocks that create V2Factory, deploy v2, await waitForDeployment, and log v2Address) to reduce noise; if you need to keep it for future reference, move the snippet into a dedicated branch or an issue/notes file instead of leaving it commented inline — ensure all similar commented blocks for BungeeOpenRouterV2 elsewhere in the file (the additional commented sections that reference V2Factory, v2, and v2Address) are handled the same way.scripts/e2e/utils/erc20.ts (1)
8-13: ⚡ Quick winConsolidate ERC-20 ABI into a single source of truth.
This should define one shared ABI constant and reuse it for both
ethers.Interfaceandethers.Contractto prevent drift between helpers.Proposed diff
-const ERC20_IFACE = new ethers.Interface([ +const ERC20_ABI = [ 'function approve(address spender, uint256 amount) external returns (bool)', 'function transfer(address recipient, uint256 amount) external returns (bool)', 'function balanceOf(address account) external view returns (uint256)', 'function allowance(address owner, address spender) external view returns (uint256)', -]); + 'function decimals() external view returns (uint8)', +] as const; + +const ERC20_IFACE = new ethers.Interface(ERC20_ABI); export function getErc20Contract(tokenAddress: string, providerOrSigner: ethers.Provider | ethers.Signer): ethers.Contract { return new ethers.Contract( tokenAddress, - [ - 'function approve(address spender, uint256 amount) external returns (bool)', - 'function allowance(address owner, address spender) external view returns (uint256)', - 'function balanceOf(address account) external view returns (uint256)', - 'function decimals() external view returns (uint8)', - ], + ERC20_ABI, providerOrSigner, ); }As per coding guidelines, this improves 6‑month maintainability and avoids over-engineered duplication.
Also applies to: 42-53
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/e2e/utils/erc20.ts` around lines 8 - 13, Create a single shared ABI constant (e.g., ERC20_ABI) and use it wherever the ERC-20 ABI is needed instead of constructing new Interface instances inline; replace the inline array currently passed to new ethers.Interface (symbol ERC20_IFACE) and any places creating new ethers.Contract with reuse of ERC20_ABI so both the Interface instance (ERC20_IFACE) and Contract constructors reference the same ABI constant to avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hardhat.config.ts`:
- Around line 37-41: The Hardhat network name `networks.arbitrum` does not match
the Etherscan API key `etherscan.apiKey.arbitrumOne`, so verification with `npx
hardhat verify --network arbitrum` will fail; fix by making the names
consistent: either rename the network key `arbitrum` -> `arbitrumOne` in the
networks object (so `networks.arbitrumOne`) or add an `arbitrum` entry to the
`etherscan.apiKey` object with the same value as `arbitrumOne` (so
`etherscan.apiKey.arbitrum = etherscan.apiKey.arbitrumOne`), ensuring
`networks.arbitrum` and the apiKey key share the same identifier used by the
Hardhat verify task.
In `@OPENROUTER.md`:
- Around line 260-272: The three flow examples (AllowanceHolder, OpenRouter,
AcrossRouter, SpokePool) are fragmented and unclear—expand each into a
self-contained flow description or point to tests: for each numbered flow,
describe the sequence of calls and data transformations (e.g., AllowanceHolder
-> OpenRouter(fee transfer) -> OpenRouter(modify input) -> SwapExecution ->
OpenRouter(modify input) -> SpokePool), define symbols like OpenRouter,
AcrossRouter(amount, AcrossBridgeData) (use a clearer signature such as
AcrossRouter.execute(amount, AcrossBridgeData) in prose), and either include
minimal pseudo-code snippets showing inputs/outputs at each step or add links to
example tests (e.g., test/poc/OpenOceanAcrossOpenRouterPoC.t.sol) demonstrating
the behavior; ensure the final line replaces vague directives like "should have
all the fee, swap, bridge code in this" with an explicit instruction of where
that logic lives (function/class names) and how the fee/swap/bridge
responsibilities are split.
In `@scripts/deploy/deployBungeeOpenRouterV2.ts`:
- Around line 22-26: The check for openRouterSigner is dead because
openRouterSigner is set from deployer.address; either remove the conditional and
the error throw, or (preferred) actually read the documented env var: set
openRouterSigner = process.env.OPEN_ROUTER_SIGNER_ADDRESS ?? deployer.address,
then validate that openRouterSigner exists and keep the existing throw (updating
the message if falling back to deployer.address case). Update references to
openRouterSigner, deployer.address, and the thrown Error message so they match
the chosen behavior.
In `@scripts/e2e/bridgeViaRelay.ts`:
- Line 256: The variable useModular is hardcoded to false causing mismatch with
the docstring; change its initialization to read the USE_MODULAR env var (e.g.,
const useModular = process.env.USE_MODULAR === 'true' or similar) so the script
respects USE_MODULAR=true, defaulting to false when the env var is absent or not
'true'; update any downstream uses of useModular (the constant declared as
useModular) accordingly so behavior follows the environment flag.
- Around line 98-103: The axios POST to Relay.link (the call using
axios.post<RelayQuoteResponse>(...)) has no request timeout; update the call in
bridgeViaRelay.ts to include a sensible timeout (e.g. a few seconds) so the
request cannot hang indefinitely—either add a timeout property to the existing
options object passed to axios.post (alongside headers) or use an axios instance
with a default timeout; ensure the timeout value is configurable or documented
and that any timeout errors are handled/propagated consistently by the
surrounding function (the code that awaits response.data).
- Around line 139-144: Add a bounds check and clear error handling before
accessing quote.steps[1] and its nested fields: verify quote.steps exists and
has length >= 2, that quote.steps[1].items exists and has at least one element,
and that depositItem.data/to are present before using them (these are the
variables depositStep, depositItem, depositTarget, depositData). If any of those
checks fail, throw or return a descriptive error message (e.g., "Relay quote
missing deposit step/items/data") instead of letting an uncaught exception
occur. Ensure the rest of the function uses the validated values.
In `@scripts/e2e/config.ts`:
- Around line 21-23: ROUTER_ADDRESS is hardcoded; change it to read from the
environment (process.env.ROUTER_ADDRESS) and fail fast if not provided: replace
the exported constant ROUTER_ADDRESS with logic that reads the env var,
validates it's present (non-empty) and throws an error (or calls
process.exit(1)) with a clear message when missing so e2e runs cannot proceed
against the wrong router; update any tests/uses expecting the old constant to
import the new exported value unchanged.
- Around line 93-97: FEE_BPS must be parsed and range-checked before converting
to BigInt to avoid runtime failures in bpsOf; change the FEE_BPS initialization
to parse process.env.FEE_AMOUNT_BPS with Number.parseInt (falling back to 10 if
undefined), validate that the parsed value is a finite integer in the range
0..10000, and if invalid throw a clear Error (or choose a safe default) so
invalid env input never reaches BigInt(bps); ensure bpsOf(amount: bigint, bps:
number) continues to accept a validated number and uses BigInt(bps) only after
this validation (refer to symbols FEE_BPS and bpsOf).
In `@scripts/e2e/swapBridgeViaArbitrumNative.ts`:
- Line 283: The hardcoded const useModular = true contradicts the docstring;
replace it by reading and parsing the USE_MODULAR environment variable so
behavior matches docs. Locate the useModular declaration and change it to derive
its value from process.env.USE_MODULAR (or equivalent env accessor), converting
"true"/"1" to boolean true and defaulting to false when unset/invalid; ensure
downstream code that uses useModular remains compatible.
- Line 149: The axios.get call that fetches the OpenOcean quote
(axios.get<OpenOceanSwapQuoteResponse>(url, { params })) lacks a timeout; add a
timeout option to the request options (for example { params, timeout:
AXIOS_TIMEOUT_MS }) and declare/reuse a numeric AXIOS_TIMEOUT_MS constant (e.g.,
10000) near the top of the module so the request will fail fast on network hangs
and remain consistent with the other swap implementations.
In `@scripts/e2e/swapBridgeViaCctp.ts`:
- Line 270: The constant useModular is hardcoded to false but should honor the
documented USE_MODULAR env var; replace the hardcoded assignment for the
top-level variable useModular with logic that reads process.env.USE_MODULAR and
treats common truthy values (e.g., "true", "1", case-insensitive) as true and
defaults to false otherwise so the runtime behavior matches the docstring;
update any related comments to reflect the env-driven behavior and keep the
symbol name useModular unchanged.
- Around line 98-100: The axios GET to the OpenOcean API (the call using
axios.get<OpenOceanSwapQuoteResponse>(url, { params })) can hang indefinitely;
add a timeout option to the request (e.g., pass { params, timeout: <ms> } or use
an axios instance with a default timeout) so the call fails fast on
slow/unresponsive endpoints, and handle the timeout error where
response.data.data (q) is used; update the axios invocation that constructs url
and calls axios.get to include the timeout value.
In `@scripts/e2e/swapBridgeViaStargateNative.ts`:
- Line 123: The axios GET call to fetch OpenOcean quotes (const response = await
axios.get<OpenOceanSwapQuoteResponse>(url, { params });) lacks a timeout; update
the request options to include a sensible timeout (e.g., timeout:
OPEN_OCEAN_TIMEOUT_MS or a numeric value like 5000) so the call becomes
axios.get<OpenOceanSwapQuoteResponse>(url, { params, timeout: <timeoutMs> }) and
use a configurable constant if available.
- Line 339: The file hardcodes const useModular = true which contradicts the
docstring; change it to read the environment variable (e.g. const useModular =
process.env.USE_MODULAR === 'true') so the script behavior follows the
documented USE_MODULAR env var, ensure useModular remains a boolean and update
any downstream conditional usage (functions or blocks referencing useModular) to
work with the new env-driven value.
In `@src/combined/BungeeOpenRouterV2.sol`:
- Around line 282-287: The native-token branch in the function handling token
transfers currently allows msg.value > amount (the block checking token ==
CurrencyLib.NATIVE_TOKEN_ADDRESS uses if (msg.value < amount)), which causes
excess ETH to remain in the contract; change that conditional to require exact
equality (use if (msg.value != amount) revert InsufficientMsgValue();) in the
token == CurrencyLib.NATIVE_TOKEN_ADDRESS branch so the call only succeeds when
msg.value exactly equals amount.
- Line 233: The calls to _performAction(exec.bridge.target, bridgeValue,
bridgeData) and _performAction(exec.swap.target, exec.swap.value,
exec.swap.data) reference a missing helper; implement an internal helper
mirroring the behavior of _doCall in BungeeOpenRouterV2Unchecked: add an
internal function _performAction(address target, uint256 value, bytes memory
data) that does a low-level call (target.call{value: value}(data)), checks the
success flag, and bubbles the revert reason on failure (either via require with
the returned revert message or using assembly to revert with returned data);
then use that helper in place of the current calls.
In `@src/combined/BungeeOpenRouterV2Unchecked.sol`:
- Around line 250-256: The current check for native token uses only msg.value <
amount which allows excess ETH to remain in the contract; update the logic in
the block handling CurrencyLib.NATIVE_TOKEN_ADDRESS to require exact payment
(use msg.value == amount) or implement an explicit refund path for msg.value >
amount; modify the condition that now reverts via InsufficientMsgValue() so it
instead reverts on msg.value != amount (or add refund and only revert on
msg.value < amount), referencing the variables token,
CurrencyLib.NATIVE_TOKEN_ADDRESS, msg.value, amount and the existing
InsufficientMsgValue() error to locate and update the code.
In `@test/poc/OpenOceanAcrossOpenRouterPoC.t.sol`:
- Around line 44-75: Add negative-case PoC tests alongside
test_openOceanSwapManipulatorAcrossDeposit_arbitrumFork: implement separate
functions e.g., test_openOceanSwap_zeroInput_arbitrumFork,
test_openOceanSwap_insufficientBalance_arbitrumFork,
test_openOceanSwap_swapFailure_arbitrumFork,
test_openOceanSwap_bridgeFailure_arbitrumFork that reuse Router,
AcrossERC20AmountManipulator, _buildActions and deal to set up conditions and
then call router.performModularExecution; for zero input set inputAmount = 0 and
assert performModularExecution reverts or returns an error, for insufficient
balance set deal to a smaller amount than input and expect revert, for swap
failure provide malformed/mismatched OPENOCEAN_SWAP_CALLDATA or manipulate
slippage params to force the swap step to fail and assert revert/result
indicates swap failure, and for bridge failure configure the spoke
pool/token/address to an invalid value before building actions and assert the
bridge step fails; in each test assert balances (ERC20 balanceOf on router and
ACROSS_ARBITRUM_SPOKE_POOL) and emitted failure semantics (revert or error
result) instead of success and reuse _assertPocResult-like checks adapted for
expected failures.
In `@test/poc/OpenOceanStargateNativeOpenRouterPoC.t.sol`:
- Around line 76-119: The current test
test_openOceanSwapStargateNativeBridge_arbitrumFork only covers the happy path;
add negative tests and edge-case tests and extract common initialization into
setUp() to avoid duplication. Create a setUp() that deploys/initializes Router,
MathManipulator, funds balances (used in
test_openOceanSwapStargateNativeBridge_arbitrumFork and new tests), then add
tests such as test_RevertWhen_SwapFails (simulate OpenOcean swap revert by
crafting failing _openOceanSwapCalldata or mocking response and assert
performModularExecution reverts), test_RevertWhen_InsufficientOutput (set fee >
swap output and assert revert), test_ZeroAmount (inputAmount = 0 and assert
expected revert or no-op), and test_MaxFee / test_FeeBoundaryConditions (fee =
max uint256 or boundary BPS values) using the helpers _buildActions,
_openOceanSwapCalldata, _stargateCalldata and verify results via
_assertPocResult or expectRevert as appropriate.
- Around line 76-89: The test
test_openOceanSwapStargateNativeBridge_arbitrumFork currently emits a log and
returns when ARBITRUM_RPC is missing, causing a silent pass; replace that
emit+return behavior in the conditional that checks bytes(rpcUrl).length == 0
with an explicit vm.skip("ARBITRUM_RPC not set") to mark the test skipped, or
instead enforce configuration by using a require(bytes(rpcUrl).length != 0,
"ARBITRUM_RPC not set") so the test fails when missing; update the conditional
in test_openOceanSwapStargateNativeBridge_arbitrumFork (where Router router =
_routerAtFixtureAddress() and MathManipulator manipulator = new
MathManipulator() are set) to use one of these two approaches.
---
Nitpick comments:
In `@scripts/deploy/deployBungeeOpenRouterV2.ts`:
- Around line 34-43: Remove the dead commented-out deployment code for
BungeeOpenRouterV2 (the blocks that create V2Factory, deploy v2, await
waitForDeployment, and log v2Address) to reduce noise; if you need to keep it
for future reference, move the snippet into a dedicated branch or an issue/notes
file instead of leaving it commented inline — ensure all similar commented
blocks for BungeeOpenRouterV2 elsewhere in the file (the additional commented
sections that reference V2Factory, v2, and v2Address) are handled the same way.
In `@scripts/e2e/utils/erc20.ts`:
- Around line 8-13: Create a single shared ABI constant (e.g., ERC20_ABI) and
use it wherever the ERC-20 ABI is needed instead of constructing new Interface
instances inline; replace the inline array currently passed to new
ethers.Interface (symbol ERC20_IFACE) and any places creating new
ethers.Contract with reuse of ERC20_ABI so both the Interface instance
(ERC20_IFACE) and Contract constructors reference the same ABI constant to avoid
drift.
In `@scripts/e2e/utils/modularActionsBuilder/index.js`:
- Around line 56-59: The code redundantly sets storeResult on splice sources
both during construction (in the loop over action.splices where
validateSpliceForAction is called and
this._actions[splice.sourceActionIndex].storeResult = true) and again in
_markSpliceSources(), so remove the duplicate by having the constructor-side
marking be the primary path and change _markSpliceSources() to only mark splice
sources when actions have had splices added later (i.e., detect actions with
empty splices at construction or when _splice() is used) — keep
validateSpliceForAction and the loop over action.splices as-is, and modify
_markSpliceSources() to iterate only over actions that currently lack
storeResult but now contain splices (or to be a no-op if all splices were
handled at construction).
In `@scripts/e2e/utils/routerAbi.ts`:
- Around line 1-21: ROUTER_ABI is an inline, hand-written ABI for
performExecution and performModularExecution that can drift from the Solidity
router; replace it by importing the TypeChain-generated ABI/types (or the JSON
artifact) produced by the project's typechain build and use those types for
calls to performExecution/performModularExecution to keep struct layouts in
sync, or if you must keep the inline fragment add a clear TODO comment above
ROUTER_ABI stating it must be maintained alongside the Solidity router and point
to the TypeChain artifact that should be used instead (reference symbols:
ROUTER_ABI, performExecution, performModularExecution).
In `@src/manipulators/AcrossERC20AmountManipulator.sol`:
- Around line 30-32: The downscale division in the conversion (decimalDiff and
amountAfterFee in AcrossERC20AmountManipulator) truncates fractions so small
inputs can become zero; either document this truncation behavior or prevent
accidental zero outputs by adding a guard or using rounding-up. Concretely,
after computing decimalDiff (and checking MAX_SAFE_DECIMAL_DIFF and
DecimalDiffTooLarge), either add a comment clarifying that return amountAfterFee
/ (10 ** decimalDiff) intentionally truncates, or enforce a minimum non-zero
output (e.g., require(amountAfterFee >= 10 ** decimalDiff, "OutputUnderflow") or
revert with a new SmallOutputZero error) or perform ceiling division (compute
(amountAfterFee + 10**decimalDiff - 1) / (10**decimalDiff)) before returning.
In `@src/manipulators/MathManipulator.sol`:
- Around line 16-18: The percent(uint256 amount, uint256 bps) function currently
allows bps greater than BPS_DENOMINATOR which yields percentages >100%; add
validation in percent to revert on invalid bps (e.g., if (bps > BPS_DENOMINATOR)
revert InvalidBps();) or alternatively update the function comment to document
that bps > BPS_DENOMINATOR is intentionally supported for multiples—locate the
percent function and either insert the validation using the InvalidBps revert or
add a clear NatSpec comment above percent referencing BPS_DENOMINATOR to explain
supported ranges.
In `@test/poc/OpenOceanAcrossOpenRouterPoC.t.sol`:
- Around line 116-117: The depositSplices use hard-coded offsets (132, 164)
which are brittle; replace them by named constants and/or document why those
offsets correspond to ISpokePool.deposit's inputAmount and outputAmount to make
the intent robust and maintainable. Define descriptive constants such as
ACROSS_DEPOSIT_INPUT_AMOUNT_OFFSET and ACROSS_DEPOSIT_OUTPUT_AMOUNT_OFFSET and
use those in the calls to _splice (i.e., where depositSplices[0] and
depositSplices[1] are set), and add a short comment explaining how the calldata
layout produces those offsets from ISpokePool.deposit's ABI so future edits
won’t silently break.
- Around line 145-161: The hardcoded 3-hour extension used to set fillDeadline
in _emptyAcrossDepositCalldata (the expression block.timestamp + 3 hours) should
be made configurable; change _emptyAcrossDepositCalldata to accept a deadline
extension parameter (or read a test env variable like ACROSS_DEADLINE_HOURS) and
use that value to compute fillDeadline instead of the literal 3 hours, updating
any callers/tests to pass the desired extension or ensure the env var is set;
keep the selector and other encoded args unchanged and ensure the new parameter
is converted to seconds before adding to block.timestamp.
In `@tsconfig.json`:
- Around line 10-11: The tsconfig currently lists "hardhat.config.ts" twice
(once inside the "include" array and again in the "files" array); remove the
redundant explicit entry by deleting "hardhat.config.ts" from the "files" array
(or alternatively remove the "include" entry if you intentionally want explicit
file listing), ensuring the change targets the tsconfig.json keys "include" and
"files" and leaves other entries unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 478d0ad1-2b32-4d7e-ad88-9ef7d2830e8a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (28)
.env.example.gitignoreOPENROUTER.mdhardhat.config.tspackage.jsonremappings.txtscripts/deploy/deployBungeeOpenRouterV2.tsscripts/e2e/bridgeViaRelay.tsscripts/e2e/config.tsscripts/e2e/swapBridgeViaArbitrumNative.tsscripts/e2e/swapBridgeViaCctp.tsscripts/e2e/swapBridgeViaStargateNative.tsscripts/e2e/utils/allowanceHolder.tsscripts/e2e/utils/contractTypes.tsscripts/e2e/utils/erc20.tsscripts/e2e/utils/modularActionsBuilder/README.mdscripts/e2e/utils/modularActionsBuilder/index.d.tsscripts/e2e/utils/modularActionsBuilder/index.jsscripts/e2e/utils/routerAbi.tssrc/combined/BungeeOpenRouterV2.solsrc/combined/BungeeOpenRouterV2Unchecked.solsrc/dummyRouter.solsrc/manipulators/AcrossERC20AmountManipulator.solsrc/manipulators/MathManipulator.solsrc/monolithic/BungeeOpenRouterAH.soltest/poc/OpenOceanAcrossOpenRouterPoC.t.soltest/poc/OpenOceanStargateNativeOpenRouterPoC.t.soltsconfig.json
💤 Files with no reviewable changes (1)
- src/dummyRouter.sol
Comment on lines
+98
to
+103
| const response = await axios.post<RelayQuoteResponse>( | ||
| 'https://api.relay.link/quote/v2', | ||
| body, | ||
| { headers }, | ||
| ); | ||
| return response.data; |
There was a problem hiding this comment.
Add timeout to axios request.
No timeout configured for the Relay.link API call.
Proposed fix
const response = await axios.post<RelayQuoteResponse>(
'https://api.relay.link/quote/v2',
body,
- { headers },
+ { headers, timeout: 30000 },
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const response = await axios.post<RelayQuoteResponse>( | |
| 'https://api.relay.link/quote/v2', | |
| body, | |
| { headers }, | |
| ); | |
| return response.data; | |
| const response = await axios.post<RelayQuoteResponse>( | |
| 'https://api.relay.link/quote/v2', | |
| body, | |
| { headers, timeout: 30000 }, | |
| ); | |
| return response.data; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/e2e/bridgeViaRelay.ts` around lines 98 - 103, The axios POST to
Relay.link (the call using axios.post<RelayQuoteResponse>(...)) has no request
timeout; update the call in bridgeViaRelay.ts to include a sensible timeout
(e.g. a few seconds) so the request cannot hang indefinitely—either add a
timeout property to the existing options object passed to axios.post (alongside
headers) or use an axios instance with a default timeout; ensure the timeout
value is configurable or documented and that any timeout errors are
handled/propagated consistently by the surrounding function (the code that
awaits response.data).
Comment on lines
+282
to
+287
| if (token == CurrencyLib.NATIVE_TOKEN_ADDRESS) { | ||
| // ETH is already sent as msg.value directly to this contract. | ||
| if (msg.value < amount) { | ||
| revert InsufficientMsgValue(); | ||
| } | ||
| return; |
There was a problem hiding this comment.
Exact native token amount check missing.
Identical issue to BungeeOpenRouterV2Unchecked: line 284 checks msg.value >= amount instead of equality. Excess ETH accumulates in the contract.
This should enforce if (msg.value != amount) for exact amount matching.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/combined/BungeeOpenRouterV2.sol` around lines 282 - 287, The native-token
branch in the function handling token transfers currently allows msg.value >
amount (the block checking token == CurrencyLib.NATIVE_TOKEN_ADDRESS uses if
(msg.value < amount)), which causes excess ETH to remain in the contract; change
that conditional to require exact equality (use if (msg.value != amount) revert
InsufficientMsgValue();) in the token == CurrencyLib.NATIVE_TOKEN_ADDRESS branch
so the call only succeeds when msg.value exactly equals amount.
Comment on lines
+250
to
+256
| if (token == CurrencyLib.NATIVE_TOKEN_ADDRESS) { | ||
| // ETH is already sent as msg.value directly to this contract. | ||
| if (msg.value < amount) { | ||
| revert InsufficientMsgValue(); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
Exact native token amount check missing.
Line 252 checks msg.value >= amount but doesn't enforce equality. If a caller sends excess ETH, it accumulates in the contract. While this doesn't create a direct security issue, it can lead to:
- ETH dust accumulation across multiple transactions
- Confusion about contract balance state
- Potential griefing by intentionally over-funding
This should change the check to if (msg.value != amount) to enforce exact amounts, or add a refund mechanism for excess value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/combined/BungeeOpenRouterV2Unchecked.sol` around lines 250 - 256, The
current check for native token uses only msg.value < amount which allows excess
ETH to remain in the contract; update the logic in the block handling
CurrencyLib.NATIVE_TOKEN_ADDRESS to require exact payment (use msg.value ==
amount) or implement an explicit refund path for msg.value > amount; modify the
condition that now reverts via InsufficientMsgValue() so it instead reverts on
msg.value != amount (or add refund and only revert on msg.value < amount),
referencing the variables token, CurrencyLib.NATIVE_TOKEN_ADDRESS, msg.value,
amount and the existing InsufficientMsgValue() error to locate and update the
code.
Comment on lines
+44
to
+75
| function test_openOceanSwapManipulatorAcrossDeposit_arbitrumFork() public { | ||
| string memory rpcUrl = vm.envOr("ARBITRUM_RPC", string("")); | ||
| if (bytes(rpcUrl).length != 0) { | ||
| uint256 forkBlock = vm.envOr("ARBITRUM_FORK_BLOCK", FORK_BLOCK_NUMBER); | ||
| vm.createSelectFork(rpcUrl, forkBlock); | ||
| vm.warp(FORK_BLOCK_TIMESTAMP); | ||
| } | ||
|
|
||
| Router router = _routerAtFixtureAddress(); | ||
| AcrossERC20AmountManipulator manipulator = new AcrossERC20AmountManipulator(); | ||
| if (bytes(rpcUrl).length == 0) { | ||
| emit log("Set ARBITRUM_RPC to execute this fork PoC."); | ||
| return; | ||
| } | ||
|
|
||
| uint256 inputAmount = vm.envOr("POC_USDC_AMOUNT", SWAP_INPUT_USDC); | ||
| uint256 bridgeFee = vm.envOr("POC_ACROSS_BRIDGE_FEE", DEFAULT_ACROSS_BRIDGE_FEE); | ||
|
|
||
| deal(ARBITRUM_USDC, address(router), inputAmount); | ||
| deal(ARBITRUM_WETH, address(router), 0); | ||
|
|
||
| Router.Action[] memory actions = | ||
| _buildActions(manipulator, inputAmount, bridgeFee, vm.parseBytes(OPENOCEAN_SWAP_CALLDATA)); | ||
| uint256 spokePoolWethBefore = ERC20(ARBITRUM_WETH).balanceOf(ACROSS_ARBITRUM_SPOKE_POOL); | ||
|
|
||
| uint256 gasBeforeExecute = gasleft(); | ||
| bytes[] memory results = router.performModularExecution(actions); | ||
| uint256 executeGasUsed = gasBeforeExecute - gasleft(); | ||
| emit log_named_uint("router.performModularExecution gas used", executeGasUsed); | ||
|
|
||
| _assertPocResult(router, bridgeFee, spokePoolWethBefore, results[2]); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Add failure scenario tests.
This PoC only tests the happy path. Essential failure scenarios are missing:
- Zero input amount
- Insufficient balance
- Swap failure (e.g., slippage exceeded)
- Bridge failure (e.g., invalid spoke pool interaction)
These scenarios matter for validating error handling in the modular execution flow. As per coding guidelines, tests should cover both happy path and failure scenarios.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/poc/OpenOceanAcrossOpenRouterPoC.t.sol` around lines 44 - 75, Add
negative-case PoC tests alongside
test_openOceanSwapManipulatorAcrossDeposit_arbitrumFork: implement separate
functions e.g., test_openOceanSwap_zeroInput_arbitrumFork,
test_openOceanSwap_insufficientBalance_arbitrumFork,
test_openOceanSwap_swapFailure_arbitrumFork,
test_openOceanSwap_bridgeFailure_arbitrumFork that reuse Router,
AcrossERC20AmountManipulator, _buildActions and deal to set up conditions and
then call router.performModularExecution; for zero input set inputAmount = 0 and
assert performModularExecution reverts or returns an error, for insufficient
balance set deal to a smaller amount than input and expect revert, for swap
failure provide malformed/mismatched OPENOCEAN_SWAP_CALLDATA or manipulate
slippage params to force the swap step to fail and assert revert/result
indicates swap failure, and for bridge failure configure the spoke
pool/token/address to an invalid value before building actions and assert the
bridge step fails; in each test assert balances (ERC20 balanceOf on router and
ACROSS_ARBITRUM_SPOKE_POOL) and emitted failure semantics (revert or error
result) instead of success and reuse _assertPocResult-like checks adapted for
expected failures.
Comment on lines
+76
to
+119
| function test_openOceanSwapStargateNativeBridge_arbitrumFork() public { | ||
| string memory rpcUrl = vm.envOr("ARBITRUM_RPC", string("")); | ||
| if (bytes(rpcUrl).length != 0) { | ||
| uint256 forkBlock = vm.envOr("ARBITRUM_FORK_BLOCK", FORK_BLOCK_NUMBER); | ||
| vm.createSelectFork(rpcUrl, forkBlock); | ||
| vm.warp(FORK_BLOCK_TIMESTAMP); | ||
| } | ||
|
|
||
| Router router = _routerAtFixtureAddress(); | ||
| MathManipulator manipulator = new MathManipulator(); | ||
| if (bytes(rpcUrl).length == 0) { | ||
| emit log("Set ARBITRUM_RPC to execute this fork PoC."); | ||
| return; | ||
| } | ||
|
|
||
| uint256 inputAmount = vm.envOr("POC_USDC_AMOUNT", SWAP_INPUT_USDC); | ||
| uint256 nativeFee = vm.envOr("POC_STARGATE_NATIVE_FEE", STARGATE_NATIVE_FEE); | ||
|
|
||
| deal(ARBITRUM_USDC, address(router), inputAmount); | ||
| uint256 initialNativeBalance = address(router).balance; | ||
| uint256 initialFeeRecipientBalance = FEE_RECIPIENT.balance; | ||
| uint256 initialWethBalance = ERC20(ARBITRUM_WETH).balanceOf(address(router)); | ||
|
|
||
| Router.Action[] memory actions = _buildActions( | ||
| manipulator, inputAmount, nativeFee, _openOceanSwapCalldata(inputAmount), _stargateCalldata(nativeFee) | ||
| ); | ||
|
|
||
| uint256 gasBeforeExecute = gasleft(); | ||
| bytes[] memory results = router.performModularExecution(actions); | ||
| uint256 executeGasUsed = gasBeforeExecute - gasleft(); | ||
| emit log_named_uint("router.performModularExecution gas used", executeGasUsed); | ||
|
|
||
| _assertPocResult( | ||
| router, | ||
| nativeFee, | ||
| initialNativeBalance, | ||
| initialFeeRecipientBalance, | ||
| initialWethBalance, | ||
| results[1], | ||
| results[2], | ||
| results[4], | ||
| results[5] | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Missing failure scenarios and edge case coverage.
The test only covers a single happy-path fork scenario. As per coding guidelines, tests should cover:
- Failure scenarios: What happens when the swap fails? When Stargate bridge reverts? When fee > swap output?
- Edge cases: Zero input amount, maximum uint256 values, boundary conditions for fee BPS
- setUp() function: Test initialization should be in setUp() for consistency
This should add:
- Negative test cases (e.g.,
test_RevertWhen_SwapFails,test_RevertWhen_InsufficientOutput) - Edge case tests (e.g.,
test_ZeroAmount,test_MaxFee) - Extract common setup (router deployment, balance setup) into
setUp()
As per coding guidelines: "Tests cover both happy path and failure scenarios" and "Edge cases tested - zero values, maximum values, boundary conditions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/poc/OpenOceanStargateNativeOpenRouterPoC.t.sol` around lines 76 - 119,
The current test test_openOceanSwapStargateNativeBridge_arbitrumFork only covers
the happy path; add negative tests and edge-case tests and extract common
initialization into setUp() to avoid duplication. Create a setUp() that
deploys/initializes Router, MathManipulator, funds balances (used in
test_openOceanSwapStargateNativeBridge_arbitrumFork and new tests), then add
tests such as test_RevertWhen_SwapFails (simulate OpenOcean swap revert by
crafting failing _openOceanSwapCalldata or mocking response and assert
performModularExecution reverts), test_RevertWhen_InsufficientOutput (set fee >
swap output and assert revert), test_ZeroAmount (inputAmount = 0 and assert
expected revert or no-op), and test_MaxFee / test_FeeBoundaryConditions (fee =
max uint256 or boundary BPS values) using the helpers _buildActions,
_openOceanSwapCalldata, _stargateCalldata and verify results via
_assertPocResult or expectRevert as appropriate.
Comment on lines
+76
to
+89
| function test_openOceanSwapStargateNativeBridge_arbitrumFork() public { | ||
| string memory rpcUrl = vm.envOr("ARBITRUM_RPC", string("")); | ||
| if (bytes(rpcUrl).length != 0) { | ||
| uint256 forkBlock = vm.envOr("ARBITRUM_FORK_BLOCK", FORK_BLOCK_NUMBER); | ||
| vm.createSelectFork(rpcUrl, forkBlock); | ||
| vm.warp(FORK_BLOCK_TIMESTAMP); | ||
| } | ||
|
|
||
| Router router = _routerAtFixtureAddress(); | ||
| MathManipulator manipulator = new MathManipulator(); | ||
| if (bytes(rpcUrl).length == 0) { | ||
| emit log("Set ARBITRUM_RPC to execute this fork PoC."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Test silently skips instead of failing when ARBITRUM_RPC is not set.
Lines 86-89 emit a log and return early if the RPC URL is not configured. This causes the test to pass without actually testing anything.
This should either:
- Use
vm.skip()to explicitly mark the test as skipped, or - Remove the conditional and require ARBITRUM_RPC to be set for the test suite to pass
Silent success on missing configuration masks broken tests in CI/CD pipelines.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/poc/OpenOceanStargateNativeOpenRouterPoC.t.sol` around lines 76 - 89,
The test test_openOceanSwapStargateNativeBridge_arbitrumFork currently emits a
log and returns when ARBITRUM_RPC is missing, causing a silent pass; replace
that emit+return behavior in the conditional that checks bytes(rpcUrl).length ==
0 with an explicit vm.skip("ARBITRUM_RPC not set") to mark the test skipped, or
instead enforce configuration by using a require(bytes(rpcUrl).length != 0,
"ARBITRUM_RPC not set") so the test fails when missing; update the conditional
in test_openOceanSwapStargateNativeBridge_arbitrumFork (where Router router =
_routerAtFixtureAddress() and MathManipulator manipulator = new
MathManipulator() are set) to use one of these two approaches.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (5)
scripts/e2e/swapBridgeViaCctp.ts (2)
99-99:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd timeout to prevent indefinite hangs.
The OpenOcean API call has no timeout. If the API is slow or unresponsive, this script will hang indefinitely.
🔧 Proposed fix
- const response = await axios.get<OpenOceanSwapQuoteResponse>(url, { params }); + const response = await axios.get<OpenOceanSwapQuoteResponse>(url, { params, timeout: 30000 });As per coding guidelines, handle edge cases like network failures.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/e2e/swapBridgeViaCctp.ts` at line 99, The OpenOcean API call using axios.get<OpenOceanSwapQuoteResponse>(url, { params }) lacks a timeout and can hang; add a timeout option to the request options (e.g., include timeout: <ms>) and wrap the call in a try/catch around the axios.get call that assigns to response so network timeouts are handled gracefully (log/throw a descriptive error or handle retry as appropriate).
270-270:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHardcoded flag contradicts documentation.
The docstring on lines 20-21 describes
USE_MODULAR=trueenv var, but line 270 hardcodesfalse. This should read from environment to match documented behavior.🔧 Proposed fix
- const useModular = false; + const useModular = process.env.USE_MODULAR === 'true';As per coding guidelines, ensure the code matches its documentation and check for edge cases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/e2e/swapBridgeViaCctp.ts` at line 270, The hardcoded flag const useModular = false contradicts the documented USE_MODULAR env var; update the declaration (useModular) to read and parse process.env.USE_MODULAR into a boolean (e.g., treat "true" or "1" as true, default to false) so runtime behavior matches the docstring; locate the variable useModular in scripts/e2e/swapBridgeViaCctp.ts and replace the hardcoded value with an environment-based boolean conversion, ensuring downstream code using useModular continues to receive a boolean.OPENROUTER.md (1)
260-272:⚠️ Potential issue | 🟡 Minor | ⚖️ Poor tradeoffFlow examples remain fragmented and unclear.
These numbered flows still lack sufficient context:
- Symbols like
OpenRouter,AcrossManipulator,AcrossRouter,SpokePoolare undefined- The notation
AcrossRouter(amount, AcrossBridgeData)doesn't follow standard function syntax- Line 272's "should have all the fee, swap, bridge code in this" is vague
This should either expand into complete flow descriptions with defined symbols, or link to test files that demonstrate these patterns (e.g.,
test/poc/OpenOceanAcrossOpenRouterPoC.t.sol).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@OPENROUTER.md` around lines 260 - 272, The flow examples for AllowanceHolder, OpenRouter, AcrossManipulator, AcrossRouter(amount, AcrossBridgeData) and SpokePool are too terse and use undefined symbols/notation; update OPENROUTER.md by replacing each numbered fragment with a clear, step-by-step flow that defines each symbol (e.g., "OpenRouter: contract that handles X", "AcrossRouter(amount, AcrossBridgeData): function signature where AcrossBridgeData = {fromToken, toToken, recipient, relayerFee,...}") and show the exact call sequence and state transitions (what input is modified and what output is passed to the next step) for at least one full example, or alternatively add direct links/references to the concrete test demonstrating the pattern (e.g., test/poc/OpenOceanAcrossOpenRouterPoC.t.sol) so readers can inspect the real implementation and inputs/outputs; ensure you normalize function notation to standard signatures and remove vague remarks like "should have all the fee, swap, bridge code in this" by either expanding the implementation details or pointing to the exact test/contract that contains them.scripts/deploy/deployBungeeOpenRouterV2.ts (1)
21-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDead code: this check can never trigger.
openRouterSigneris hardcoded todeployer.addresson line 22, making the check on line 24 unreachable. The docstring claimsOPEN_ROUTER_SIGNER_ADDRESSis required, but the code never reads from environment.🔧 Proposed fix
- const owner = deployer.address; - const openRouterSigner = deployer.address; - - if (!openRouterSigner) { - throw new Error('OPEN_ROUTER_SIGNER_ADDRESS is not set in environment'); - } + const owner = process.env.OWNER_ADDRESS ?? deployer.address; + const openRouterSigner = process.env.OPEN_ROUTER_SIGNER_ADDRESS ?? deployer.address;As per coding guidelines, check for edge cases and ensure the code matches its documentation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/deploy/deployBungeeOpenRouterV2.ts` around lines 21 - 26, The code sets openRouterSigner to deployer.address, making the subsequent existence check dead; instead read the required OPEN_ROUTER_SIGNER_ADDRESS from the environment and validate it. Replace the assignment of openRouterSigner with a read from process.env.OPEN_ROUTER_SIGNER_ADDRESS (e.g., const openRouterSigner = process.env.OPEN_ROUTER_SIGNER_ADDRESS) and keep the existing if (!openRouterSigner) throw new Error(...) check so the error will trigger when the env var is missing; reference symbols: openRouterSigner, deployer.address, OPEN_ROUTER_SIGNER_ADDRESS.test/poc/OpenOceanStargateNativeOpenRouterPoC.t.sol (1)
86-89:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest silently passes when
ARBITRUM_RPCis not set.Same issue as OpenOceanAcrossOpenRouterPoC.t.sol. The test passes without executing when RPC is not configured, which masks skipped tests in CI.
Proposed fix
if (bytes(rpcUrl).length == 0) { - emit log("Set ARBITRUM_RPC to execute this fork PoC."); - return; + vm.skip(true); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/poc/OpenOceanStargateNativeOpenRouterPoC.t.sol` around lines 86 - 89, The test currently silently returns when rpcUrl is empty (the if block checking bytes(rpcUrl).length == 0), which masks skipped tests in CI; replace the emit log + return behavior with a hard failure so missing configuration fails the test (e.g., use require(bytes(rpcUrl).length != 0, "Set ARBITRUM_RPC to execute this fork PoC.") or revert with a clear message) so tests fail loudly when rpcUrl is not set; locate the conditional that references rpcUrl in OpenOceanStargateNativeOpenRouterPoC.t.sol and change it accordingly.
🧹 Nitpick comments (3)
scripts/e2e/utils/modularActionsBuilder/index.d.ts (2)
36-36: 💤 Low valueRemove redundant type alias.
Actionis just an alias forLogicalActionwith no added value. This creates two names for the same concept, making the codebase harder to navigate.♻️ Proposed fix
-export type Action = LogicalAction;Update references to use
LogicalActiondirectly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/e2e/utils/modularActionsBuilder/index.d.ts` at line 36, Remove the redundant type alias "Action" that merely re-exports "LogicalAction": delete the line `export type Action = LogicalAction;` from the declaration file and update all usages/references of "Action" across the codebase to use "LogicalAction" directly (search for symbol "Action" and replace with "LogicalAction"), ensuring imports/exports remain consistent and tests/type-checking still pass after the renaming.
5-7: ⚡ Quick win
ExecutionContexttype is too permissive.The index signature
[key: string]: unknownallows arbitrary properties with no type safety. If this is used for metadata/logging, document that clearly. If specific properties are expected (e.g.,routeId,chainIdfrom the README example), define them explicitly.♻️ Proposed fix
export interface ExecutionContext { + routeId?: string; + chainId?: number; [key: string]: unknown; }As per coding guidelines, check whether this structure clearly defines its purpose and whether another developer will understand it in 6 months.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/e2e/utils/modularActionsBuilder/index.d.ts` around lines 5 - 7, The ExecutionContext type is too permissive; update the ExecutionContext declaration in modularActionsBuilder/index.d.ts to explicitly list expected properties (e.g., routeId?: string, chainId?: string, userId?: string) and their types and make any remaining arbitrary keys intentional by either removing the broad index signature or replacing it with a narrower one (e.g., Record<string, unknown> only if truly needed) and add a short JSDoc comment describing its purpose (metadata/logging) so future devs understand expected fields; target the ExecutionContext interface symbol to apply this change.scripts/deploy/deployBungeeOpenRouterV2.ts (1)
37-42: 💤 Low valueCommented-out code should be removed.
Large blocks of commented code clutter the file. Use version control history instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/deploy/deployBungeeOpenRouterV2.ts` around lines 37 - 42, Remove the large commented-out deployment block for BungeeOpenRouterV2 (the lines referencing V2Factory, V2Factory.deploy, v2.waitForDeployment, v2.getAddress, and the console.log statements) — delete these commented lines entirely so the file no longer contains dead commented code; rely on git history to recover if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Around line 9-13: The .env example contains inline comments on empty
assignments which can be ambiguous to parsers; change each assignment to be just
the key and = with no trailing comment (OPEN_ROUTER_SIGNER_ADDRESS=,
RELAY_API_KEY=, OPEN_OCEAN_API_KEY=) and move the explanatory notes onto
separate comment lines above or below the key (e.g., a line starting with #
describing BungeeOpenRouterV2, relay.link x-api-key, OpenOcean API key) so the
dotenv parser reads an empty value reliably and linters won’t misinterpret
spaces or inline comments.
In `@hardhat.config.ts`:
- Around line 9-11: Normalize the deployer private key before constructing the
accounts array: read process.env.DEPLOYER_PRIVATE_KEY into deployerKey, and if
present ensure it is a 0x-prefixed hex string (prepend "0x" when the value is 64
hex chars without the prefix) before assigning to accounts; update the logic
around deployerKey and accounts in hardhat.config.ts so accounts receives a
valid 0x-prefixed key or remains an empty array.
In `@package.json`:
- Line 7: The "deploy:v2" npm script is incomplete because it ends with the
`--network` flag with no value; update the "deploy:v2" script entry so it either
removes the dangling `--network` flag or provides a sensible default network
name (e.g., set `deploy:v2` to include a default like `--network arbitrum`),
keeping the script name "deploy:v2" unchanged and leaving it possible for users
to override the network at runtime with `npm run deploy:v2 -- --network <name>`.
In `@scripts/e2e/swapBridgeViaArbitrumNative.ts`:
- Around line 325-334: The computed bridge value passed into buildModularActions
can be 0n when minAmountOut ≤ feeAmount; compute that value into a local
variable (e.g., bridgeValue = minAmountOut > feeAmount ? minAmountOut -
feeAmount : 0n) before calling buildModularActions and add a guard that throws a
clear error (e.g., throw new Error("bridgeValue is 0: minAmountOut <=
feeAmount")) if bridgeValue === 0n so we fail fast instead of submitting a
transaction that bridges nothing; update the call to buildModularActions to use
the bridgeValue variable.
In `@scripts/e2e/utils/erc20.ts`:
- Around line 77-87: The approveErc20 function currently calls
token.approve(...) and awaits tx.wait() but never checks the returned receipt;
update approveErc20 to capture the receipt from tx.wait(), verify receipt.status
=== 1 (or truthy) before treating it as success, and throw or reject with an
Error (including tx.hash and tokenAddress/spender/amount) if status === 0 so
failures aren't logged as successes; reference the approveErc20 function and the
token.approve / tx.wait call sites to implement this check and adjust the
console.log to run only after a successful receipt.
In `@src/combined/BungeeOpenRouterV2Unchecked.sol`:
- Around line 211-216: The unchecked subtraction computing swapInput can
underflow when exec.swap.target != address(0); add an explicit bounds check that
requires exec.input.inputAmount >= exec.preFee.amount before performing the
subtraction in the block that contains exec.swap.approvalSpender and
SafeTransferLib.safeApproveWithRetry; if the check fails revert with a clear
error (e.g., InvalidSwapInput or similar) so swapInput is only computed when
safe, and update any related logic in the same function/contract (references:
exec.swap.approvalSpender, exec.input.inputAmount, exec.preFee.amount,
swapInput) to rely on this validation.
In `@test/poc/OpenOceanAcrossOpenRouterPoC.t.sol`:
- Around line 54-57: The test currently emits a log and returns when the
ARBITRUM_RPC env var is missing (checking bytes(rpcUrl).length == 0), which lets
the test silently pass; change this to explicitly skip the test by calling
vm.skip(true) (optionally with a clear message) when rpcUrl is empty so CI
records the test as skipped rather than passed; locate the check that references
rpcUrl and replace the emit log + return behavior with vm.skip(true).
---
Duplicate comments:
In `@OPENROUTER.md`:
- Around line 260-272: The flow examples for AllowanceHolder, OpenRouter,
AcrossManipulator, AcrossRouter(amount, AcrossBridgeData) and SpokePool are too
terse and use undefined symbols/notation; update OPENROUTER.md by replacing each
numbered fragment with a clear, step-by-step flow that defines each symbol
(e.g., "OpenRouter: contract that handles X", "AcrossRouter(amount,
AcrossBridgeData): function signature where AcrossBridgeData = {fromToken,
toToken, recipient, relayerFee,...}") and show the exact call sequence and state
transitions (what input is modified and what output is passed to the next step)
for at least one full example, or alternatively add direct links/references to
the concrete test demonstrating the pattern (e.g.,
test/poc/OpenOceanAcrossOpenRouterPoC.t.sol) so readers can inspect the real
implementation and inputs/outputs; ensure you normalize function notation to
standard signatures and remove vague remarks like "should have all the fee,
swap, bridge code in this" by either expanding the implementation details or
pointing to the exact test/contract that contains them.
In `@scripts/deploy/deployBungeeOpenRouterV2.ts`:
- Around line 21-26: The code sets openRouterSigner to deployer.address, making
the subsequent existence check dead; instead read the required
OPEN_ROUTER_SIGNER_ADDRESS from the environment and validate it. Replace the
assignment of openRouterSigner with a read from
process.env.OPEN_ROUTER_SIGNER_ADDRESS (e.g., const openRouterSigner =
process.env.OPEN_ROUTER_SIGNER_ADDRESS) and keep the existing if
(!openRouterSigner) throw new Error(...) check so the error will trigger when
the env var is missing; reference symbols: openRouterSigner, deployer.address,
OPEN_ROUTER_SIGNER_ADDRESS.
In `@scripts/e2e/swapBridgeViaCctp.ts`:
- Line 99: The OpenOcean API call using
axios.get<OpenOceanSwapQuoteResponse>(url, { params }) lacks a timeout and can
hang; add a timeout option to the request options (e.g., include timeout: <ms>)
and wrap the call in a try/catch around the axios.get call that assigns to
response so network timeouts are handled gracefully (log/throw a descriptive
error or handle retry as appropriate).
- Line 270: The hardcoded flag const useModular = false contradicts the
documented USE_MODULAR env var; update the declaration (useModular) to read and
parse process.env.USE_MODULAR into a boolean (e.g., treat "true" or "1" as true,
default to false) so runtime behavior matches the docstring; locate the variable
useModular in scripts/e2e/swapBridgeViaCctp.ts and replace the hardcoded value
with an environment-based boolean conversion, ensuring downstream code using
useModular continues to receive a boolean.
In `@test/poc/OpenOceanStargateNativeOpenRouterPoC.t.sol`:
- Around line 86-89: The test currently silently returns when rpcUrl is empty
(the if block checking bytes(rpcUrl).length == 0), which masks skipped tests in
CI; replace the emit log + return behavior with a hard failure so missing
configuration fails the test (e.g., use require(bytes(rpcUrl).length != 0, "Set
ARBITRUM_RPC to execute this fork PoC.") or revert with a clear message) so
tests fail loudly when rpcUrl is not set; locate the conditional that references
rpcUrl in OpenOceanStargateNativeOpenRouterPoC.t.sol and change it accordingly.
---
Nitpick comments:
In `@scripts/deploy/deployBungeeOpenRouterV2.ts`:
- Around line 37-42: Remove the large commented-out deployment block for
BungeeOpenRouterV2 (the lines referencing V2Factory, V2Factory.deploy,
v2.waitForDeployment, v2.getAddress, and the console.log statements) — delete
these commented lines entirely so the file no longer contains dead commented
code; rely on git history to recover if needed.
In `@scripts/e2e/utils/modularActionsBuilder/index.d.ts`:
- Line 36: Remove the redundant type alias "Action" that merely re-exports
"LogicalAction": delete the line `export type Action = LogicalAction;` from the
declaration file and update all usages/references of "Action" across the
codebase to use "LogicalAction" directly (search for symbol "Action" and replace
with "LogicalAction"), ensuring imports/exports remain consistent and
tests/type-checking still pass after the renaming.
- Around line 5-7: The ExecutionContext type is too permissive; update the
ExecutionContext declaration in modularActionsBuilder/index.d.ts to explicitly
list expected properties (e.g., routeId?: string, chainId?: string, userId?:
string) and their types and make any remaining arbitrary keys intentional by
either removing the broad index signature or replacing it with a narrower one
(e.g., Record<string, unknown> only if truly needed) and add a short JSDoc
comment describing its purpose (metadata/logging) so future devs understand
expected fields; target the ExecutionContext interface symbol to apply this
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cba1391d-ffd5-4eb2-9326-75903a4a7032
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (28)
.env.example.gitignoreOPENROUTER.mdhardhat.config.tspackage.jsonremappings.txtscripts/deploy/deployBungeeOpenRouterV2.tsscripts/e2e/bridgeViaRelay.tsscripts/e2e/config.tsscripts/e2e/swapBridgeViaArbitrumNative.tsscripts/e2e/swapBridgeViaCctp.tsscripts/e2e/swapBridgeViaStargateNative.tsscripts/e2e/utils/allowanceHolder.tsscripts/e2e/utils/contractTypes.tsscripts/e2e/utils/erc20.tsscripts/e2e/utils/modularActionsBuilder/README.mdscripts/e2e/utils/modularActionsBuilder/index.d.tsscripts/e2e/utils/modularActionsBuilder/index.jsscripts/e2e/utils/routerAbi.tssrc/combined/BungeeOpenRouterV2.solsrc/combined/BungeeOpenRouterV2Unchecked.solsrc/dummyRouter.solsrc/manipulators/AcrossERC20AmountManipulator.solsrc/manipulators/MathManipulator.solsrc/monolithic/BungeeOpenRouterAH.soltest/poc/OpenOceanAcrossOpenRouterPoC.t.soltest/poc/OpenOceanStargateNativeOpenRouterPoC.t.soltsconfig.json
💤 Files with no reviewable changes (1)
- src/dummyRouter.sol
Comment on lines
+9
to
+13
| OPEN_ROUTER_SIGNER_ADDRESS= # only needed for BungeeOpenRouterV2 (not Unchecked) | ||
|
|
||
| # External API keys | ||
| RELAY_API_KEY= # optional, relay.link x-api-key header | ||
| OPEN_OCEAN_API_KEY= # optional, OpenOcean API key |
There was a problem hiding this comment.
Avoid inline comments on empty env assignments.
Lines 9, 12, and 13 should keep comments on separate lines (KEY= only) to avoid parser/linter ambiguity for empty values.
In dotenv (Node.js dotenv v16), how is `KEY= # comment` parsed? Is the value treated as empty string, spaces, or literal with comment?
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 9-9: [SpaceCharacter] The line has spaces around equal sign
(SpaceCharacter)
[warning] 9-9: [UnorderedKey] The OPEN_ROUTER_SIGNER_ADDRESS key should go before the OWNER_ADDRESS key
(UnorderedKey)
[warning] 9-9: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 12-12: [SpaceCharacter] The line has spaces around equal sign
(SpaceCharacter)
[warning] 12-12: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 13-13: [SpaceCharacter] The line has spaces around equal sign
(SpaceCharacter)
[warning] 13-13: [UnorderedKey] The OPEN_OCEAN_API_KEY key should go before the RELAY_API_KEY key
(UnorderedKey)
[warning] 13-13: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.env.example around lines 9 - 13, The .env example contains inline comments
on empty assignments which can be ambiguous to parsers; change each assignment
to be just the key and = with no trailing comment (OPEN_ROUTER_SIGNER_ADDRESS=,
RELAY_API_KEY=, OPEN_OCEAN_API_KEY=) and move the explanatory notes onto
separate comment lines above or below the key (e.g., a line starting with #
describing BungeeOpenRouterV2, relay.link x-api-key, OpenOcean API key) so the
dotenv parser reads an empty value reliably and linters won’t misinterpret
spaces or inline comments.
Comment on lines
+9
to
+11
| const deployerKey = process.env.DEPLOYER_PRIVATE_KEY; | ||
| const accounts = deployerKey ? [deployerKey] : []; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Hardhat v2.22.x network config, does accounts: [privateKey]require0x-prefixed hex private keys, or are non-prefixed 64-hex strings accepted?
💡 Result:
In Hardhat v2.22.x, when you set a network’s accounts as an array of private-key strings (e.g. accounts: [privateKey]), the private keys must be 0x-prefixed hex strings; a non-prefixed 64-hex string is not accepted in practice. Hard evidence: - Hardhat’s network configuration docs describe accounts as “an array of hex-encoded private keys”. In the examples, they are shown with a 0x prefix (e.g. accounts: [privateKey1, privateKey2, ...] in config and discussions consistently show accounts: [�9x<PRIVATE_KEY> ]). [1][2] - Users commonly hit Hardhat/ethers errors when the private key is missing the 0x prefix (e.g. ethers.js throws “hex string must have 0x prefix” when given an unprefixed hex string). [3] - Community guidance for Hardhat config specifically says to add 0x as a prefix of the MetaMask private key before passing it in the accounts array. [2] So: use 0x + 64 hex characters (32 bytes). If you currently have a non-prefixed 64-hex string, normalize it like: accounts: [0x${PRIVATE_KEY}] [2].
Citations:
- 1: https://hardhat.org/hardhat-runner/docs/config
- 2: https://ethereum.stackexchange.com/questions/130006/help-please-hardhat-config-js-invalid-account-0-for-network-rinkeby-priva
- 3: new Wallet(privateKey) throws "invalid hexlify value" when privateKey is not 0x prefixed ethers-io/ethers.js#1166
Normalize deployer private key with 0x prefix before passing to accounts.
The code passes process.env.DEPLOYER_PRIVATE_KEY directly to the accounts array. Hardhat requires private keys to be 0x-prefixed hex strings; a non-prefixed 64-hex string will cause runtime failures during network operations. Since your .env.example allows unprefixed values, this should normalize the key to prevent invalid account configuration.
Diff
const deployerKey = process.env.DEPLOYER_PRIVATE_KEY;
-const accounts = deployerKey ? [deployerKey] : [];
+const normalizedDeployerKey = deployerKey
+ ? deployerKey.startsWith("0x")
+ ? deployerKey
+ : `0x${deployerKey}`
+ : undefined;
+const accounts = normalizedDeployerKey ? [normalizedDeployerKey] : [];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const deployerKey = process.env.DEPLOYER_PRIVATE_KEY; | |
| const accounts = deployerKey ? [deployerKey] : []; | |
| const deployerKey = process.env.DEPLOYER_PRIVATE_KEY; | |
| const normalizedDeployerKey = deployerKey | |
| ? deployerKey.startsWith("0x") | |
| ? deployerKey | |
| : `0x${deployerKey}` | |
| : undefined; | |
| const accounts = normalizedDeployerKey ? [normalizedDeployerKey] : []; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@hardhat.config.ts` around lines 9 - 11, Normalize the deployer private key
before constructing the accounts array: read process.env.DEPLOYER_PRIVATE_KEY
into deployerKey, and if present ensure it is a 0x-prefixed hex string (prepend
"0x" when the value is 64 hex chars without the prefix) before assigning to
accounts; update the logic around deployerKey and accounts in hardhat.config.ts
so accounts receives a valid 0x-prefixed key or remains an empty array.
| "private": true, | ||
| "scripts": { | ||
| "compile": "hardhat compile", | ||
| "deploy:v2": "hardhat run scripts/deploy/deployBungeeOpenRouterV2.ts --network", |
There was a problem hiding this comment.
Deploy script syntax is incomplete.
The deploy:v2 script ends with --network but provides no network name. This will fail when run.
🔧 Proposed fix
Option A - Remove incomplete flag:
- "deploy:v2": "hardhat run scripts/deploy/deployBungeeOpenRouterV2.ts --network",
+ "deploy:v2": "hardhat run scripts/deploy/deployBungeeOpenRouterV2.ts",Option B - Add placeholder/default:
- "deploy:v2": "hardhat run scripts/deploy/deployBungeeOpenRouterV2.ts --network",
+ "deploy:v2": "hardhat run scripts/deploy/deployBungeeOpenRouterV2.ts --network hardhat",Users can override with npm run deploy:v2 -- --network arbitrum.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "deploy:v2": "hardhat run scripts/deploy/deployBungeeOpenRouterV2.ts --network", | |
| "deploy:v2": "hardhat run scripts/deploy/deployBungeeOpenRouterV2.ts", |
Suggested change
| "deploy:v2": "hardhat run scripts/deploy/deployBungeeOpenRouterV2.ts --network", | |
| "deploy:v2": "hardhat run scripts/deploy/deployBungeeOpenRouterV2.ts --network hardhat", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@package.json` at line 7, The "deploy:v2" npm script is incomplete because it
ends with the `--network` flag with no value; update the "deploy:v2" script
entry so it either removes the dangling `--network` flag or provides a sensible
default network name (e.g., set `deploy:v2` to include a default like `--network
arbitrum`), keeping the script name "deploy:v2" unchanged and leaving it
possible for users to override the network at runtime with `npm run deploy:v2 --
--network <name>`.
Comment on lines
+325
to
+334
| if (useModular) { | ||
| const actions = buildModularActions( | ||
| signerAddress, | ||
| ROUTER_ADDRESS, | ||
| inputAmount, | ||
| feeAmount, | ||
| minAmountOut > feeAmount ? minAmountOut - feeAmount : 0n, | ||
| ooRouterAddress, | ||
| swapData, | ||
| ); |
There was a problem hiding this comment.
Edge case: bridgeValue can be 0 when minAmountOut ≤ feeAmount.
Line 331 calculates minAmountOut > feeAmount ? minAmountOut - feeAmount : 0n. If minAmountOut is unexpectedly small (due to extreme slippage or low input), the bridge receives 0 ETH but the transaction proceeds. This should fail fast with a clear error rather than submitting a transaction that bridges nothing.
Proposed fix
+ const bridgeValue = minAmountOut > feeAmount ? minAmountOut - feeAmount : 0n;
+ if (bridgeValue === 0n) {
+ throw new Error(
+ `minAmountOut (${ethers.formatEther(minAmountOut)}) is too small to cover ` +
+ `feeAmount (${ethers.formatEther(feeAmount)}). Increase AAVE balance.`,
+ );
+ }
+
if (useModular) {
const actions = buildModularActions(
signerAddress,
ROUTER_ADDRESS,
inputAmount,
feeAmount,
- minAmountOut > feeAmount ? minAmountOut - feeAmount : 0n,
+ bridgeValue,
ooRouterAddress,
swapData,
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (useModular) { | |
| const actions = buildModularActions( | |
| signerAddress, | |
| ROUTER_ADDRESS, | |
| inputAmount, | |
| feeAmount, | |
| minAmountOut > feeAmount ? minAmountOut - feeAmount : 0n, | |
| ooRouterAddress, | |
| swapData, | |
| ); | |
| const bridgeValue = minAmountOut > feeAmount ? minAmountOut - feeAmount : 0n; | |
| if (bridgeValue === 0n) { | |
| throw new Error( | |
| `minAmountOut (${ethers.formatEther(minAmountOut)}) is too small to cover ` + | |
| `feeAmount (${ethers.formatEther(feeAmount)}). Increase AAVE balance.`, | |
| ); | |
| } | |
| if (useModular) { | |
| const actions = buildModularActions( | |
| signerAddress, | |
| ROUTER_ADDRESS, | |
| inputAmount, | |
| feeAmount, | |
| bridgeValue, | |
| ooRouterAddress, | |
| swapData, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/e2e/swapBridgeViaArbitrumNative.ts` around lines 325 - 334, The
computed bridge value passed into buildModularActions can be 0n when
minAmountOut ≤ feeAmount; compute that value into a local variable (e.g.,
bridgeValue = minAmountOut > feeAmount ? minAmountOut - feeAmount : 0n) before
calling buildModularActions and add a guard that throws a clear error (e.g.,
throw new Error("bridgeValue is 0: minAmountOut <= feeAmount")) if bridgeValue
=== 0n so we fail fast instead of submitting a transaction that bridges nothing;
update the call to buildModularActions to use the bridgeValue variable.
Comment on lines
+77
to
+87
| export async function approveErc20( | ||
| tokenAddress: string, | ||
| spender: string, | ||
| amount: bigint, | ||
| signer: ethers.Signer, | ||
| ): Promise<void> { | ||
| const token = getErc20Contract(tokenAddress, signer); | ||
| const tx = await token.approve(spender, amount); | ||
| await tx.wait(); | ||
| console.log(`Approved ${spender} for ${amount} of ${tokenAddress}`); | ||
| } |
There was a problem hiding this comment.
Missing transaction status check in approveErc20.
tx.wait() can return a receipt with status === 0 (reverted). The function logs success without verifying the transaction succeeded.
Proposed fix
export async function approveErc20(
tokenAddress: string,
spender: string,
amount: bigint,
signer: ethers.Signer,
): Promise<void> {
const token = getErc20Contract(tokenAddress, signer);
const tx = await token.approve(spender, amount);
- await tx.wait();
- console.log(`Approved ${spender} for ${amount} of ${tokenAddress}`);
+ const receipt = await tx.wait();
+ if (!receipt || receipt.status !== 1) {
+ throw new Error(`ERC20 approve failed: ${tx.hash}`);
+ }
+ console.log(`Approved ${spender} for ${amount} of ${tokenAddress}`);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/e2e/utils/erc20.ts` around lines 77 - 87, The approveErc20 function
currently calls token.approve(...) and awaits tx.wait() but never checks the
returned receipt; update approveErc20 to capture the receipt from tx.wait(),
verify receipt.status === 1 (or truthy) before treating it as success, and throw
or reject with an Error (including tx.hash and tokenAddress/spender/amount) if
status === 0 so failures aren't logged as successes; reference the approveErc20
function and the token.approve / tx.wait call sites to implement this check and
adjust the console.log to run only after a successful receipt.
Comment on lines
+211
to
+216
| if (exec.swap.approvalSpender != address(0) && exec.input.inputToken != CurrencyLib.NATIVE_TOKEN_ADDRESS) { | ||
| uint256 swapInput; | ||
| unchecked { | ||
| swapInput = exec.input.inputAmount - exec.preFee.amount; | ||
| } | ||
| SafeTransferLib.safeApproveWithRetry(exec.input.inputToken, exec.swap.approvalSpender, swapInput); |
There was a problem hiding this comment.
Potential underflow in swap input calculation.
When exec.swap.target != address(0), control flows here without validating that exec.input.inputAmount >= exec.preFee.amount. The unchecked subtraction at line 214 can underflow, producing an enormous swapInput value.
The validation at lines 168-170 only runs in the no-swap branch. This should add a bounds check before the subtraction:
🛡️ Proposed fix
function _performSwap(MonolithicExecution calldata exec)
internal
returns (address finalToken, uint256 finalAmount)
{
uint256 preBalance = CurrencyLib.balanceOf(exec.swap.outputToken, address(this));
if (exec.swap.approvalSpender != address(0) && exec.input.inputToken != CurrencyLib.NATIVE_TOKEN_ADDRESS) {
+ if (exec.preFee.amount > exec.input.inputAmount) {
+ revert InsufficientFunds();
+ }
uint256 swapInput;
unchecked {
swapInput = exec.input.inputAmount - exec.preFee.amount;
}
SafeTransferLib.safeApproveWithRetry(exec.input.inputToken, exec.swap.approvalSpender, swapInput);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (exec.swap.approvalSpender != address(0) && exec.input.inputToken != CurrencyLib.NATIVE_TOKEN_ADDRESS) { | |
| uint256 swapInput; | |
| unchecked { | |
| swapInput = exec.input.inputAmount - exec.preFee.amount; | |
| } | |
| SafeTransferLib.safeApproveWithRetry(exec.input.inputToken, exec.swap.approvalSpender, swapInput); | |
| if (exec.swap.approvalSpender != address(0) && exec.input.inputToken != CurrencyLib.NATIVE_TOKEN_ADDRESS) { | |
| if (exec.preFee.amount > exec.input.inputAmount) { | |
| revert InsufficientFunds(); | |
| } | |
| uint256 swapInput; | |
| unchecked { | |
| swapInput = exec.input.inputAmount - exec.preFee.amount; | |
| } | |
| SafeTransferLib.safeApproveWithRetry(exec.input.inputToken, exec.swap.approvalSpender, swapInput); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/combined/BungeeOpenRouterV2Unchecked.sol` around lines 211 - 216, The
unchecked subtraction computing swapInput can underflow when exec.swap.target !=
address(0); add an explicit bounds check that requires exec.input.inputAmount >=
exec.preFee.amount before performing the subtraction in the block that contains
exec.swap.approvalSpender and SafeTransferLib.safeApproveWithRetry; if the check
fails revert with a clear error (e.g., InvalidSwapInput or similar) so swapInput
is only computed when safe, and update any related logic in the same
function/contract (references: exec.swap.approvalSpender,
exec.input.inputAmount, exec.preFee.amount, swapInput) to rely on this
validation.
Comment on lines
+54
to
+57
| if (bytes(rpcUrl).length == 0) { | ||
| emit log("Set ARBITRUM_RPC to execute this fork PoC."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Test silently passes when ARBITRUM_RPC is not set.
This emit log + return pattern causes the test to pass without executing anything. In CI, this masks that the test wasn't actually run.
Proposed fix
if (bytes(rpcUrl).length == 0) {
- emit log("Set ARBITRUM_RPC to execute this fork PoC.");
- return;
+ vm.skip(true);
}Note: vm.skip(true) explicitly marks the test as skipped rather than silently passing.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (bytes(rpcUrl).length == 0) { | |
| emit log("Set ARBITRUM_RPC to execute this fork PoC."); | |
| return; | |
| } | |
| if (bytes(rpcUrl).length == 0) { | |
| vm.skip(true); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/poc/OpenOceanAcrossOpenRouterPoC.t.sol` around lines 54 - 57, The test
currently emits a log and returns when the ARBITRUM_RPC env var is missing
(checking bytes(rpcUrl).length == 0), which lets the test silently pass; change
this to explicitly skip the test by calling vm.skip(true) (optionally with a
clear message) when rpcUrl is empty so CI records the test as skipped rather
than passed; locate the check that references rpcUrl and replace the emit log +
return behavior with vm.skip(true).
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/combined/BungeeOpenRouterV2Unchecked.sol (1)
211-216:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
preFee.amountbefore the unchecked subtraction.
swapInputwraps here whenexec.preFee.amount > exec.input.inputAmount, which can turn this approval into an absurd allowance. That is still reachable if the router already holds extra balance ofexec.input.inputToken, because the earlier pre-fee transfer can succeed instead of stopping execution. This should reject that state before the fee transfer or before this subtraction.Suggested fix
function _runMonolithic(MonolithicExecution calldata exec) internal { if (exec.bridge.target == address(0) || exec.input.user == address(0) || exec.input.inputToken == address(0)) { revert InvalidExecution(); } // 1. pull funds from user via AllowanceHolder _pullFromUser(exec.input.inputToken, exec.input.user, exec.input.inputAmount); + + if (exec.preFee.amount > exec.input.inputAmount) { + revert InsufficientFunds(); + } // 2. optional pre-swap fee in input token if (exec.preFee.amount != 0) { CurrencyLib.transfer(exec.input.inputToken, exec.preFee.receiver, exec.preFee.amount); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/combined/BungeeOpenRouterV2Unchecked.sol` around lines 211 - 216, The subtraction that computes swapInput can underflow when exec.preFee.amount > exec.input.inputAmount; before performing unchecked subtraction (and preferably before any pre-fee transfer), add a validation that exec.preFee.amount <= exec.input.inputAmount and revert with a clear error if not (e.g., require or custom error). Specifically, in the block that references exec.swap.approvalSpender, exec.input.inputToken, exec.input.inputAmount, and exec.preFee.amount (prior to calling SafeTransferLib.safeApproveWithRetry), check the preFee amount and fail early to prevent underflow and accidental huge allowance; update any callers or state-flow so this validation runs before fee transfers occur.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/poc/OneInchCctpOpenRouterPoC.t.sol`:
- Around line 51-63: Move the POLYGON_RPC existence check into the shared
setUp() so tests are not silently no-ops; in setUp() read
vm.envOr("POLYGON_RPC", ""), vm.envOr("POLYGON_FORK_BLOCK", ...),
createSelectFork and vm.warp there, and require(bytes(rpcUrl).length != 0,
"POLYGON_RPC not set") (or otherwise fail the setup) so missing env causes a
failing precondition instead of early returns. Remove the early-return guards
from test_oneInchSwapCctpBridge_polygonFork and the other test_* cases (the ones
at the other ranges mentioned) so they rely on the shared setUp precondition and
exercise the flows.
- Around line 51-179: Add negative and boundary tests alongside the existing
happy-path PoCs for the three flows (test_oneInchSwapCctpBridge_polygonFork,
test_oneInchSwapCctpBridgeMonolithic_polygonFork,
test_oneInchSwapCctpBridgeSocketGatewayReference_polygonFork): create additional
test cases that call
_buildActions/_buildMonolithicExecution/_referenceSocketGatewayCalldata with bad
splice indices, minOutput set above realistic return (to force revert), zero and
extremely large input amounts, and calls made without the AllowanceHolder
context; for each add expectations that the router methods
(router.performModularExecution, router.performExecution, and the SocketGateway
reference call) revert with the correct custom errors or emit expected failure
events, and assert balances/totalSupply unchanged on revert (using
IAllowanceHolder.exec and direct call patterns shown) to cover boundary and
failure behavior referenced in the review.
- Around line 54-56: The tests currently call vm.createSelectFork(rpcUrl,
forkBlock) then unconditionally vm.warp(FORK_BLOCK_TIMESTAMP), which causes a
timestamp/fork mismatch when POLYGON_FORK_BLOCK is overridden; update the setup
so after computing forkBlock (via vm.envOr("POLYGON_FORK_BLOCK",
FORK_BLOCK_NUMBER)) you either (a) skip vm.warp when an override was supplied
(i.e., when forkBlock != FORK_BLOCK_NUMBER) or (b) derive the correct timestamp
for the selected forkBlock and pass that to vm.warp (fetch the block by number
from the RPC and parse its timestamp) instead of always using
FORK_BLOCK_TIMESTAMP; change the code around vm.createSelectFork and vm.warp
accordingly (referencing forkBlock, FORK_BLOCK_NUMBER, FORK_BLOCK_TIMESTAMP,
vm.createSelectFork and vm.warp).
---
Duplicate comments:
In `@src/combined/BungeeOpenRouterV2Unchecked.sol`:
- Around line 211-216: The subtraction that computes swapInput can underflow
when exec.preFee.amount > exec.input.inputAmount; before performing unchecked
subtraction (and preferably before any pre-fee transfer), add a validation that
exec.preFee.amount <= exec.input.inputAmount and revert with a clear error if
not (e.g., require or custom error). Specifically, in the block that references
exec.swap.approvalSpender, exec.input.inputToken, exec.input.inputAmount, and
exec.preFee.amount (prior to calling SafeTransferLib.safeApproveWithRetry),
check the preFee amount and fail early to prevent underflow and accidental huge
allowance; update any callers or state-flow so this validation runs before fee
transfers occur.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fb49258b-33c7-4a78-90be-e9b4fcde724f
📒 Files selected for processing (8)
scripts/e2e/swapBridgeViaArbitrumNative.tsscripts/e2e/swapBridgeViaCctp.tsscripts/e2e/swapBridgeViaStargateNative.tsscripts/e2e/utils/contractTypes.tsscripts/e2e/utils/routerAbi.tssrc/combined/BungeeOpenRouterV2.solsrc/combined/BungeeOpenRouterV2Unchecked.soltest/poc/OneInchCctpOpenRouterPoC.t.sol
🚧 Files skipped from review as they are similar to previous changes (6)
- scripts/e2e/utils/routerAbi.ts
- scripts/e2e/utils/contractTypes.ts
- scripts/e2e/swapBridgeViaCctp.ts
- scripts/e2e/swapBridgeViaStargateNative.ts
- scripts/e2e/swapBridgeViaArbitrumNative.ts
- src/combined/BungeeOpenRouterV2.sol
Comment on lines
+51
to
+63
| function test_oneInchSwapCctpBridge_polygonFork() public { | ||
| string memory rpcUrl = vm.envOr("POLYGON_RPC", string("")); | ||
| if (bytes(rpcUrl).length != 0) { | ||
| uint256 forkBlock = vm.envOr("POLYGON_FORK_BLOCK", FORK_BLOCK_NUMBER); | ||
| vm.createSelectFork(rpcUrl, forkBlock); | ||
| vm.warp(FORK_BLOCK_TIMESTAMP); | ||
| } | ||
|
|
||
| Router router = _routerAtFixtureAddress(); | ||
| if (bytes(rpcUrl).length == 0) { | ||
| emit log("Set POLYGON_RPC to execute this fork PoC."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Do not let these test_* methods succeed as no-ops.
A missing POLYGON_RPC currently turns all three cases into green exits without exercising the flow at all. This should be moved into shared setUp() initialization and handled as an explicit test precondition instead of a plain return, otherwise this file can silently stop testing anything.
As per coding guidelines "Proper use of setUp() for test initialization."
Also applies to: 96-108, 140-150
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/poc/OneInchCctpOpenRouterPoC.t.sol` around lines 51 - 63, Move the
POLYGON_RPC existence check into the shared setUp() so tests are not silently
no-ops; in setUp() read vm.envOr("POLYGON_RPC", ""),
vm.envOr("POLYGON_FORK_BLOCK", ...), createSelectFork and vm.warp there, and
require(bytes(rpcUrl).length != 0, "POLYGON_RPC not set") (or otherwise fail the
setup) so missing env causes a failing precondition instead of early returns.
Remove the early-return guards from test_oneInchSwapCctpBridge_polygonFork and
the other test_* cases (the ones at the other ranges mentioned) so they rely on
the shared setUp precondition and exercise the flows.
Comment on lines
+51
to
+179
| function test_oneInchSwapCctpBridge_polygonFork() public { | ||
| string memory rpcUrl = vm.envOr("POLYGON_RPC", string("")); | ||
| if (bytes(rpcUrl).length != 0) { | ||
| uint256 forkBlock = vm.envOr("POLYGON_FORK_BLOCK", FORK_BLOCK_NUMBER); | ||
| vm.createSelectFork(rpcUrl, forkBlock); | ||
| vm.warp(FORK_BLOCK_TIMESTAMP); | ||
| } | ||
|
|
||
| Router router = _routerAtFixtureAddress(); | ||
| if (bytes(rpcUrl).length == 0) { | ||
| emit log("Set POLYGON_RPC to execute this fork PoC."); | ||
| return; | ||
| } | ||
|
|
||
| uint256 inputAmount = vm.envOr("POC_AAVE_AMOUNT", SWAP_INPUT_AAVE); | ||
|
|
||
| deal(POLYGON_AAVE, FIXTURE_RECIPIENT, inputAmount); | ||
| deal(POLYGON_AAVE, address(router), 0); | ||
| deal(POLYGON_USDC, address(router), 0); | ||
|
|
||
| vm.prank(FIXTURE_RECIPIENT); | ||
| ERC20(POLYGON_AAVE).approve(address(ALLOWANCE_HOLDER), inputAmount); | ||
|
|
||
| uint256 feeRecipientUsdcBefore = ERC20(POLYGON_USDC).balanceOf(FEE_RECIPIENT); | ||
| uint256 usdcSupplyBefore = ERC20(POLYGON_USDC).totalSupply(); | ||
|
|
||
| Router.Action[] memory actions = _buildActions(inputAmount, vm.parseBytes(ONEINCH_SWAP_CALLDATA)); | ||
|
|
||
| bytes memory ahResult; | ||
| vm.prank(FIXTURE_RECIPIENT); | ||
| uint256 gasBeforeExecute = gasleft(); | ||
| ahResult = IAllowanceHolder(address(ALLOWANCE_HOLDER)).exec( | ||
| address(router), | ||
| POLYGON_AAVE, | ||
| inputAmount, | ||
| payable(address(router)), | ||
| abi.encodeCall(router.performModularExecution, (actions)) | ||
| ); | ||
| uint256 executeGasUsed = gasBeforeExecute - gasleft(); | ||
| emit log_named_uint("AllowanceHolder.exec -> router.performModularExecution gas used", executeGasUsed); | ||
|
|
||
| bytes[] memory results = abi.decode(ahResult, (bytes[])); | ||
| _assertPocResult(router, feeRecipientUsdcBefore, usdcSupplyBefore, results[2], results[5]); | ||
| } | ||
|
|
||
| function test_oneInchSwapCctpBridgeMonolithic_polygonFork() public { | ||
| string memory rpcUrl = vm.envOr("POLYGON_RPC", string("")); | ||
| if (bytes(rpcUrl).length != 0) { | ||
| uint256 forkBlock = vm.envOr("POLYGON_FORK_BLOCK", FORK_BLOCK_NUMBER); | ||
| vm.createSelectFork(rpcUrl, forkBlock); | ||
| vm.warp(FORK_BLOCK_TIMESTAMP); | ||
| } | ||
|
|
||
| Router router = _routerAtFixtureAddress(); | ||
| if (bytes(rpcUrl).length == 0) { | ||
| emit log("Set POLYGON_RPC to execute this fork PoC."); | ||
| return; | ||
| } | ||
|
|
||
| uint256 inputAmount = vm.envOr("POC_AAVE_AMOUNT", SWAP_INPUT_AAVE); | ||
|
|
||
| deal(POLYGON_AAVE, FIXTURE_RECIPIENT, inputAmount); | ||
| deal(POLYGON_AAVE, address(router), 0); | ||
| deal(POLYGON_USDC, address(router), 0); | ||
|
|
||
| vm.prank(FIXTURE_RECIPIENT); | ||
| ERC20(POLYGON_AAVE).approve(address(ALLOWANCE_HOLDER), inputAmount); | ||
|
|
||
| uint256 feeRecipientUsdcBefore = ERC20(POLYGON_USDC).balanceOf(FEE_RECIPIENT); | ||
| uint256 usdcSupplyBefore = ERC20(POLYGON_USDC).totalSupply(); | ||
|
|
||
| Router.MonolithicExecution memory exec = | ||
| _buildMonolithicExecution(inputAmount, vm.parseBytes(ONEINCH_SWAP_CALLDATA)); | ||
|
|
||
| vm.prank(FIXTURE_RECIPIENT); | ||
| uint256 gasBeforeExecute = gasleft(); | ||
| IAllowanceHolder(address(ALLOWANCE_HOLDER)).exec( | ||
| address(router), | ||
| POLYGON_AAVE, | ||
| inputAmount, | ||
| payable(address(router)), | ||
| abi.encodeCall(router.performExecution, (exec)) | ||
| ); | ||
| uint256 executeGasUsed = gasBeforeExecute - gasleft(); | ||
| emit log_named_uint("AllowanceHolder.exec -> router.performExecution gas used", executeGasUsed); | ||
|
|
||
| _assertMonolithicPocResult(router, feeRecipientUsdcBefore, usdcSupplyBefore); | ||
| } | ||
|
|
||
| function test_oneInchSwapCctpBridgeSocketGatewayReference_polygonFork() public { | ||
| string memory rpcUrl = vm.envOr("POLYGON_RPC", string("")); | ||
| if (bytes(rpcUrl).length != 0) { | ||
| uint256 forkBlock = vm.envOr("POLYGON_FORK_BLOCK", FORK_BLOCK_NUMBER); | ||
| vm.createSelectFork(rpcUrl, forkBlock); | ||
| vm.warp(FORK_BLOCK_TIMESTAMP); | ||
| } | ||
|
|
||
| if (bytes(rpcUrl).length == 0) { | ||
| emit log("Set POLYGON_RPC to execute this fork PoC."); | ||
| return; | ||
| } | ||
|
|
||
| uint256 inputAmount = vm.envOr("POC_AAVE_AMOUNT", SWAP_INPUT_AAVE); | ||
| assertEq(inputAmount, SWAP_INPUT_AAVE, "reference gateway calldata fixes the input amount"); | ||
|
|
||
| deal(POLYGON_AAVE, FIXTURE_RECIPIENT, inputAmount); | ||
| deal(POLYGON_AAVE, FIXTURE_ROUTER, 0); | ||
| deal(POLYGON_USDC, FIXTURE_ROUTER, 0); | ||
|
|
||
| vm.prank(FIXTURE_RECIPIENT); | ||
| ERC20(POLYGON_AAVE).approve(FIXTURE_ROUTER, REFERENCE_GATEWAY_INITIAL_AAVE_ALLOWANCE); | ||
|
|
||
| uint256 feeRecipientUsdcBefore = ERC20(POLYGON_USDC).balanceOf(FEE_RECIPIENT); | ||
| uint256 usdcSupplyBefore = ERC20(POLYGON_USDC).totalSupply(); | ||
|
|
||
| bytes memory referenceCalldata = _referenceSocketGatewayCalldata(); | ||
|
|
||
| vm.prank(FIXTURE_RECIPIENT, FIXTURE_RECIPIENT); | ||
| uint256 gasBeforeExecute = gasleft(); | ||
| (bool success, bytes memory returndata) = FIXTURE_ROUTER.call(referenceCalldata); | ||
| uint256 executeGasUsed = gasBeforeExecute - gasleft(); | ||
| if (!success) { | ||
| assembly ("memory-safe") { | ||
| revert(add(returndata, 0x20), mload(returndata)) | ||
| } | ||
| } | ||
| emit log_named_uint("SocketGateway reference call gas used", executeGasUsed); | ||
|
|
||
| _assertSocketGatewayPocResult(feeRecipientUsdcBefore, usdcSupplyBefore); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Add failure and boundary coverage for these new router flows.
This file only proves one known-good Polygon path. It never checks the revert behavior that matters for this router surface—bad splice indices, minOutput misses, missing AllowanceHolder context, or zero/boundary amounts—so regressions in the custom-error paths will slip through.
As per coding guidelines "Tests cover both happy path and failure scenarios. Edge cases tested - zero values, maximum values, boundary conditions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/poc/OneInchCctpOpenRouterPoC.t.sol` around lines 51 - 179, Add negative
and boundary tests alongside the existing happy-path PoCs for the three flows
(test_oneInchSwapCctpBridge_polygonFork,
test_oneInchSwapCctpBridgeMonolithic_polygonFork,
test_oneInchSwapCctpBridgeSocketGatewayReference_polygonFork): create additional
test cases that call
_buildActions/_buildMonolithicExecution/_referenceSocketGatewayCalldata with bad
splice indices, minOutput set above realistic return (to force revert), zero and
extremely large input amounts, and calls made without the AllowanceHolder
context; for each add expectations that the router methods
(router.performModularExecution, router.performExecution, and the SocketGateway
reference call) revert with the correct custom errors or emit expected failure
events, and assert balances/totalSupply unchanged on revert (using
IAllowanceHolder.exec and direct call patterns shown) to cover boundary and
failure behavior referenced in the review.
Comment on lines
+54
to
+56
| uint256 forkBlock = vm.envOr("POLYGON_FORK_BLOCK", FORK_BLOCK_NUMBER); | ||
| vm.createSelectFork(rpcUrl, forkBlock); | ||
| vm.warp(FORK_BLOCK_TIMESTAMP); |
There was a problem hiding this comment.
Do not pin block.timestamp to a different block than the selected fork.
These tests let POLYGON_FORK_BLOCK vary, but they always warp to FORK_BLOCK_TIMESTAMP. That creates an inconsistent fork whenever the block override is used, which can skew time-sensitive 1inch/CCTP behavior. This should either skip the warp for override runs or derive the timestamp from the selected fork.
Also applies to: 99-101, 143-145
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/poc/OneInchCctpOpenRouterPoC.t.sol` around lines 54 - 56, The tests
currently call vm.createSelectFork(rpcUrl, forkBlock) then unconditionally
vm.warp(FORK_BLOCK_TIMESTAMP), which causes a timestamp/fork mismatch when
POLYGON_FORK_BLOCK is overridden; update the setup so after computing forkBlock
(via vm.envOr("POLYGON_FORK_BLOCK", FORK_BLOCK_NUMBER)) you either (a) skip
vm.warp when an override was supplied (i.e., when forkBlock !=
FORK_BLOCK_NUMBER) or (b) derive the correct timestamp for the selected
forkBlock and pass that to vm.warp (fetch the block by number from the RPC and
parse its timestamp) instead of always using FORK_BLOCK_TIMESTAMP; change the
code around vm.createSelectFork and vm.warp accordingly (referencing forkBlock,
FORK_BLOCK_NUMBER, FORK_BLOCK_TIMESTAMP, vm.createSelectFork and vm.warp).
- txn log summary - changed most scripts src chain to Polygon for better gas comparison - added three cases in stargate native - added new usdt0 oft tests
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
scripts/e2e/utils/relayLinkQuote.ts (2)
66-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the quote shape before indexing into
steps.This reads
steps[0],steps[1], anditems[0]unconditionally. A truncated or error-shaped Relay response turns into an opaqueTypeErrorinstead of a clear failure.Proposed fix
export function parseRelayQuote(quote: RelayQuoteResponse): { relaySpender: string; depositTarget: string; depositData: string; } { + if (quote.steps.length < 2) { + throw new Error(`Relay quote missing expected steps: got ${quote.steps.length}`); + } + const approveIface = new ethers.Interface([ 'function approve(address spender, uint256 amount) external returns (bool)', ]); - const approveStep = quote.steps[0]; - const approveDataHex = approveStep.items[0].data.data ?? ''; + const approveDataHex = quote.steps[0]?.items?.[0]?.data?.data; + if (!approveDataHex) { + throw new Error('Relay quote missing approve calldata'); + } let relaySpender: string; try { relaySpender = ethers.getAddress( approveIface.decodeFunctionData('approve', approveDataHex)[0], ); @@ - const depositStep = quote.steps[1]; - const depositItem = depositStep.items[0].data; - const depositTarget = depositItem.to ?? ''; + const depositItem = quote.steps[1]?.items?.[0]?.data; + if (!depositItem?.to) { + throw new Error('Relay quote missing deposit target'); + } + const depositTarget = depositItem.to; const depositData = depositItem.data ?? '0x';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/e2e/utils/relayLinkQuote.ts` around lines 66 - 85, Before accessing quote.steps and nested items, validate the shape and length: ensure quote && Array.isArray(quote.steps) && quote.steps.length >= 2, and that quote.steps[0].items and quote.steps[1].items are arrays with at least one element; if any check fails, throw a clear Error like "Invalid relay quote: missing steps/items" referencing the quote variable. Apply these guards before computing approveStep, approveDataHex, depositStep, depositItem, depositTarget and depositData so you never index into undefined and can provide a meaningful failure message.
49-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet a timeout on the Relay quote request.
This helper fronts every Relay E2E path. Without a timeout, a slow Relay response can hang the script indefinitely.
Proposed fix
const response = await axios.post<RelayQuoteResponse>( 'https://api.relay.link/quote/v2', body, - { headers }, + { headers, timeout: 30_000 }, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/e2e/utils/relayLinkQuote.ts` around lines 49 - 53, The axios.post call that fetches the Relay quote (await axios.post<RelayQuoteResponse>(...)) lacks a request timeout and can hang; add a timeout option to the third argument (where headers are passed) — e.g., include timeout: <ms> alongside headers so the request fails fast on slow Relay responses and ensure any calling code that awaits the response can handle the timeout error thrown by axios.scripts/e2e/swapBridgeViaCctp.ts (1)
81-82:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet a timeout on the OpenOcean quote call.
This external request currently has no deadline, so the CCTP E2E can hang indefinitely instead of failing fast.
Proposed fix
- const response = await axios.get<OpenOceanSwapQuoteResponse>(url, { params }); + const response = await axios.get<OpenOceanSwapQuoteResponse>(url, { + params, + timeout: 30_000, + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/e2e/swapBridgeViaCctp.ts` around lines 81 - 82, Add a timeout to the external OpenOcean quote request so the E2E won't hang: update the axios.get call that fetches the OpenOcean swap quote (the line creating "url" and the "response = await axios.get<OpenOceanSwapQuoteResponse>(...)" in scripts/e2e/swapBridgeViaCctp.ts) to include a reasonable timeout option (e.g. timeout in milliseconds) in the request config object (alongside params) so the call fails fast on network stalls.scripts/e2e/swapBridgeViaStargateNative.ts (1)
267-269:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet a timeout on the OpenOcean quote call.
No timeout here means a slow OpenOcean response can wedge the whole E2E run.
Proposed fix
- const response = await axios.get<OoQuoteResponse>(url, { params }); + const response = await axios.get<OoQuoteResponse>(url, { + params, + timeout: 30_000, + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/e2e/swapBridgeViaStargateNative.ts` around lines 267 - 269, The OpenOcean quote request using axios.get (variables: url, params, response, q) has no timeout and can hang the E2E; add a request timeout (e.g., 5000–15000 ms) to the axios call options (axios.get<OoQuoteResponse>(url, { params, timeout: 10000 })) or create an axios instance with a default timeout and use it for the call, and handle timeout errors (e.g., catch and surface a clear error) so a slow OpenOcean response cannot stall the test run.hardhat.config.ts (1)
9-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize
DEPLOYER_PRIVATE_KEYbefore buildingaccounts.Line 10 passes the env value straight to Hardhat. A bare 64-hex key will fail during network operations, so this should prepend
0xwhen needed before putting it inaccounts.Proposed fix
-const deployerKey = process.env.DEPLOYER_PRIVATE_KEY; -const accounts = deployerKey ? [deployerKey] : []; +const deployerKey = process.env.DEPLOYER_PRIVATE_KEY?.trim(); +const normalizedDeployerKey = + deployerKey && !deployerKey.startsWith('0x') + ? `0x${deployerKey}` + : deployerKey; +const accounts = normalizedDeployerKey ? [normalizedDeployerKey] : [];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hardhat.config.ts` around lines 9 - 10, The DEPLOYER_PRIVATE_KEY is used raw to build accounts which fails if the key is a bare 64-hex string; update the logic around deployerKey and accounts so you normalize the env value by checking if deployerKey exists and, if so, ensure it has the "0x" prefix (prepend "0x" when missing) before assigning the accounts array; reference the deployerKey variable and the accounts assignment in hardhat.config.ts when making this change.scripts/e2e/config.ts (1)
205-209:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
FEE_AMOUNT_BPSbefore fee math.
Number(...)here acceptsNaN, fractions, negatives, and values above 10000. Those either blow up inBigInt(bps)or produce broken fee calculations across every E2E entrypoint.Proposed fix
-/** Fee applied in scripts that take pre-/post-route fees (basis points). */ -export const FEE_BPS = Number(process.env.FEE_AMOUNT_BPS ?? '10'); +/** Fee applied in scripts that take pre-/post-route fees (basis points). */ +const feeBpsRaw = process.env.FEE_AMOUNT_BPS ?? '10'; +const feeBps = Number(feeBpsRaw); +if (!Number.isInteger(feeBps) || feeBps < 0 || feeBps > 10_000) { + throw new Error(`Invalid FEE_AMOUNT_BPS: ${feeBpsRaw}`); +} +export const FEE_BPS = feeBps;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/e2e/config.ts` around lines 205 - 209, The FEE_BPS initialization accepts invalid inputs (NaN, fractions, negatives, >10000) which breaks fee math; change the initialization so FEE_BPS is derived using parseInt on process.env.FEE_AMOUNT_BPS, validate Number.isInteger(value) and that 0 <= value <= 10000, and either throw a clear Error on invalid input or fall back to a safe default; ensure bpsOf(amount: bigint, bps: number) continues to use a validated integer bps (and document that callers must pass the validated FEE_BPS) so BigInt(bps) never receives invalid values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/e2e/config.ts`:
- Around line 49-64: The function routerAddressForChain currently ignores
malformed overrides and falls back to ROUTER_BY_CHAIN_ID; change it to fail fast
by validating process.env[`ROUTER_CHAIN_${chainId}`] and
process.env.ROUTER_ADDRESS with ADDR_HEX_RE and throwing an Error if either is
present but invalid. Specifically, in routerAddressForChain: if envSpecific is
defined but fails ADDR_HEX_RE, throw a descriptive error referencing
ROUTER_CHAIN_<chainId>; similarly if legacy is defined but fails ADDR_HEX_RE,
throw an error referencing ROUTER_ADDRESS; only use ROUTER_BY_CHAIN_ID when no
override env var is set at all.
In `@scripts/e2e/swapBridgeViaOft.ts`:
- Around line 759-779: The script currently only handles 'usdt0-direct' and
'aave-usdt0-oft' values for caseArg, causing the documented 'all' and the no-arg
default to fail; update the main argument handling so that when caseArg ===
'all' or when caseArg is undefined/null/empty it sequentially calls
runCase1(signer, signerAddress, routerIface) then runCase2(signer,
signerAddress, routerIface), printing the existing completion messages for each
case and then returning; keep the existing single-case branches for
'aave-usdt0-oft' and 'usdt0-direct' unchanged and preserve the final
unknown-case error for other values.
- Around line 137-138: The axios GET to OpenOcean (variable url, using
axios.get<OoSwapQuoteResponse>(url, { params })) lacks a timeout and can hang;
update the request to include a timeout (e.g. timeout: 5000 or 10000 ms) in the
config object passed to axios.get and ensure the call is wrapped in try/catch to
surface a clear error if it times out (log or rethrow with context like
"OpenOcean quote request timed out" referencing url/params).
In `@scripts/e2e/swapBridgeViaStargateNative.ts`:
- Around line 512-516: The code currently subtracts NATIVE_INPUT_GAS_RESERVE
once from walletBalance then splits the remainder for two legs, which can leave
the second leg short; update the logic in runCase (native-input case 4 and the
similar blocks around the other mentions) to either (A) reserve headroom per leg
by subtracting 2 * NATIVE_INPUT_GAS_RESERVE (or better: 2 * (nativeFeeWithBuffer
+ gas buffer)) before splitting, or (B) after executing the first leg re-read
the signer balance (await signer.getBalance()) and recompute the second leg
inputAmount from the live balance; adjust references to walletBalance,
inputAmount, nativeFeeWithBuffer and NATIVE_INPUT_GAS_RESERVE accordingly so
each leg has its own reserved gas+fee headroom.
---
Duplicate comments:
In `@hardhat.config.ts`:
- Around line 9-10: The DEPLOYER_PRIVATE_KEY is used raw to build accounts which
fails if the key is a bare 64-hex string; update the logic around deployerKey
and accounts so you normalize the env value by checking if deployerKey exists
and, if so, ensure it has the "0x" prefix (prepend "0x" when missing) before
assigning the accounts array; reference the deployerKey variable and the
accounts assignment in hardhat.config.ts when making this change.
In `@scripts/e2e/config.ts`:
- Around line 205-209: The FEE_BPS initialization accepts invalid inputs (NaN,
fractions, negatives, >10000) which breaks fee math; change the initialization
so FEE_BPS is derived using parseInt on process.env.FEE_AMOUNT_BPS, validate
Number.isInteger(value) and that 0 <= value <= 10000, and either throw a clear
Error on invalid input or fall back to a safe default; ensure bpsOf(amount:
bigint, bps: number) continues to use a validated integer bps (and document that
callers must pass the validated FEE_BPS) so BigInt(bps) never receives invalid
values.
In `@scripts/e2e/swapBridgeViaCctp.ts`:
- Around line 81-82: Add a timeout to the external OpenOcean quote request so
the E2E won't hang: update the axios.get call that fetches the OpenOcean swap
quote (the line creating "url" and the "response = await
axios.get<OpenOceanSwapQuoteResponse>(...)" in scripts/e2e/swapBridgeViaCctp.ts)
to include a reasonable timeout option (e.g. timeout in milliseconds) in the
request config object (alongside params) so the call fails fast on network
stalls.
In `@scripts/e2e/swapBridgeViaStargateNative.ts`:
- Around line 267-269: The OpenOcean quote request using axios.get (variables:
url, params, response, q) has no timeout and can hang the E2E; add a request
timeout (e.g., 5000–15000 ms) to the axios call options
(axios.get<OoQuoteResponse>(url, { params, timeout: 10000 })) or create an axios
instance with a default timeout and use it for the call, and handle timeout
errors (e.g., catch and surface a clear error) so a slow OpenOcean response
cannot stall the test run.
In `@scripts/e2e/utils/relayLinkQuote.ts`:
- Around line 66-85: Before accessing quote.steps and nested items, validate the
shape and length: ensure quote && Array.isArray(quote.steps) &&
quote.steps.length >= 2, and that quote.steps[0].items and quote.steps[1].items
are arrays with at least one element; if any check fails, throw a clear Error
like "Invalid relay quote: missing steps/items" referencing the quote variable.
Apply these guards before computing approveStep, approveDataHex, depositStep,
depositItem, depositTarget and depositData so you never index into undefined and
can provide a meaningful failure message.
- Around line 49-53: The axios.post call that fetches the Relay quote (await
axios.post<RelayQuoteResponse>(...)) lacks a request timeout and can hang; add a
timeout option to the third argument (where headers are passed) — e.g., include
timeout: <ms> alongside headers so the request fails fast on slow Relay
responses and ensure any calling code that awaits the response can handle the
timeout error thrown by axios.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 76591667-2db3-453c-92d8-8141bf86ac70
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
hardhat.config.tspackage.jsonscripts/e2e/bridgeViaRelay.tsscripts/e2e/config.tsscripts/e2e/swapBridgeViaArbitrumNative.tsscripts/e2e/swapBridgeViaCctp.tsscripts/e2e/swapBridgeViaOft.tsscripts/e2e/swapBridgeViaStargateNative.tsscripts/e2e/utils/relayLinkQuote.tsscripts/e2e/utils/sleep.tsscripts/e2e/utils/txnLogSummary.ts
✅ Files skipped from review due to trivial changes (2)
- scripts/e2e/utils/txnLogSummary.ts
- scripts/e2e/utils/sleep.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- scripts/e2e/swapBridgeViaArbitrumNative.ts
Comment on lines
+49
to
+64
| export function routerAddressForChain(chainId: number): string { | ||
| const envSpecific = process.env[`ROUTER_CHAIN_${chainId}`]?.trim(); | ||
| if (envSpecific && ADDR_HEX_RE.test(envSpecific)) { | ||
| return envSpecific; | ||
| } | ||
| const mapped = ROUTER_BY_CHAIN_ID[chainId]; | ||
| if (mapped) { | ||
| return mapped; | ||
| } | ||
| const legacy = process.env.ROUTER_ADDRESS?.trim(); | ||
| if (legacy && ADDR_HEX_RE.test(legacy)) { | ||
| return legacy; | ||
| } | ||
| throw new Error( | ||
| `routerAddressForChain(${chainId}): no ROUTER_BY_CHAIN_ID entry and neither ROUTER_CHAIN_${chainId} nor ROUTER_ADDRESS is set.`, | ||
| ); |
There was a problem hiding this comment.
Fail fast on malformed router overrides.
If ROUTER_CHAIN_<chainId> or ROUTER_ADDRESS is set but malformed, this silently falls back to the baked-in address and can run the script against the wrong deployment. This should throw instead of ignoring the bad override.
Proposed fix
export function routerAddressForChain(chainId: number): string {
const envSpecific = process.env[`ROUTER_CHAIN_${chainId}`]?.trim();
- if (envSpecific && ADDR_HEX_RE.test(envSpecific)) {
- return envSpecific;
+ if (envSpecific) {
+ if (!ADDR_HEX_RE.test(envSpecific)) {
+ throw new Error(`Invalid ROUTER_CHAIN_${chainId}: ${envSpecific}`);
+ }
+ return envSpecific;
}
const mapped = ROUTER_BY_CHAIN_ID[chainId];
if (mapped) {
return mapped;
}
const legacy = process.env.ROUTER_ADDRESS?.trim();
- if (legacy && ADDR_HEX_RE.test(legacy)) {
- return legacy;
+ if (legacy) {
+ if (!ADDR_HEX_RE.test(legacy)) {
+ throw new Error(`Invalid ROUTER_ADDRESS: ${legacy}`);
+ }
+ return legacy;
}
throw new Error(
`routerAddressForChain(${chainId}): no ROUTER_BY_CHAIN_ID entry and neither ROUTER_CHAIN_${chainId} nor ROUTER_ADDRESS is set.`,
);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/e2e/config.ts` around lines 49 - 64, The function
routerAddressForChain currently ignores malformed overrides and falls back to
ROUTER_BY_CHAIN_ID; change it to fail fast by validating
process.env[`ROUTER_CHAIN_${chainId}`] and process.env.ROUTER_ADDRESS with
ADDR_HEX_RE and throwing an Error if either is present but invalid.
Specifically, in routerAddressForChain: if envSpecific is defined but fails
ADDR_HEX_RE, throw a descriptive error referencing ROUTER_CHAIN_<chainId>;
similarly if legacy is defined but fails ADDR_HEX_RE, throw an error referencing
ROUTER_ADDRESS; only use ROUTER_BY_CHAIN_ID when no override env var is set at
all.
Comment on lines
+137
to
+138
| const url = `https://open-api.openocean.finance/v3/${CHAIN_IDS.POLYGON}/swap_quote`; | ||
| const response = await axios.get<OoSwapQuoteResponse>(url, { params }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file
fd -t f "swapBridgeViaOft.ts" --exec head -n 150 {} \;Repository: SocketDotTech/poc-openrouter
Length of output: 6478
🏁 Script executed:
# Get file size to determine how much to read
wc -l scripts/e2e/swapBridgeViaOft.tsRepository: SocketDotTech/poc-openrouter
Length of output: 108
🏁 Script executed:
# Read the specific section around lines 137-145
sed -n '130,150p' scripts/e2e/swapBridgeViaOft.tsRepository: SocketDotTech/poc-openrouter
Length of output: 643
🏁 Script executed:
# Check for any axios configuration or timeout patterns in the file
rg -A 3 -B 3 "timeout|axios" scripts/e2e/swapBridgeViaOft.tsRepository: SocketDotTech/poc-openrouter
Length of output: 614
🏁 Script executed:
# Search for global axios configuration in the codebase
fd -t f "\.ts$" -o "\.js$" | xargs rg "axios\.(defaults|interceptors)" 2>/dev/null | head -20Repository: SocketDotTech/poc-openrouter
Length of output: 202
🏁 Script executed:
# Search for axios defaults or global timeout configuration
rg "axios\." scripts/ | grep -E "(defaults|interceptor|timeout)" | head -20Repository: SocketDotTech/poc-openrouter
Length of output: 54
🏁 Script executed:
# Check where fetchOpenOceanQuote is called
rg "fetchOpenOceanQuote" scripts/e2e/swapBridgeViaOft.ts -A 2 -B 2Repository: SocketDotTech/poc-openrouter
Length of output: 479
🏁 Script executed:
# Look for test runner or any error handling around axios calls
rg "try|catch|timeout" scripts/e2e/swapBridgeViaOft.ts | head -20Repository: SocketDotTech/poc-openrouter
Length of output: 96
🏁 Script executed:
# Check if there's package.json and what test setup exists
cat package.json | grep -A 10 -E "(jest|mocha|timeout|test)" || echo "Checking structure..."Repository: SocketDotTech/poc-openrouter
Length of output: 94
🏁 Script executed:
# Look at how the script is intended to be run and its main execution
sed -n '750,785p' scripts/e2e/swapBridgeViaOft.tsRepository: SocketDotTech/poc-openrouter
Length of output: 998
Add timeout to the OpenOcean quote API call.
This external API request hangs indefinitely without a timeout, which stalls the entire E2E runner instead of failing fast. In a test pipeline context, this breaks CI/CD execution.
Proposed fix
- const response = await axios.get<OoSwapQuoteResponse>(url, { params });
+ const response = await axios.get<OoSwapQuoteResponse>(url, {
+ params,
+ timeout: 30_000,
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const url = `https://open-api.openocean.finance/v3/${CHAIN_IDS.POLYGON}/swap_quote`; | |
| const response = await axios.get<OoSwapQuoteResponse>(url, { params }); | |
| const url = `https://open-api.openocean.finance/v3/${CHAIN_IDS.POLYGON}/swap_quote`; | |
| const response = await axios.get<OoSwapQuoteResponse>(url, { | |
| params, | |
| timeout: 30_000, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/e2e/swapBridgeViaOft.ts` around lines 137 - 138, The axios GET to
OpenOcean (variable url, using axios.get<OoSwapQuoteResponse>(url, { params }))
lacks a timeout and can hang; update the request to include a timeout (e.g.
timeout: 5000 or 10000 ms) in the config object passed to axios.get and ensure
the call is wrapped in try/catch to surface a clear error if it times out (log
or rethrow with context like "OpenOcean quote request timed out" referencing
url/params).
Comment on lines
+759
to
+779
| const caseArg = process.argv[2]?.toLowerCase(); | ||
|
|
||
| if (caseArg === 'usdt0-direct') { | ||
| await runCase2(signer, signerAddress, routerIface); | ||
| console.log( | ||
| '\nCase 2 complete — USDT0 arrives on Arbitrum once LZ delivers the message.', | ||
| ); | ||
| return; | ||
| } | ||
| if (caseArg === 'aave-usdt0-oft') { | ||
| await runCase1(signer, signerAddress, routerIface); | ||
| console.log( | ||
| '\nCase 1 complete — USDT0 arrives on Arbitrum once LZ delivers the message.', | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| console.error( | ||
| `Unknown case: ${caseArg}. Use: all | aave-usdt0-oft | usdt0-direct`, | ||
| ); | ||
| process.exit(1); |
There was a problem hiding this comment.
Implement the documented all path.
Lines 761-779 only handle the two single-case flags, so ts-node scripts/e2e/swapBridgeViaOft.ts all always exits as unknown even though the docstring advertises it. This should run both cases, and ideally make no arg behave the same way.
Proposed fix
const caseArg = process.argv[2]?.toLowerCase();
+ if (!caseArg || caseArg === 'all') {
+ await runCase1(signer, signerAddress, routerIface);
+ console.log('\nSleeping 3s before case 2...');
+ await sleep(3000);
+ await runCase2(signer, signerAddress, routerIface);
+ return;
+ }
+
if (caseArg === 'usdt0-direct') {
await runCase2(signer, signerAddress, routerIface);
console.log(
'\nCase 2 complete — USDT0 arrives on Arbitrum once LZ delivers the message.',
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/e2e/swapBridgeViaOft.ts` around lines 759 - 779, The script currently
only handles 'usdt0-direct' and 'aave-usdt0-oft' values for caseArg, causing the
documented 'all' and the no-arg default to fail; update the main argument
handling so that when caseArg === 'all' or when caseArg is undefined/null/empty
it sequentially calls runCase1(signer, signerAddress, routerIface) then
runCase2(signer, signerAddress, routerIface), printing the existing completion
messages for each case and then returning; keep the existing single-case
branches for 'aave-usdt0-oft' and 'usdt0-direct' unchanged and preserve the
final unknown-case error for other values.
# Conflicts: # src/minimal/BungeeOpenRouterMinimalAH.sol # src/modular/BungeeOpenRouterModularAH.sol
refactor: check and set max approval
sebastiantf
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit