Conversation
Contributor
|
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 the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@foundry.toml`:
- Line 170: The networks.json entry for Tempo is using an incompatible Etherscan
config; update the Tempo object so its verification settings match the actual
verifier used in foundry.toml: change "verificationType" from "etherscan" to
"custom" (or replace "explorerApiUrl" with "https://contracts.tempo.xyz" and
adjust any "explorerApi" parameters to Sourcify-compatible format) so
helperFunctions.sh reads the correct verifier; ensure the Tempo entry still
includes chain "4217" and any existing keys (e.g., explorerApiUrl,
verificationType) are updated consistently.
---
Duplicate comments:
In `@config/networks.json`:
- Around line 1152-1154: The network entry uses verificationType "etherscan" and
explorerApiUrl "https://api.etherscan.io/v2/api?chainid=4217" which conflicts
with foundry.toml's verifier = "custom" and url = "https://contracts.tempo.xyz";
update the JSON keys (verificationType, explorerUrl, explorerApiUrl) in the
network block to match the custom verifier used in foundry.toml (e.g., set
verificationType to "custom" and explorerApiUrl to the contracts.tempo.xyz URL
or remove the Etherscan-specific API URL), ensuring consistency between the
network config and foundry.toml's verifier and url settings so both reference
the same custom verification service.
…o include contract verification notes, create new tempo.json for deployment addresses, enhance deploySingleContract.sh with additional flags for tempo, and refactor healthCheck.ts and safe-utils.ts to utilize new HTTP transport configuration for RPC URLs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
script/deploy/deploySingleContract.sh (1)
261-263: PreferechoDebugfor new deployment diagnosticsLines 261-263 currently log on every attempt, even in non-debug runs. Consider routing these through
echoDebugto keep normal deploy output cleaner.♻️ Suggested tweak
- echo "ADDITIONAL_FLAGS: $ADDITIONAL_FLAGS" - echo "SKIP_SIMULATION_FLAG: $SKIP_SIMULATION_FLAG" - echo "GAS_ESTIMATE_MULTIPLIER: $GAS_ESTIMATE_MULTIPLIER" + echoDebug "ADDITIONAL_FLAGS: $ADDITIONAL_FLAGS" + echoDebug "SKIP_SIMULATION_FLAG: $SKIP_SIMULATION_FLAG" + echoDebug "GAS_ESTIMATE_MULTIPLIER: $GAS_ESTIMATE_MULTIPLIER"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/deploy/deploySingleContract.sh` around lines 261 - 263, The script currently prints ADDITIONAL_FLAGS, SKIP_SIMULATION_FLAG, and GAS_ESTIMATE_MULTIPLIER unconditionally, cluttering non-debug output; change those three echo calls to use the debug helper echoDebug instead (replace echo "ADDITIONAL_FLAGS: $ADDITIONAL_FLAGS", echo "SKIP_SIMULATION_FLAG: $SKIP_SIMULATION_FLAG", and echo "GAS_ESTIMATE_MULTIPLIER: $GAS_ESTIMATE_MULTIPLIER" with echoDebug "ADDITIONAL_FLAGS: $ADDITIONAL_FLAGS", echoDebug "SKIP_SIMULATION_FLAG: $SKIP_SIMULATION_FLAG", and echoDebug "GAS_ESTIMATE_MULTIPLIER: $GAS_ESTIMATE_MULTIPLIER") so these diagnostics are emitted only when debugging is enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@script/tasks/updateERC20Proxy.sh`:
- Line 30: The call to universalCast in updateERC20Proxy.sh hardcodes the
environment as "production"; change the argument used for the env/key path to
the ENVIRONMENT variable (e.g., replace the literal "production" with
"$ENVIRONMENT" or "${ENVIRONMENT}") so the script respects the passed
environment; verify that ENVIRONMENT is exported or set earlier in the script
(or provide a sensible default) before the universalCast "send" invocation that
references ERC20PROXY and setAuthorizedCaller.
---
Nitpick comments:
In `@script/deploy/deploySingleContract.sh`:
- Around line 261-263: The script currently prints ADDITIONAL_FLAGS,
SKIP_SIMULATION_FLAG, and GAS_ESTIMATE_MULTIPLIER unconditionally, cluttering
non-debug output; change those three echo calls to use the debug helper
echoDebug instead (replace echo "ADDITIONAL_FLAGS: $ADDITIONAL_FLAGS", echo
"SKIP_SIMULATION_FLAG: $SKIP_SIMULATION_FLAG", and echo
"GAS_ESTIMATE_MULTIPLIER: $GAS_ESTIMATE_MULTIPLIER" with echoDebug
"ADDITIONAL_FLAGS: $ADDITIONAL_FLAGS", echoDebug "SKIP_SIMULATION_FLAG:
$SKIP_SIMULATION_FLAG", and echoDebug "GAS_ESTIMATE_MULTIPLIER:
$GAS_ESTIMATE_MULTIPLIER") so these diagnostics are emitted only when debugging
is enabled.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
config/across.jsonconfig/networks.jsondeployments/tempo.jsonscript/deploy/deploySingleContract.shscript/deploy/healthCheck.tsscript/deploy/safe/safe-utils.tsscript/tasks/diamondUpdatePeriphery.shscript/tasks/updateERC20Proxy.shscript/utils/viemScriptHelpers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- config/networks.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@script/helperFunctions.sh`:
- Around line 1841-1843: The new etherscan branch appends --etherscan-api-key
without validating that the referenced API_KEY env var is set, which lets
verification run with an empty key; update the etherscan branch (the block that
checks VERIFICATION_TYPE == "etherscan" and mutates VERIFY_CMD) to validate the
dynamic API key (${!API_KEY:-}) before appending: check if the value is
non-empty (e.g., [[ -n "${!API_KEY:-}" ]]) and if empty call the existing
error/exit flow used elsewhere, otherwise append "--verifier" "etherscan" and
"--etherscan-api-key" "${!API_KEY}" to VERIFY_CMD so the required-key check is
enforced exactly as in the other verification paths.
- Around line 214-215: The sed pattern used to strip consola prefixes is too
strict and can drop valid JSON; update the trimming to match a JSON object or
array even if preceded by whitespace by changing the sed regex used when
assigning MONGO_RESULT to look for a leading optional whitespace followed by
either "{" or "[" (i.e., match /^[[:space:]]*[{[]/,$ ) instead of /^[{]/,$ so
indented or single-line JSON is preserved; apply the same adjusted pattern to
the other two occurrences flagged (the similar sed usages around lines 362-363
and 682-683) so all prefix-stripping uses the relaxed regex and only removes
non-JSON leading text.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
config/networks.jsonfoundry.tomlscript/helperFunctions.sh
…ing zero and non-contract addresses. Added logging for wrappedNativeAddress and acrossSpokePool in DeployAcrossFacetV4 script
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
script/deploy/facets/utils/ScriptBase.sol (1)
38-79: Logic is sound; consider using the existing custom error.The new overload correctly implements the flag-based behavior:
- Early return for allowed zero addresses
- Early return for allowed non-contract addresses (e.g., dummy address(1) for tempo)
- Contract-code check with revert otherwise
However, the custom error
NotAContract(string key)defined at line 12 is unused. The code usesrevert(string.concat(...))instead, which is less gas-efficient and inconsistent with the error definition.♻️ Optional: Use the existing custom error for consistency
// check if address contains code if (!LibAsset.isContract(contractAddress)) - revert( - string.concat(key, " in file ", path, " is not a contract") - ); + revert NotAContract(key); return contractAddress; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/deploy/facets/utils/ScriptBase.sol` around lines 38 - 79, The function _getConfigContractAddress currently reverts with a concatenated string when the address has no contract code; replace that string-based revert with the existing custom error NotAContract(string key) to save gas and keep errors consistent—locate the contract-code check in _getConfigContractAddress and change the revert(...) to revert NotAContract(key), leaving the early-return logic for allowZeroAddress and allowNonContractAddress intact.config/networks.json (1)
1143-1163: Configuration looks correct; minor devNotes inconsistency.The tempo network entry is properly structured with all required fields. The dummy
wrappedNativeAddressof address(1) correctly works with theallowNonContractAddress=trueflag inDeployAcrossFacetV4.s.sol.Minor note: The devNotes state "DeployAcrossFacetV4.s.sol uses allowZeroAddress for wrappedNativeAddress" but the actual code uses
allowNonContractAddress = true(notallowZeroAddress). Consider updating for accuracy:📝 Suggested devNotes clarification
- "devNotes": "In order to deploy contract its required to have GAS_MULTIPLIER set to 550 in .env file. If it's more than 550, the deployment for LiFiDEXAggregator will fail with gas limit exceeded error if less than 500 then the deployment for LiFiDEXAggregator will fail with out of gas error. Moreover we use dummy wrapped native address for AcrossFacetV4 because we don't activate the native path (DeployAcrossFacetV4.s.sol uses allowZeroAddress for wrappedNativeAddress so tempo and similar networks can use this dummy value)." + "devNotes": "In order to deploy contract its required to have GAS_MULTIPLIER set to 550 in .env file. If it's more than 550, the deployment for LiFiDEXAggregator will fail with gas limit exceeded error if less than 500 then the deployment for LiFiDEXAggregator will fail with out of gas error. Moreover we use dummy wrapped native address for AcrossFacetV4 because we don't activate the native path (DeployAcrossFacetV4.s.sol uses allowNonContractAddress for wrappedNativeAddress so tempo and similar networks can use this dummy value)."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/networks.json` around lines 1143 - 1163, The devNotes for the "tempo" entry incorrectly says DeployAcrossFacetV4.s.sol uses allowZeroAddress for wrappedNativeAddress; update the devNotes text to accurately state that DeployAcrossFacetV4.s.sol uses allowNonContractAddress = true (and mention the dummy wrappedNativeAddress/address(1) usage) so readers see the correct deployment flag; reference the "tempo" object, the wrappedNativeAddress field, and DeployAcrossFacetV4.s.sol with the allowNonContractAddress flag in the updated note.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@script/helperFunctions.sh`:
- Around line 359-363: The sed extraction for MONGO_RESULT is too broad and
treats lines like "[info] ..." as JSON; update the sed regexp used when setting
MONGO_RESULT to only start extracting when a line begins with '[' followed
(after optional whitespace) by a valid JSON array element character (e.g. ']',
'{', '"', a digit, '-', 'n', 't', or 'f') instead of any '['; modify the sed
pattern in the MONGO_RESULT assignment so it checks the character immediately
after the '[' to ensure it's a JSON array start before printing from that line.
---
Nitpick comments:
In `@config/networks.json`:
- Around line 1143-1163: The devNotes for the "tempo" entry incorrectly says
DeployAcrossFacetV4.s.sol uses allowZeroAddress for wrappedNativeAddress; update
the devNotes text to accurately state that DeployAcrossFacetV4.s.sol uses
allowNonContractAddress = true (and mention the dummy
wrappedNativeAddress/address(1) usage) so readers see the correct deployment
flag; reference the "tempo" object, the wrappedNativeAddress field, and
DeployAcrossFacetV4.s.sol with the allowNonContractAddress flag in the updated
note.
In `@script/deploy/facets/utils/ScriptBase.sol`:
- Around line 38-79: The function _getConfigContractAddress currently reverts
with a concatenated string when the address has no contract code; replace that
string-based revert with the existing custom error NotAContract(string key) to
save gas and keep errors consistent—locate the contract-code check in
_getConfigContractAddress and change the revert(...) to revert
NotAContract(key), leaving the early-return logic for allowZeroAddress and
allowNonContractAddress intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 76b22641-3a67-4916-b040-bc4d431b30c1
📒 Files selected for processing (6)
config/networks.jsondeployments/tempo.diamond.jsondeployments/tempo.jsonscript/deploy/facets/DeployAcrossFacetV4.s.solscript/deploy/facets/utils/ScriptBase.solscript/helperFunctions.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- deployments/tempo.json
- deployments/tempo.diamond.json
…emove redundant variable in helperFunctions.sh
…in updateERC20Proxy
mirooon
reviewed
Mar 4, 2026
mirooon
reviewed
Mar 4, 2026
… into deploy-network-tempo
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
3 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.
Which Jira task belongs to this PR?
SMAR-130
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)