feat: added calldata executor and bridge receiver#7
Conversation
WalkthroughAdds BungeeReceiver and CalldataExecutor plus interfaces and a safe-call library; enforces chain-bound signed DestPayload execution with a one-time nonce bitmap, transfers bridged funds, dispatches IBungeeExecutor calls via CalldataExecutor, and provides CREATE3 deterministic deployment tooling and Foundry tests. ChangesDestination-Side Execution & Deployment
Sequence DiagramsequenceDiagram
participant Sender
participant BungeeReceiver
participant CalldataExecutor
participant BungeeExecutor
Sender->>BungeeReceiver: executeDestPayload(payload, executionCallData, signature)
BungeeReceiver->>BungeeReceiver: verify signature & use nonce
BungeeReceiver->>CalldataExecutor: executeCalldata(abi.encodeCall(IBungeeExecutor.executeData,...), gasLimit)
CalldataExecutor->>BungeeExecutor: low-level call executeData(quoteId, amount, token, callData)
CalldataExecutor-->>BungeeReceiver: return success
BungeeReceiver-->>Sender: emit DestPayloadExecuted(quoteId, success)
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/deploy/checkReceiverDeployment.ts`:
- Around line 69-72: The check currently only logs deployment info for
CalldataExecutor; add a strict equality check between
executorStatus.bungeeReceiver and the computed receiverAddress and, if they
differ, set hasError = true and emit a clear error/log indicating the mismatch
(include executorAddress, receiverAddress and executorStatus.bungeeReceiver for
context). Locate the block that prints the console.log for "CalldataExecutor
deployed ..." and insert the comparison and error handling there so the script
fails the check when the BUNGEE_RECEIVER wiring is not deterministic.
In `@scripts/deploy/create3.ts`:
- Around line 124-146: Update getBungeeReceiverDeploymentStatus to verify the
contract is actually a BungeeReceiver by calling one or more
BungeeReceiver-specific view methods (e.g., CALLDATA_EXECUTOR() and/or
SOLVER_SIGNER()) in addition to owner(); after confirming bytecode exists,
instantiate the Contract as you do now, call owner() and then call
CALLDATA_EXECUTOR() and/or SOLVER_SIGNER(), treat the deployment as valid only
if those Bungee-specific calls succeed and return plausible values
(addresses/non-zero), and propagate or handle errors so that exceptions from
unrelated Ownable contracts do not yield a false positive; keep the existing
return shape { address, deployed: boolean, owner?: string } and only set
deployed=true when the Bungee-specific probes succeed.
In `@scripts/deploy/deployReceiverAndExecutor.ts`:
- Around line 74-78: When executorStatus.deployed is true, assert that the
deployed executor is wired to the expected receiver by checking
executorStatus.bungeeReceiver === receiverAddress before skipping deployment; if
the check fails log an error (including executorAddress, expected
receiverAddress and actual executorStatus.bungeeReceiver) and either throw or
force redeploy. Update the branch around executorStatus.deployed (the block that
currently logs "CalldataExecutor already deployed...") to perform this
validation using executorStatus.bungeeReceiver and receiverAddress and handle
the mismatch deterministically (error/throw or redeploy) instead of silently
accepting it.
In `@src/BungeeReceiver.sol`:
- Around line 152-156: Reject zero addresses when configuring SOLVER_SIGNER and
CALLDATA_EXECUTOR: in the constructor (where SOLVER_SIGNER and CALLDATA_EXECUTOR
are assigned) add require(_solverSigner != address(0) && _calldataExecutor !=
address(0), "invalid-zero-address") so both must be non-zero, and apply the same
check to any setter/initializer that assigns these state vars (e.g., functions
that set SOLVER_SIGNER or CALLDATA_EXECUTOR such as
setSolverSigner/setCalldataExecutor or initialize) to prevent assigning
address(0).
- Around line 282-285: The unchecked subtraction of feeData.amount from
outputAmount in the POST_SWAP branch can wrap; update the block in the function
handling fee deduction (the code referencing feeData, FeeType.POST_SWAP,
CurrencyLib.transfer, and outputAmount) to first require(feeData.amount <=
outputAmount) with a clear revert/error message, then perform the transfer and
subtraction (remove or keep unchecked after the guard). This ensures any
misconfigured fee causes an immediate, explicit revert rather than a silent
wrap.
In `@src/CalldataExecutor.sol`:
- Around line 25-27: The constructor currently assigns BUNGEE_RECEIVER without
validation, allowing address(0) which would break executeCalldata access
control; update the constructor to require(_bungeeReceiver != address(0)) (or
revert with a clear error) before setting BUNGEE_RECEIVER so any zero address
passed causes deployment to revert, referencing the constructor and the
BUNGEE_RECEIVER state used by executeCalldata.
- Around line 30-36: In executeCalldata, reject EOAs/no-code targets before
calling to.excessivelySafeCall by checking to.code.length > 0 and reverting if
zero; add a guard (e.g., revert TargetNotContract() or require(to.code.length >
0, "target not contract")) immediately after the OnlyBungeeReceiver check and
before calling excessivelySafeCall so the call cannot return success for an
address with no contract code (referencing executeCalldata, BUNGEE_RECEIVER,
OnlyBungeeReceiver, excessivelySafeCall, and MAX_COPY_BYTES).
🪄 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: 5063be1d-6f67-4eb6-8da0-42f34351ae3e
📒 Files selected for processing (8)
scripts/deploy/checkReceiverDeployment.tsscripts/deploy/create3.tsscripts/deploy/deployReceiverAndExecutor.tssrc/BungeeReceiver.solsrc/CalldataExecutor.solsrc/interfaces/IBungeeExecutor.solsrc/interfaces/ICalldataExecutor.solsrc/lib/ExcessivelySafeCall.sol
| export async function getBungeeReceiverDeploymentStatus(params: { | ||
| provider: Provider; | ||
| address: string; | ||
| }): Promise<{ address: string; deployed: boolean; owner?: string }> { | ||
| const { provider, address } = params; | ||
| const bytecode = await provider.getCode(address); | ||
|
|
||
| if (!hasContractBytecode(bytecode)) { | ||
| return { address, deployed: false }; | ||
| } | ||
|
|
||
| try { | ||
| const contract = new Contract( | ||
| address, | ||
| ['function owner() view returns (address)'], | ||
| provider, | ||
| ); | ||
| const owner = (await contract.owner()) as string; | ||
| return { address, deployed: true, owner }; | ||
| } catch { | ||
| return { address, deployed: false }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Strengthen getBungeeReceiverDeploymentStatus contract identity check.
Current logic marks deployment as valid if owner() responds. That can false-positive on unrelated Ownable contracts. This should also probe BungeeReceiver-specific view(s), e.g. CALLDATA_EXECUTOR() and/or SOLVER_SIGNER().
Suggested direction
export async function getBungeeReceiverDeploymentStatus(params: {
provider: Provider;
address: string;
-}): Promise<{ address: string; deployed: boolean; owner?: string }> {
+}): Promise<{ address: string; deployed: boolean; owner?: string; calldataExecutor?: string }> {
@@
try {
const contract = new Contract(
address,
- ['function owner() view returns (address)'],
+ [
+ 'function owner() view returns (address)',
+ 'function CALLDATA_EXECUTOR() view returns (address)',
+ ],
provider,
);
const owner = (await contract.owner()) as string;
- return { address, deployed: true, owner };
+ const calldataExecutor = (await contract.CALLDATA_EXECUTOR()) as string;
+ return { address, deployed: true, owner, calldataExecutor };
} catch {
return { address, deployed: false };
}
}As per coding guidelines, “Check for resource leaks, race conditions, and unhandled edge cases. Flag over-engineering and premature abstractions.”
📝 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.
| export async function getBungeeReceiverDeploymentStatus(params: { | |
| provider: Provider; | |
| address: string; | |
| }): Promise<{ address: string; deployed: boolean; owner?: string }> { | |
| const { provider, address } = params; | |
| const bytecode = await provider.getCode(address); | |
| if (!hasContractBytecode(bytecode)) { | |
| return { address, deployed: false }; | |
| } | |
| try { | |
| const contract = new Contract( | |
| address, | |
| ['function owner() view returns (address)'], | |
| provider, | |
| ); | |
| const owner = (await contract.owner()) as string; | |
| return { address, deployed: true, owner }; | |
| } catch { | |
| return { address, deployed: false }; | |
| } | |
| } | |
| export async function getBungeeReceiverDeploymentStatus(params: { | |
| provider: Provider; | |
| address: string; | |
| }): Promise<{ address: string; deployed: boolean; owner?: string; calldataExecutor?: string }> { | |
| const { provider, address } = params; | |
| const bytecode = await provider.getCode(address); | |
| if (!hasContractBytecode(bytecode)) { | |
| return { address, deployed: false }; | |
| } | |
| try { | |
| const contract = new Contract( | |
| address, | |
| [ | |
| 'function owner() view returns (address)', | |
| 'function CALLDATA_EXECUTOR() view returns (address)', | |
| ], | |
| provider, | |
| ); | |
| const owner = (await contract.owner()) as string; | |
| const calldataExecutor = (await contract.CALLDATA_EXECUTOR()) as string; | |
| return { address, deployed: true, owner, calldataExecutor }; | |
| } catch { | |
| return { address, deployed: false }; | |
| } | |
| } |
🤖 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/create3.ts` around lines 124 - 146, Update
getBungeeReceiverDeploymentStatus to verify the contract is actually a
BungeeReceiver by calling one or more BungeeReceiver-specific view methods
(e.g., CALLDATA_EXECUTOR() and/or SOLVER_SIGNER()) in addition to owner(); after
confirming bytecode exists, instantiate the Contract as you do now, call owner()
and then call CALLDATA_EXECUTOR() and/or SOLVER_SIGNER(), treat the deployment
as valid only if those Bungee-specific calls succeed and return plausible values
(addresses/non-zero), and propagate or handle errors so that exceptions from
unrelated Ownable contracts do not yield a false positive; keep the existing
return shape { address, deployed: boolean, owner?: string } and only set
deployed=true when the Bungee-specific probes succeed.
| function executeCalldata(address to, bytes memory encodedData, uint256 msgGasLimit) external returns (bool success) { | ||
| if (msg.sender != BUNGEE_RECEIVER) { | ||
| revert OnlyBungeeReceiver(); | ||
| } | ||
|
|
||
| (success,) = to.excessivelySafeCall(msgGasLimit, MAX_COPY_BYTES, encodedData); | ||
| } |
There was a problem hiding this comment.
Reject non-contract targets before low-level execution.
Line 35 can report success=true when to is an EOA/no-code address, which silently skips protocol execution while appearing successful. This should enforce to.code.length > 0.
Suggested fix
error OnlyBungeeReceiver();
+ error InvalidExecutorTarget();
@@
function executeCalldata(address to, bytes memory encodedData, uint256 msgGasLimit) external returns (bool success) {
if (msg.sender != BUNGEE_RECEIVER) {
revert OnlyBungeeReceiver();
}
+ if (to.code.length == 0) revert InvalidExecutorTarget();
(success,) = to.excessivelySafeCall(msgGasLimit, MAX_COPY_BYTES, encodedData);
}📝 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.
| function executeCalldata(address to, bytes memory encodedData, uint256 msgGasLimit) external returns (bool success) { | |
| if (msg.sender != BUNGEE_RECEIVER) { | |
| revert OnlyBungeeReceiver(); | |
| } | |
| (success,) = to.excessivelySafeCall(msgGasLimit, MAX_COPY_BYTES, encodedData); | |
| } | |
| function executeCalldata(address to, bytes memory encodedData, uint256 msgGasLimit) external returns (bool success) { | |
| if (msg.sender != BUNGEE_RECEIVER) { | |
| revert OnlyBungeeReceiver(); | |
| } | |
| if (to.code.length == 0) revert InvalidExecutorTarget(); | |
| (success,) = to.excessivelySafeCall(msgGasLimit, MAX_COPY_BYTES, encodedData); | |
| } |
🤖 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/CalldataExecutor.sol` around lines 30 - 36, In executeCalldata, reject
EOAs/no-code targets before calling to.excessivelySafeCall by checking
to.code.length > 0 and reverting if zero; add a guard (e.g., revert
TargetNotContract() or require(to.code.length > 0, "target not contract"))
immediately after the OnlyBungeeReceiver check and before calling
excessivelySafeCall so the call cannot return success for an address with no
contract code (referencing executeCalldata, BUNGEE_RECEIVER, OnlyBungeeReceiver,
excessivelySafeCall, and MAX_COPY_BYTES).
…outer into feat/dst-payload
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test/e2e/TestBungeeExecutor.sol (1)
4-9: ⚡ Quick winImplement
IBungeeExecutoron the mock.This should explicitly satisfy the same interface as production executors. Otherwise interface drift can leave the test contract compiling while the real execution path has already changed.
Suggested fix
+import {IBungeeExecutor} from "../../src/interfaces/IBungeeExecutor.sol"; import {RescueFundsLib} from "../../src/common/lib/RescueFundsLib.sol"; import {Ownable} from "../../src/common/utils/Ownable.sol"; /// `@notice` Test implementation of IBungeeExecutor. Decodes a uint256 from callData and stores it. /// `@dev` Deploy on destination chain. Use abi.encode(uint256) as executionCallData in scripts. -contract TestBungeeExecutor is Ownable { +contract TestBungeeExecutor is Ownable, IBungeeExecutor { @@ - function executeData(bytes32 quoteId, uint256 amount, address token, bytes calldata callData) external payable { + function executeData(bytes32 quoteId, uint256 amount, address token, bytes calldata callData) external payable override {Also applies to: 21-21
🤖 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/e2e/TestBungeeExecutor.sol` around lines 4 - 9, The TestBungeeExecutor mock currently doesn't declare or implement the IBungeeExecutor interface—update it to explicitly implement IBungeeExecutor by importing IBungeeExecutor and changing the contract declaration to implement that interface (e.g., contract TestBungeeExecutor is Ownable, IBungeeExecutor) and add all required functions from IBungeeExecutor with the exact signatures and override specifiers (implement the executor entrypoint(s) used in production, plus any view/owner methods) so the mock cannot drift from the real executor API; compile and fix any signature/visibility/return mismatches until no interface warnings remain.
🤖 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-11: The three env entries SOLVER_SIGNER_ADDRESS,
OPENROUTER_ADDRESS, and ACROSS_MANIPULATOR_ADDRESS currently have inline
comments after the = which makes dotenv treat them as empty strings; change each
to a bare empty assignment (e.g. SOLVER_SIGNER_ADDRESS=) and move the
explanatory comment to its own line above or below (e.g. # BungeeReceiver deploy
only; defaults to deployer) so the vars are unambiguously empty and
lint-friendly while preserving the existing comment text.
In `@package.json`:
- Around line 12-14: The check:* npm scripts (check:openrouter, check:receiver,
check:across-manipulator) currently call "hardhat run <script>" and will
silently run against the default network; change them to require an explicit
network and fail fast when none is supplied. Replace each script invocation with
a small wrapper that verifies npm_config_network is set (or inspects
process.argv for a --network arg) and exits with an error if missing, then
invokes hardhat run with --network "$npm_config_network" (or passes through the
provided --network). Update the three scripts (check:openrouter, check:receiver,
check:across-manipulator) to use this wrapper approach or call a shared
checkNetwork helper before running hardhat.
In `@scripts/deploy/create3.ts`:
- Around line 119-125: The resolveAcrossManipulatorAddress function currently
falls back silently to ACROSS_MANIPULATOR_EXPECTED_ADDRESS when
process.env.ACROSS_MANIPULATOR_ADDRESS exists but is malformed; change it to
reject malformed overrides by throwing an error: if ACROSS_MANIPULATOR_ADDRESS
is defined and ADDR_HEX_RE.test(envAddress) is false, throw a descriptive Error
(include the invalid env value) so callers know the override is invalid;
otherwise return the validated envAddress or the expected
ACROSS_MANIPULATOR_EXPECTED_ADDRESS as before.
In `@test/e2e/TestBungeeExecutor.sol`:
- Around line 21-23: In executeData, don't silently treat 1–31 byte callData as
zero; instead explicitly handle empty callData as counter=0 and require that
non-empty callData be at least 32 bytes before decoding. Update the executeData
function: if callData.length == 0 set counter = 0; else require(callData.length
>= 32, "Malformed callData"); then abi.decode(callData, (uint256)) into
counterValue and assign counter = counterValue. Reference: executeData and
counter.
---
Nitpick comments:
In `@test/e2e/TestBungeeExecutor.sol`:
- Around line 4-9: The TestBungeeExecutor mock currently doesn't declare or
implement the IBungeeExecutor interface—update it to explicitly implement
IBungeeExecutor by importing IBungeeExecutor and changing the contract
declaration to implement that interface (e.g., contract TestBungeeExecutor is
Ownable, IBungeeExecutor) and add all required functions from IBungeeExecutor
with the exact signatures and override specifiers (implement the executor
entrypoint(s) used in production, plus any view/owner methods) so the mock
cannot drift from the real executor API; compile and fix any
signature/visibility/return mismatches until no interface warnings remain.
🪄 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: 718bce8d-7c59-48bd-b244-18ddd4104613
📒 Files selected for processing (14)
.env.example.gitignorepackage.jsonscripts/deploy/checkReceiverDeployment.tsscripts/deploy/create3.tsscripts/deploy/deployReceiverAndExecutor.tssrc/BungeeReceiver.solsrc/CalldataExecutor.solsrc/OpenRouter.solsrc/interfaces/IBungeeExecutor.solsrc/interfaces/ICalldataExecutor.solsrc/lib/ExcessivelySafeCall.soltest/combined/BungeeReceiverDestPayload.t.soltest/e2e/TestBungeeExecutor.sol
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- src/OpenRouter.sol
🚧 Files skipped from review as they are similar to previous changes (7)
- scripts/deploy/checkReceiverDeployment.ts
- scripts/deploy/deployReceiverAndExecutor.ts
- src/CalldataExecutor.sol
- test/combined/BungeeReceiverDestPayload.t.sol
- src/interfaces/ICalldataExecutor.sol
- src/BungeeReceiver.sol
- src/lib/ExcessivelySafeCall.sol
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test/e2e/TestBungeeExecutor.sol (1)
4-9: ⚡ Quick winImplement
IBungeeExecutoron the mock.This should explicitly satisfy the same interface as production executors. Otherwise interface drift can leave the test contract compiling while the real execution path has already changed.
Suggested fix
+import {IBungeeExecutor} from "../../src/interfaces/IBungeeExecutor.sol"; import {RescueFundsLib} from "../../src/common/lib/RescueFundsLib.sol"; import {Ownable} from "../../src/common/utils/Ownable.sol"; /// `@notice` Test implementation of IBungeeExecutor. Decodes a uint256 from callData and stores it. /// `@dev` Deploy on destination chain. Use abi.encode(uint256) as executionCallData in scripts. -contract TestBungeeExecutor is Ownable { +contract TestBungeeExecutor is Ownable, IBungeeExecutor { @@ - function executeData(bytes32 quoteId, uint256 amount, address token, bytes calldata callData) external payable { + function executeData(bytes32 quoteId, uint256 amount, address token, bytes calldata callData) external payable override {Also applies to: 21-21
🤖 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/e2e/TestBungeeExecutor.sol` around lines 4 - 9, The TestBungeeExecutor mock currently doesn't declare or implement the IBungeeExecutor interface—update it to explicitly implement IBungeeExecutor by importing IBungeeExecutor and changing the contract declaration to implement that interface (e.g., contract TestBungeeExecutor is Ownable, IBungeeExecutor) and add all required functions from IBungeeExecutor with the exact signatures and override specifiers (implement the executor entrypoint(s) used in production, plus any view/owner methods) so the mock cannot drift from the real executor API; compile and fix any signature/visibility/return mismatches until no interface warnings remain.
🤖 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-11: The three env entries SOLVER_SIGNER_ADDRESS,
OPENROUTER_ADDRESS, and ACROSS_MANIPULATOR_ADDRESS currently have inline
comments after the = which makes dotenv treat them as empty strings; change each
to a bare empty assignment (e.g. SOLVER_SIGNER_ADDRESS=) and move the
explanatory comment to its own line above or below (e.g. # BungeeReceiver deploy
only; defaults to deployer) so the vars are unambiguously empty and
lint-friendly while preserving the existing comment text.
In `@package.json`:
- Around line 12-14: The check:* npm scripts (check:openrouter, check:receiver,
check:across-manipulator) currently call "hardhat run <script>" and will
silently run against the default network; change them to require an explicit
network and fail fast when none is supplied. Replace each script invocation with
a small wrapper that verifies npm_config_network is set (or inspects
process.argv for a --network arg) and exits with an error if missing, then
invokes hardhat run with --network "$npm_config_network" (or passes through the
provided --network). Update the three scripts (check:openrouter, check:receiver,
check:across-manipulator) to use this wrapper approach or call a shared
checkNetwork helper before running hardhat.
In `@scripts/deploy/create3.ts`:
- Around line 119-125: The resolveAcrossManipulatorAddress function currently
falls back silently to ACROSS_MANIPULATOR_EXPECTED_ADDRESS when
process.env.ACROSS_MANIPULATOR_ADDRESS exists but is malformed; change it to
reject malformed overrides by throwing an error: if ACROSS_MANIPULATOR_ADDRESS
is defined and ADDR_HEX_RE.test(envAddress) is false, throw a descriptive Error
(include the invalid env value) so callers know the override is invalid;
otherwise return the validated envAddress or the expected
ACROSS_MANIPULATOR_EXPECTED_ADDRESS as before.
In `@test/e2e/TestBungeeExecutor.sol`:
- Around line 21-23: In executeData, don't silently treat 1–31 byte callData as
zero; instead explicitly handle empty callData as counter=0 and require that
non-empty callData be at least 32 bytes before decoding. Update the executeData
function: if callData.length == 0 set counter = 0; else require(callData.length
>= 32, "Malformed callData"); then abi.decode(callData, (uint256)) into
counterValue and assign counter = counterValue. Reference: executeData and
counter.
---
Nitpick comments:
In `@test/e2e/TestBungeeExecutor.sol`:
- Around line 4-9: The TestBungeeExecutor mock currently doesn't declare or
implement the IBungeeExecutor interface—update it to explicitly implement
IBungeeExecutor by importing IBungeeExecutor and changing the contract
declaration to implement that interface (e.g., contract TestBungeeExecutor is
Ownable, IBungeeExecutor) and add all required functions from IBungeeExecutor
with the exact signatures and override specifiers (implement the executor
entrypoint(s) used in production, plus any view/owner methods) so the mock
cannot drift from the real executor API; compile and fix any
signature/visibility/return mismatches until no interface warnings remain.
🪄 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: 718bce8d-7c59-48bd-b244-18ddd4104613
📒 Files selected for processing (14)
.env.example.gitignorepackage.jsonscripts/deploy/checkReceiverDeployment.tsscripts/deploy/create3.tsscripts/deploy/deployReceiverAndExecutor.tssrc/BungeeReceiver.solsrc/CalldataExecutor.solsrc/OpenRouter.solsrc/interfaces/IBungeeExecutor.solsrc/interfaces/ICalldataExecutor.solsrc/lib/ExcessivelySafeCall.soltest/combined/BungeeReceiverDestPayload.t.soltest/e2e/TestBungeeExecutor.sol
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- src/OpenRouter.sol
🚧 Files skipped from review as they are similar to previous changes (7)
- scripts/deploy/checkReceiverDeployment.ts
- scripts/deploy/deployReceiverAndExecutor.ts
- src/CalldataExecutor.sol
- test/combined/BungeeReceiverDestPayload.t.sol
- src/interfaces/ICalldataExecutor.sol
- src/BungeeReceiver.sol
- src/lib/ExcessivelySafeCall.sol
🛑 Comments failed to post (4)
.env.example (1)
9-11:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick win🧩 Analysis chain
🌐 Web query:
How does dotenv parse a.envline likeFOO= # commentwhen the value is unquoted? Does it remain empty, or does the whitespace/comment become the value?💡 Result:
In the standard Node.js
dotenvlibrary, the value forFOO= # commentbecomes an empty string, not the comment [1][2]. For unquoted values,dotenvfollows these specific rules: - It trims whitespace from both ends of the value [1][2]. - The#character marks the beginning of a comment [1][2]. - When a line containsKEY= # comment, the parser identifies the value (which is empty) and treats the#as the start of a comment, resulting in an empty string [1][2]. Important considerations regarding this behavior: - Compatibility: While the Node.jsdotenvlibrary consistently treats this as an empty string, behavior can vary across different implementations and languages [3]. Some parsers might include the#as part of the value if they do not support inline comments or if they require specific spacing (e.g., some versions or alternative libraries require a space before the#to recognize it as a comment) [4][3][5]. - Portability: Because inline comment support is not universally standardized across all.envparsers, it is widely considered best practice to wrap values in quotes if they contain special characters like#or if you need to ensure leading/trailing whitespace is preserved [3]. Top results: [1][2][3]Citations:
- 1: https://github.com/motdotla/dotenv?tab=readme-ov-file
- 2: https://registry.npmjs.org/dotenv
- 3: https://env.dev/guides/env-file-syntax
- 4: https://pypi.org/project/python-dotenv/
- 5: https://dotenvx.com/docs/env-file
🏁 Script executed:
#!/bin/bash set -euo pipefail # Find where these env vars are read/used rg -n --hidden --glob '!.git/*' \ 'process\.env\.(SOLVER_SIGNER_ADDRESS|OPENROUTER_ADDRESS|ACROSS_MANIPULATOR_ADDRESS)\b' \ . # Also catch destructuring / direct string access patterns rg -n --hidden --glob '!.git/*' \ '(SOLVER_SIGNER_ADDRESS|OPENROUTER_ADDRESS|ACROSS_MANIPULATOR_ADDRESS)' \ src . || trueRepository: SocketDotTech/openrouter
Length of output: 1581
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show relevant code sections sed -n '1,120p' scripts/deploy/deployReceiverAndExecutor.ts sed -n '1,220p' scripts/deploy/create3.ts # Find which .env parser is used ls -1 if [ -f package.json ]; then cat package.json; fi rg -n --hidden --glob '!.git/*' "(dotenvx|dotenv)" . || trueRepository: SocketDotTech/openrouter
Length of output: 24281
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n --hidden --glob '!.git/*' --fixed-strings \ "SOLVER_SIGNER_ADDRESS" \ "OPENROUTER_ADDRESS" \ "ACROSS_MANIPULATOR_ADDRESS" \ . || trueRepository: SocketDotTech/openrouter
Length of output: 568
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n --hidden --glob '!.git/*' --fixed-strings \ -e "process.env.SOLVER_SIGNER_ADDRESS" \ -e "process.env.OPENROUTER_ADDRESS" \ -e "process.env.ACROSS_MANIPULATOR_ADDRESS" \ . || trueRepository: SocketDotTech/openrouter
Length of output: 378
Move inline comments off
=in.env.exampleoptional overrides (9-11)With this repo’s
dotenv,KEY= # ...results in an empty value, and the deploy scripts already fall back viatrim()/||and (for OpenRouter/Across) hex validation—so this doesn’t break defaults here. This formatting still triggers lint warnings and is brittle across dotenv parsers, so keep these env vars unambiguously empty by moving comments to their own lines.Suggested fix
- SOLVER_SIGNER_ADDRESS= # BungeeReceiver deploy only; defaults to deployer - OPENROUTER_ADDRESS= # optional override for check:openrouter - ACROSS_MANIPULATOR_ADDRESS= # optional override for check:across-manipulator + # BungeeReceiver deploy only; defaults to deployer + SOLVER_SIGNER_ADDRESS= + # Optional override for check:openrouter + OPENROUTER_ADDRESS= + # Optional override for check:across-manipulator + ACROSS_MANIPULATOR_ADDRESS=📝 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.# BungeeReceiver deploy only; defaults to deployer SOLVER_SIGNER_ADDRESS= # Optional override for check:openrouter OPENROUTER_ADDRESS= # Optional override for check:across-manipulator ACROSS_MANIPULATOR_ADDRESS=🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 9-9: [SpaceCharacter] The line has spaces around equal sign
(SpaceCharacter)
[warning] 9-9: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 10-10: [SpaceCharacter] The line has spaces around equal sign
(SpaceCharacter)
[warning] 10-10: [UnorderedKey] The OPENROUTER_ADDRESS key should go before the OWNER_ADDRESS key
(UnorderedKey)
[warning] 10-10: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 11-11: [SpaceCharacter] The line has spaces around equal sign
(SpaceCharacter)
[warning] 11-11: [UnorderedKey] The ACROSS_MANIPULATOR_ADDRESS key should go before the OPENROUTER_ADDRESS key
(UnorderedKey)
[warning] 11-11: [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 - 11, The three env entries SOLVER_SIGNER_ADDRESS, OPENROUTER_ADDRESS, and ACROSS_MANIPULATOR_ADDRESS currently have inline comments after the = which makes dotenv treat them as empty strings; change each to a bare empty assignment (e.g. SOLVER_SIGNER_ADDRESS=) and move the explanatory comment to its own line above or below (e.g. # BungeeReceiver deploy only; defaults to deployer) so the vars are unambiguously empty and lint-friendly while preserving the existing comment text.package.json (1)
12-14:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win🧩 Analysis chain
🌐 Web query:
In Hardhat 2.22.7, what network doeshardhat run <script>use when--networkis omitted, and willnpm run <script> --correctly satisfy a script defined ashardhat run <script> --network?💡 Result:
When
hardhat run <script>is executed without--network, Hardhat uses the config’s default network. If you don’t override it, the default value ofdefaultNetworkis "hardhat" [1]. So in Hardhat 2.22.7, the effective network is the built-in Hardhat in-memory network (the network namedhardhat) when--networkis omitted [1]. Regardingnpm run <script> -- <network>vs a script defined ashardhat run <script> --network: -npm run <name> -- <args...>forwards everything after the--to the command that the script expands to (standard npm behavior, i.e., arguments are passed through to the underlying CLI) [2]. - However, whether-- <network>correctly satisfies--networkdepends on how yourpackage.jsonscript is written. - It will work if your npm script is defined so that the forwarded argument lands immediately after--network, e.g."deploy": "hardhat run scripts/deploy.js --network"(thennpm run deploy -- hardhatresults inhardhat run ... --network hardhat). - It will not work if your npm script already hardcodes--network <something>or if there’s any mismatch in ordering/quoting (for example, if the script is"deploy": "hardhat run scripts/deploy.js --network <network>"and<network>is a literal placeholder rather than where npm is appending its forwarded argument). In short: Hardhat’s default (when--networkis omitted) ishardhat[1], andnpm run <script> -- <network>can correctly satisfyhardhat run <script> --networkonly if the--networkflag in yourpackage.jsonscript is positioned so that npm’s forwarded<network>becomes the value to that--networkflag [2].Citations:
- 1: https://hardhat.org/hardhat-runner/docs/config
- 2: https://dev.to/armorbreak/npm-scripts-and-packagejson-the-complete-guide-2026-4m9j
Require an explicit target network for
check:*hardhat runs.Without
--network,hardhat run <script>uses the config’sdefaultNetwork(defaults to the in-memoryhardhatnetwork), so these on-chain deployment checks can silently run against the wrong environment. This should fail fast unless a network is provided vianpm run check:<name> -- <network>.Suggested fix
- "check:openrouter": "hardhat run scripts/deploy/checkOpenRouterDeployment.ts", - "check:receiver": "hardhat run scripts/deploy/checkReceiverDeployment.ts", - "check:across-manipulator": "hardhat run scripts/deploy/checkAcrossManipulatorDeployment.ts", + "check:openrouter": "hardhat run scripts/deploy/checkOpenRouterDeployment.ts --network", + "check:receiver": "hardhat run scripts/deploy/checkReceiverDeployment.ts --network", + "check:across-manipulator": "hardhat run scripts/deploy/checkAcrossManipulatorDeployment.ts --network",🤖 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` around lines 12 - 14, The check:* npm scripts (check:openrouter, check:receiver, check:across-manipulator) currently call "hardhat run <script>" and will silently run against the default network; change them to require an explicit network and fail fast when none is supplied. Replace each script invocation with a small wrapper that verifies npm_config_network is set (or inspects process.argv for a --network arg) and exits with an error if missing, then invokes hardhat run with --network "$npm_config_network" (or passes through the provided --network). Update the three scripts (check:openrouter, check:receiver, check:across-manipulator) to use this wrapper approach or call a shared checkNetwork helper before running hardhat.scripts/deploy/create3.ts (1)
119-125:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject an invalid
ACROSS_MANIPULATOR_ADDRESSinstead of silently falling back.This should throw when the env var is present but malformed. The current fallback masks operator error and can make deployment checks probe the default address while the caller believes an override is being used.
Suggested fix
export function resolveAcrossManipulatorAddress(): string { const envAddress = process.env.ACROSS_MANIPULATOR_ADDRESS?.trim(); - if (envAddress && ADDR_HEX_RE.test(envAddress)) { - return envAddress; + if (envAddress) { + if (!ADDR_HEX_RE.test(envAddress)) { + throw new Error( + `Invalid ACROSS_MANIPULATOR_ADDRESS: ${envAddress}`, + ); + } + return envAddress; } return ACROSS_MANIPULATOR_EXPECTED_ADDRESS; }As per coding guidelines, “Check for resource leaks, race conditions, and unhandled 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/deploy/create3.ts` around lines 119 - 125, The resolveAcrossManipulatorAddress function currently falls back silently to ACROSS_MANIPULATOR_EXPECTED_ADDRESS when process.env.ACROSS_MANIPULATOR_ADDRESS exists but is malformed; change it to reject malformed overrides by throwing an error: if ACROSS_MANIPULATOR_ADDRESS is defined and ADDR_HEX_RE.test(envAddress) is false, throw a descriptive Error (include the invalid env value) so callers know the override is invalid; otherwise return the validated envAddress or the expected ACROSS_MANIPULATOR_EXPECTED_ADDRESS as before.test/e2e/TestBungeeExecutor.sol (1)
21-23:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject malformed non-empty
callData.This should not silently treat a 1-31 byte payload as
counter = 0. That lets bad encodings look successful in the e2e flow instead of failing at the executor boundary.Suggested fix
function executeData(bytes32 quoteId, uint256 amount, address token, bytes calldata callData) external payable { - uint256 counterValue = callData.length >= 32 ? abi.decode(callData, (uint256)) : 0; + require(callData.length == 0 || callData.length == 32, "invalid callData"); + uint256 counterValue = callData.length == 32 ? abi.decode(callData, (uint256)) : 0; counter = counterValue; lastQuoteId = quoteId; lastAmount = 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 `@test/e2e/TestBungeeExecutor.sol` around lines 21 - 23, In executeData, don't silently treat 1–31 byte callData as zero; instead explicitly handle empty callData as counter=0 and require that non-empty callData be at least 32 bytes before decoding. Update the executeData function: if callData.length == 0 set counter = 0; else require(callData.length >= 32, "Malformed callData"); then abi.decode(callData, (uint256)) into counterValue and assign counter = counterValue. Reference: executeData and counter.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/deploy/networks.ts (1)
55-56: 💤 Low valueRemove redundant self-references in aliases.
These entries include the primary key itself in the alias array (e.g.,
SCROLL_RPCinSCROLL_RPCaliases). TheresolveRpcUrlfunction already checksnetwork.rpcEnvKeyfirst at line 86, making these self-references redundant.♻️ Suggested cleanup
- SCROLL_RPC: ['SCROLL_RPC_URL', 'SCROLL_RPC'], - BLAST_RPC: ['BLAST_RPC_URL', 'BLAST_RPC'], + SCROLL_RPC: ['SCROLL_RPC_URL'], + BLAST_RPC: ['BLAST_RPC_URL'],🤖 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/networks.ts` around lines 55 - 56, The alias arrays for SCROLL_RPC and BLAST_RPC contain redundant self-references; update the mapping so each alias array only lists alternate environment keys (e.g., keep 'SCROLL_RPC_URL' for SCROLL_RPC and 'BLAST_RPC_URL' for BLAST_RPC) and remove the duplicate 'SCROLL_RPC' and 'BLAST_RPC' entries; this change aligns with the existing resolveRpcUrl logic that already checks network.rpcEnvKey first and ensures functions like resolveRpcUrl operate with only true alternate aliases.
🤖 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/deploy/receiverDeployCore.ts`:
- Around line 35-42: The resolveDeployerPrivateKey function currently trims but
does not remove surrounding single or double quotes, causing values like
'"0xabc..."' to keep quotes and break the startsWith('0x') check; update
resolveDeployerPrivateKey to import and call stripEnvQuotes (or inline
equivalent logic) on process.env.DEPLOYER_PRIVATE_KEY before trimming/validation
so quoted values are stripped, then perform the existing empty check and the
startsWith('0x') normalization (reference resolveDeployerPrivateKey and
stripEnvQuotes to locate the change).
---
Nitpick comments:
In `@scripts/deploy/networks.ts`:
- Around line 55-56: The alias arrays for SCROLL_RPC and BLAST_RPC contain
redundant self-references; update the mapping so each alias array only lists
alternate environment keys (e.g., keep 'SCROLL_RPC_URL' for SCROLL_RPC and
'BLAST_RPC_URL' for BLAST_RPC) and remove the duplicate 'SCROLL_RPC' and
'BLAST_RPC' entries; this change aligns with the existing resolveRpcUrl logic
that already checks network.rpcEnvKey first and ensures functions like
resolveRpcUrl operate with only true alternate aliases.
🪄 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: ff12c8ae-d2be-4a2d-8edb-8c6c11f61c81
📒 Files selected for processing (10)
package.jsonscripts/deploy/checkCalldataExecutorAllChains.tsscripts/deploy/checkCalldataExecutorDeployment.tsscripts/deploy/checkDeployerBalances.tsscripts/deploy/checkReceiverAllChains.tsscripts/deploy/deployReceiverAllChains.tsscripts/deploy/networks.tsscripts/deploy/receiverDeployCore.tssrc/BungeeReceiver.solsrc/common/lib/AuthenticationLib.sol
💤 Files with no reviewable changes (1)
- src/BungeeReceiver.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
| export function resolveDeployerPrivateKey(): string { | ||
| const key = process.env.DEPLOYER_PRIVATE_KEY?.trim(); | ||
| if (!key) { | ||
| throw new Error('DEPLOYER_PRIVATE_KEY is required'); | ||
| } | ||
|
|
||
| return key.startsWith('0x') ? key : `0x${key}`; | ||
| } |
There was a problem hiding this comment.
Strip surrounding quotes from DEPLOYER_PRIVATE_KEY.
resolveRpcUrl in networks.ts uses stripEnvQuotes to handle values wrapped in quotes, but this function doesn't. If a user sets DEPLOYER_PRIVATE_KEY='"0x123..."' or DEPLOYER_PRIVATE_KEY="'0x123...'", the quote characters will be included in the key, causing the startsWith('0x') check to fail and potentially prepending 0x incorrectly.
This should use the same quote-stripping logic for consistency.
🛡️ Proposed fix
Import and apply stripEnvQuotes (or inline the logic):
export function resolveDeployerPrivateKey(): string {
- const key = process.env.DEPLOYER_PRIVATE_KEY?.trim();
+ const raw = process.env.DEPLOYER_PRIVATE_KEY?.trim();
+ if (!raw) {
+ throw new Error('DEPLOYER_PRIVATE_KEY is required');
+ }
+ // Strip surrounding quotes like resolveRpcUrl does
+ let key = raw;
+ if (
+ (key.startsWith("'") && key.endsWith("'")) ||
+ (key.startsWith('"') && key.endsWith('"'))
+ ) {
+ key = key.slice(1, -1);
+ }
if (!key) {
throw new Error('DEPLOYER_PRIVATE_KEY is required');
}🤖 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/receiverDeployCore.ts` around lines 35 - 42, The
resolveDeployerPrivateKey function currently trims but does not remove
surrounding single or double quotes, causing values like '"0xabc..."' to keep
quotes and break the startsWith('0x') check; update resolveDeployerPrivateKey to
import and call stripEnvQuotes (or inline equivalent logic) on
process.env.DEPLOYER_PRIVATE_KEY before trimming/validation so quoted values are
stripped, then perform the existing empty check and the startsWith('0x')
normalization (reference resolveDeployerPrivateKey and stripEnvQuotes to locate
the change).
2 participants
Summary by CodeRabbit
New Features
Tests
Chores
Style