Polymer hypercore EXSC-169 [PolymerCCTPFacet v3.0.0, ITokenMessenger v1.1.0]#1652
Open
Polymer hypercore EXSC-169 [PolymerCCTPFacet v3.0.0, ITokenMessenger v1.1.0]#1652
Conversation
Contributor
WalkthroughUpdated Solidity baseline rules for Semantic Versioning requirements. Enhanced PolymerCCTPFacet with hook-based bridging support via new struct fields and conditional TOKEN_MESSENGER routing. Extended chain ID mapping for HyperCore domain. Updated corresponding interface and deployment configurations. Improved deployment script's constructor argument extraction from broadcast artifacts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
lifi-action-bot
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
0xDEnYO
changed the title
0xDEnYO
changed the title
Collaborator
Test Coverage ReportLine Coverage: 88.08% (3238 / 3676 lines) |
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Actions performedReview triggered.
|
mirooon
previously approved these changes
Mar 16, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Facets/PolymerCCTPFacet.sol (1)
246-267: Consider extracting duplicated hook-routing logic.The hook-routing conditional (
if hookData.length > 0) is duplicated between the EVM (lines 210-231) and non-EVM (lines 246-267) branches. While the current implementation is clear and correct, extracting this into an internal helper could reduce code duplication.That said, the duplication is minor and the explicit branching improves readability for auditors reviewing the two distinct paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Facets/PolymerCCTPFacet.sol` around lines 246 - 267, The hook-routing conditional (checking _polymerData.hookData.length and calling TOKEN_MESSENGER.depositForBurnWithHook vs TOKEN_MESSENGER.depositForBurn) is duplicated in both the EVM and non‑EVM branches; extract that logic into a single internal helper (e.g., _depositWithOptionalHook or similar) that accepts the common parameters (bridgeAmount, domainId, mintRecipient, token constant like USDC, destinationCaller/UNRESTRICTED_DESTINATION_CALLER, maxCCTPFee, minFinalityThreshold, hookData) and invoke that helper from both places (replace the two inline if/else blocks with calls to your new internal function) so the hook routing is centralized while preserving existing behavior and parameter choices.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Facets/PolymerCCTPFacet.sol`:
- Around line 246-267: The hook-routing conditional (checking
_polymerData.hookData.length and calling TOKEN_MESSENGER.depositForBurnWithHook
vs TOKEN_MESSENGER.depositForBurn) is duplicated in both the EVM and non‑EVM
branches; extract that logic into a single internal helper (e.g.,
_depositWithOptionalHook or similar) that accepts the common parameters
(bridgeAmount, domainId, mintRecipient, token constant like USDC,
destinationCaller/UNRESTRICTED_DESTINATION_CALLER, maxCCTPFee,
minFinalityThreshold, hookData) and invoke that helper from both places (replace
the two inline if/else blocks with calls to your new internal function) so the
hook routing is centralized while preserving existing behavior and parameter
choices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dcc088f6-4002-48e6-ba33-298dc9e2e52e
📒 Files selected for processing (6)
.cursor/rules/100-solidity-basics.mdcdeployments/arbitrum.diamond.staging.jsondeployments/arbitrum.staging.jsonsrc/Facets/PolymerCCTPFacet.solsrc/Interfaces/ITokenMessenger.soltest/solidity/Facets/PolymerCCTPFacet.t.sol
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
deployments/base.diamond.staging.json (1)
72-75: PolymerCCTPFacet Version field is empty but PR title indicates v3.0.0.The PR title explicitly references "PolymerCCTPFacet v3.0.0", yet the Version field here is an empty string. Consider populating the version for consistency with the PR scope and to support downstream scripts that may rely on version information for tracking or duplicate detection.
Suggested fix
"0xF158ac0983D6D4eF8Ef65De7B39585B8381Fe570": { "Name": "PolymerCCTPFacet", - "Version": "" + "Version": "3.0.0" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deployments/base.diamond.staging.json` around lines 72 - 75, The JSON entry for the contract at address "0xF158ac0983D6D4eF8Ef65De7B39585B8381Fe570" lists "Name": "PolymerCCTPFacet" but has an empty "Version" field; update that "Version" value to "v3.0.0" (matching the PR title) so downstream tooling and duplicate-detection scripts can rely on consistent version metadata for the PolymerCCTPFacet entry.
🤖 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/deploy/deploySingleContract.sh`:
- Around line 381-385: The trailing "0x" cleanup currently only runs when
CONSTRUCTOR_ARGS is >322 chars; remove that hardcoded size gate and run the
cleanup whenever CONSTRUCTOR_ARGS ends with "0x" by changing the outer condition
to only check the suffix (i.e., the "${CONSTRUCTOR_ARGS: -2}" == "0x" check) and
leave the inner alignment check on WITHOUT_TRAILING (the
($((${`#WITHOUT_TRAILING`} % 64)) -eq 2) logic) intact so shorter blobs ending
with "0x" are also fixed; refer to the CONSTRUCTOR_ARGS variable,
WITHOUT_TRAILING local, and the existing alignment check when updating the if
statement.
- Around line 367-378: The current logic sets CONSTRUCTOR_ARGS from the
broadcast JSON using jq with a default of "0x", which prevents the subsequent
fallback parser from running when the field is missing; update the assignment
and check so that CONSTRUCTOR_ARGS is unset/null when the broadcast file lacks
.returns.constructorArgs.value (for example remove the jq default // "0x" or
explicitly convert "null"/"0x" to empty), and adjust the conditional that
triggers the stdout/raw parser to run when CONSTRUCTOR_ARGS is empty or
"null"/"0x"; reference the BROADCAST_JSON lookup and the CONSTRUCTOR_ARGS /
JSON_DATA variables in the block so the fallback parser (the sed/grep/jq
sequence that populates JSON_DATA and then CONSTRUCTOR_ARGS) executes when the
broadcast value is absent.
---
Nitpick comments:
In `@deployments/base.diamond.staging.json`:
- Around line 72-75: The JSON entry for the contract at address
"0xF158ac0983D6D4eF8Ef65De7B39585B8381Fe570" lists "Name": "PolymerCCTPFacet"
but has an empty "Version" field; update that "Version" value to "v3.0.0"
(matching the PR title) so downstream tooling and duplicate-detection scripts
can rely on consistent version metadata for the PolymerCCTPFacet entry.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f61ab3af-8382-440c-8b79-ea2dd255d09b
📒 Files selected for processing (3)
deployments/base.diamond.staging.jsondeployments/base.staging.jsonscript/deploy/deploySingleContract.sh
✅ Files skipped from review due to trivial changes (1)
- deployments/base.staging.json
Comment on lines
+367
to
+378
| local BROADCAST_JSON="broadcast/$(basename "$FULL_SCRIPT_PATH")/$(getChainId "$NETWORK")/run-latest.json" | ||
| if [[ -f "$BROADCAST_JSON" ]]; then | ||
| CONSTRUCTOR_ARGS=$(jq -r '.returns.constructorArgs.value // "0x"' "$BROADCAST_JSON" 2>/dev/null) | ||
| fi | ||
| if [[ -z "${CONSTRUCTOR_ARGS:-}" || "$CONSTRUCTOR_ARGS" == "null" ]]; then | ||
| local JSON_DATA | ||
| JSON_DATA=$(echo "$RAW_RETURN_DATA" | sed -n '/{"logs":/,/}$/p' | tr -d '\n' | sed 's/} *$/}/') | ||
| if [[ -z "$JSON_DATA" || "$JSON_DATA" == "" ]]; then | ||
| JSON_DATA=$(echo "$RAW_RETURN_DATA" | grep -o '{"logs":.*}' | tail -1) | ||
| fi | ||
| CONSTRUCTOR_ARGS=$(echo "$JSON_DATA" | jq -r '.returns.constructorArgs.value // "0x"' 2>/dev/null) | ||
| fi |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "deploySingleContract.sh" -type fRepository: lifinance/contracts
Length of output: 102
🏁 Script executed:
find . -name "DeployPolymerCCTPFacet.s.sol" -type fRepository: lifinance/contracts
Length of output: 114
🏁 Script executed:
# Check the actual content of deploySingleContract.sh around lines 367-378
wc -l script/deploy/deploySingleContract.shRepository: lifinance/contracts
Length of output: 104
🏁 Script executed:
sed -n '360,390p' script/deploy/deploySingleContract.sh | cat -nRepository: lifinance/contracts
Length of output: 1844
🏁 Script executed:
cat -n script/deploy/facets/DeployPolymerCCTPFacet.s.sol | head -30Repository: lifinance/contracts
Length of output: 1237
🏁 Script executed:
# Let's check if there are any example broadcast JSON files or test data
find . -path "*/broadcast/*" -name "*.json" -type f | head -5Repository: lifinance/contracts
Length of output: 45
🏁 Script executed:
# Let's also check the Foundry docs or see if there are comments about broadcast structure
grep -r "returns.constructorArgs" script/ --include="*.sh" --include="*.sol"Repository: lifinance/contracts
Length of output: 342
🏁 Script executed:
# Test the jq behavior with different scenarios
python3 << 'EOF'
import json
import subprocess
# Test case 1: broadcast with .returns.constructorArgs.value
broadcast1 = {
"returns": {
"constructorArgs": {
"value": "0x1234"
}
}
}
# Test case 2: broadcast with .returns.constructorArgs directly
broadcast2 = {
"returns": {
"constructorArgs": "0x5678"
}
}
# Test case 3: broadcast without the field
broadcast3 = {
"returns": {}
}
# Test the original jq query
test_cases = [
("nested .value", broadcast1),
("direct field", broadcast2),
("missing field", broadcast3)
]
for name, data in test_cases:
with open(f'/tmp/test_{name.replace(" ", "_")}.json', 'w') as f:
json.dump(data, f)
# Original query
result = subprocess.run(['jq', '-r', '.returns.constructorArgs.value // "0x"', f'/tmp/test_{name.replace(" ", "_")}.json'],
capture_output=True, text=True)
print(f"{name} - original query (.value // \"0x\"): {result.stdout.strip()}")
# Proposed query
result = subprocess.run(['jq', '-er', '.returns.constructorArgs.value // .returns.constructorArgs // empty', f'/tmp/test_{name.replace(" ", "_")}.json'],
capture_output=True, text=True)
print(f"{name} - proposed query (.value // .constructorArgs // empty): {result.stdout.strip()}")
print(f" Exit code: {result.returncode}")
print()
EOFRepository: lifinance/contracts
Length of output: 485
🏁 Script executed:
# Let's look at how DeployScriptBase works to understand the broadcast structure
grep -A 20 "function run()" script/deploy/utils/DeployScriptBase.sol | head -40Repository: lifinance/contracts
Length of output: 136
🏁 Script executed:
# Check how other deploy scripts handle constructor args
grep -B 5 -A 5 "returns.*constructorArgs" script/deploy/facets/*.s.sol | head -50Repository: lifinance/contracts
Length of output: 3713
🏁 Script executed:
# Let's test the actual behavior more carefully - what happens with the direct field case
python3 << 'EOF'
import json
import subprocess
# Test case 2 revisited: broadcast with .returns.constructorArgs directly (no .value nesting)
broadcast2 = {
"returns": {
"constructorArgs": "0x5678"
}
}
with open('/tmp/test_direct.json', 'w') as f:
json.dump(broadcast2, f)
# Test original query step by step
print("=== Original query (.returns.constructorArgs.value // \"0x\") ===")
result = subprocess.run(['jq', '-r', '.returns.constructorArgs.value // "0x"', '/tmp/test_direct.json'],
capture_output=True, text=True)
print(f"Output: '{result.stdout.strip()}'")
print(f"Exit code: {result.returncode}")
# What if we just try .constructorArgs without fallback?
print("\n=== Just .returns.constructorArgs (no fallback) ===")
result = subprocess.run(['jq', '-r', '.returns.constructorArgs', '/tmp/test_direct.json'],
capture_output=True, text=True)
print(f"Output: '{result.stdout.strip()}'")
print(f"Exit code: {result.returncode}")
# What about the proposed query more carefully?
print("\n=== Proposed query with explicit empty ===")
result = subprocess.run(['jq', '.returns.constructorArgs.value // .returns.constructorArgs // empty', '/tmp/test_direct.json'],
capture_output=True, text=True)
print(f"Output: '{result.stdout.strip()}'")
print(f"Exit code: {result.returncode}")
# With -r flag
print("\n=== Proposed query with -r flag ===")
result = subprocess.run(['jq', '-r', '.returns.constructorArgs.value // .returns.constructorArgs // empty', '/tmp/test_direct.json'],
capture_output=True, text=True)
print(f"Output: '{result.stdout.strip()}'")
print(f"Exit code: {result.returncode}")
EOFRepository: lifinance/contracts
Length of output: 362
🏁 Script executed:
# Let's find DeployScriptBase to understand the structure
find . -name "DeployScriptBase.sol" -o -name "*DeployScript*" | grep -E "\.sol$"Repository: lifinance/contracts
Length of output: 162
🏁 Script executed:
# Look at the actual test to see if broadcast JSON is being used
grep -r "broadcast.*run-latest" script/ test/ --include="*.sh" --include="*.sol"Repository: lifinance/contracts
Length of output: 208
🏁 Script executed:
# Check if there are any actual broadcast artifacts in git history or tests
find . -name "run-latest.json" -type fRepository: lifinance/contracts
Length of output: 45
🏁 Script executed:
# Let's understand the Foundry behavior - check the DeployScriptBase implementation
find . -type f -name "*.sol" | xargs grep -l "DeployScriptBase" | head -3Repository: lifinance/contracts
Length of output: 270
🏁 Script executed:
cat script/deploy/facets/DeployAcrossFacet.s.sol | head -50Repository: lifinance/contracts
Length of output: 1339
🏁 Script executed:
# Let's examine DeployScriptBase to understand the broadcast structure
sed -n '1,100p' script/deploy/facets/utils/DeployScriptBase.sol | cat -nRepository: lifinance/contracts
Length of output: 3020
🏁 Script executed:
# Check how the return values are structured in Foundry
grep -A 30 "returns (" script/deploy/facets/utils/DeployScriptBase.solRepository: lifinance/contracts
Length of output: 1265
🏁 Script executed:
# Let's look at a test or actual script execution to understand the broadcast JSON structure better
grep -r "\.returns" script/ test/ --include="*.sh" --include="*.sol" -A 2Repository: lifinance/contracts
Length of output: 2060
🏁 Script executed:
# Check Foundry documentation or examples about broadcast structure
# Let's search for any documentation or comments about the broadcast JSON structure
grep -r "run-latest" . --include="*.md" --include="*.txt"Repository: lifinance/contracts
Length of output: 45
🏁 Script executed:
# Let's test what Foundry actually generates - search for JSON examples or tests
find . -type f \( -name "*.json" -o -name "*.test.*" -o -name "*.spec.*" \) | xargs grep -l "constructorArgs" 2>/dev/null | head -5Repository: lifinance/contracts
Length of output: 111
🏁 Script executed:
# Let's check if there are any actual broadcasts in the codebase or in git history
git log --all --full-history --oneline -- "broadcast/*" 2>/dev/null | head -10Repository: lifinance/contracts
Length of output: 45
🏁 Script executed:
# Let's look at how helperFunctions.sh uses the broadcast pattern
sed -n '1,100p' script/helperFunctions.sh | grep -A 10 "deployed.value"Repository: lifinance/contracts
Length of output: 45
🏁 Script executed:
# Test the actual -r behavior with empty more carefully
python3 << 'EOF'
import json
import subprocess
# Create test data that matches the expected Foundry broadcast format
broadcast_with_value = {
"returns": {
"constructorArgs": {
"value": "0x1234567890"
}
}
}
with open('/tmp/test_foundry.json', 'w') as f:
json.dump(broadcast_with_value, f)
print("=== Testing different jq approaches ===\n")
# Approach 1: Original (always defaults to 0x)
print("1. Original: .returns.constructorArgs.value // \"0x\"")
result = subprocess.run(['jq', '-r', '.returns.constructorArgs.value // "0x"', '/tmp/test_foundry.json'],
capture_output=True, text=True)
print(f" Result: '{result.stdout.strip()}' (exit: {result.returncode})\n")
# Approach 2: Check if field exists first
print("2. Check existence: if .returns.constructorArgs.value then .returns.constructorArgs.value else \"0x\" end")
result = subprocess.run(['jq', '-r', 'if .returns.constructorArgs.value then .returns.constructorArgs.value else "0x" end', '/tmp/test_foundry.json'],
capture_output=True, text=True)
print(f" Result: '{result.stdout.strip()}' (exit: {result.returncode})\n")
# Approach 3: Try proposed with different handling
print("3. Proposed variant: .returns.constructorArgs.value // .returns.constructorArgs // \"0x\"")
result = subprocess.run(['jq', '-r', '.returns.constructorArgs.value // .returns.constructorArgs // "0x"', '/tmp/test_foundry.json'],
capture_output=True, text=True)
print(f" Result: '{result.stdout.strip()}' (exit: {result.returncode})\n")
# Test with missing field
broadcast_missing = {
"returns": {}
}
with open('/tmp/test_missing.json', 'w') as f:
json.dump(broadcast_missing, f)
print("=== With missing constructorArgs field ===\n")
print("1. Original with missing field:")
result = subprocess.run(['jq', '-r', '.returns.constructorArgs.value // "0x"', '/tmp/test_missing.json'],
capture_output=True, text=True)
print(f" Result: '{result.stdout.strip()}' (exit: {result.returncode})\n")
print("2. Proposed with fallback to 0x:")
result = subprocess.run(['jq', '-r', '.returns.constructorArgs.value // .returns.constructorArgs // "0x"', '/tmp/test_missing.json'],
capture_output=True, text=True)
print(f" Result: '{result.stdout.strip()}' (exit: {result.returncode})\n")
EOFRepository: lifinance/contracts
Length of output: 621
🏁 Script executed:
# Let's examine the TypeScript test file to understand the broadcast structure
cat script/deploy/shared/deployment-logger.test.ts | head -100Repository: lifinance/contracts
Length of output: 3116
🏁 Script executed:
# Search for any mock broadcast data or examples in tests
find . -type f \( -name "*.test.*" -o -name "*.spec.*" \) | xargs grep -l "broadcast\|returns" 2>/dev/nullRepository: lifinance/contracts
Length of output: 151
🏁 Script executed:
# Let's check the actual logic flow - if broadcast returns "0x", does fallback execute?
python3 << 'EOF'
# Simulating the bash logic
CONSTRUCTOR_ARGS = "0x"
# This is the check from line 371
if not CONSTRUCTOR_ARGS or CONSTRUCTOR_ARGS == "null":
print("Fallback would execute")
else:
print("Fallback would NOT execute - CONSTRUCTOR_ARGS is:", CONSTRUCTOR_ARGS)
EOFRepository: lifinance/contracts
Length of output: 115
🏁 Script executed:
# Let's check if there are any real execution logs or test outputs that show broadcast JSON
find . -type f -name "*.json" | xargs grep -l "constructorArgs" 2>/dev/null | head -10Repository: lifinance/contracts
Length of output: 45
🏁 Script executed:
# Check the specific learning from playgroundHelpers.sh which mentions field name issues
sed -n '60,70p' script/playgroundHelpers.shRepository: lifinance/contracts
Length of output: 723
🏁 Script executed:
# Let's check the actual Foundry script output format
# Look for any documentation or comments about broadcast JSON structure
grep -r "constructorArgs.value" . --include="*.sh" --include="*.sol" --include="*.md"Repository: lifinance/contracts
Length of output: 346
🏁 Script executed:
# Check if there are any real-world examples of broadcast JSON in deployment logs or tests
find . -type f -name "*.json" -o -name "*.log" | xargs grep -l "returns" 2>/dev/null | head -5Repository: lifinance/contracts
Length of output: 323
🏁 Script executed:
# Let's trace through the actual logic more carefully with a bash simulation
bash -n script/deploy/deploySingleContract.sh && echo "Bash syntax is valid"Repository: lifinance/contracts
Length of output: 83
🏁 Script executed:
# Let's look at the complete context of the constructor args handling
sed -n '365,395p' script/deploy/deploySingleContract.sh | cat -nRepository: lifinance/contracts
Length of output: 2052
🏁 Script executed:
# Let's understand what Foundry actually puts in the broadcast JSON
# Check the Foundry documentation or source to understand the returns structure
web_search "Foundry script broadcast JSON returns structure"Repository: lifinance/contracts
Length of output: 113
🏁 Script executed:
# Also let's verify our understanding by checking how helperFunctions.sh handles this
sed -n '1,200p' script/helperFunctions.sh | grep -B 5 -A 10 "deployed.value"Repository: lifinance/contracts
Length of output: 45
🏁 Script executed:
# Check what getChainId returns to understand the broadcast path
grep -A 5 "function getChainId" script/deploy/deploySingleContract.shRepository: lifinance/contracts
Length of output: 45
🏁 Script executed:
# Let's carefully trace the actual behavior with the current code
python3 << 'EOF'
# Current code behavior simulation
# Scenario 1: Broadcast file exists with nested .value
print("=== Scenario 1: Broadcast exists with .returns.constructorArgs.value ===")
CONSTRUCTOR_ARGS = "0x1234" # jq returns the value
if not CONSTRUCTOR_ARGS or CONSTRUCTOR_ARGS == "null":
print("Would execute fallback: YES")
else:
print("Would execute fallback: NO")
print()
# Scenario 2: Broadcast file exists but field is missing
print("=== Scenario 2: Broadcast exists but field missing ===")
CONSTRUCTOR_ARGS = "0x" # jq defaults to "0x"
if not CONSTRUCTOR_ARGS or CONSTRUCTOR_ARGS == "null":
print("Would execute fallback: YES")
else:
print("Would execute fallback: NO - THIS IS THE PROBLEM")
print()
# Scenario 3: Broadcast file doesn't exist
print("=== Scenario 3: Broadcast file doesn't exist ===")
CONSTRUCTOR_ARGS = None # Variable is unset
if CONSTRUCTOR_ARGS is None or CONSTRUCTOR_ARGS == "":
print("Would execute fallback: YES")
else:
print("Would execute fallback: NO")
print()
# Scenario 4: Broadcast exists with different structure (direct string, not nested)
print("=== Scenario 4: Broadcast with .returns.constructorArgs (not .value) ===")
CONSTRUCTOR_ARGS = "" # jq can't find .returns.constructorArgs.value, returns empty from -r with no output
if not CONSTRUCTOR_ARGS or CONSTRUCTOR_ARGS == "null":
print("Would execute fallback: YES")
else:
print("Would execute fallback: NO")
EOFRepository: lifinance/contracts
Length of output: 447
🏁 Script executed:
# Let me check Foundry's documentation or look for examples in the codebase
# Search for any comments or documentation about the broadcast format
grep -r "broadcast" . --include="*.md" --include="README*" -A 3 -B 3 | head -50Repository: lifinance/contracts
Length of output: 987
🏁 Script executed:
# Let's also look at the actual learnings more carefully
# The learning mentions constructorArgs vs CONSTRUCTOR_ARGS field names
grep -r "constructorArgs" script/ --include="*.sh" -B 2 -A 2 | head -80Repository: lifinance/contracts
Length of output: 2130
🏁 Script executed:
# Let's check if any existing deployments or tests show what the broadcast JSON looks like
find . -type d -name "broadcast" | head -5Repository: lifinance/contracts
Length of output: 45
🏁 Script executed:
# Check the complete picture of where constructor args are used
grep -r "CONSTRUCTOR_ARGS" script/deploy/deploySingleContract.sh -nRepository: lifinance/contracts
Length of output: 1511
🏁 Script executed:
# Now let's understand: when jq can't find a path, what does it return with -r and //
python3 << 'EOF'
import json
import subprocess
test_cases = [
("nested .value exists", {"returns": {"constructorArgs": {"value": "0xabc"}}}),
("only constructorArgs exists", {"returns": {"constructorArgs": "0xdef"}}),
("nothing exists", {"returns": {}}),
("returns doesn't exist", {}),
]
for desc, data in test_cases:
with open('/tmp/test.json', 'w') as f:
json.dump(data, f)
print(f"\n{desc}:")
print(f" Data: {data}")
# Test original jq
result = subprocess.run(['jq', '-r', '.returns.constructorArgs.value // "0x"', '/tmp/test.json'],
capture_output=True, text=True)
print(f" .returns.constructorArgs.value // \"0x\": '{result.stdout.strip()}' (code: {result.returncode})")
# Test just .returns.constructorArgs
result = subprocess.run(['jq', '-r', '.returns.constructorArgs // "0x"', '/tmp/test.json'],
capture_output=True, text=True)
print(f" .returns.constructorArgs // \"0x\": '{result.stdout.strip()}' (code: {result.returncode})")
EOFRepository: lifinance/contracts
Length of output: 758
🏁 Script executed:
# Let me check how jq behaves with nested objects and -r flag more carefully
python3 << 'EOF'
import json
import subprocess
# The key scenario: what if constructorArgs is an object with a 'value' key?
data = {
"returns": {
"constructorArgs": {
"value": "0x1234"
}
}
}
with open('/tmp/test.json', 'w') as f:
json.dump(data, f)
# The review comment's proposed fix
print("=== Review's proposed fix ===")
result = subprocess.run(
['jq', '-er', '.returns.constructorArgs.value // .returns.constructorArgs // empty', '/tmp/test.json'],
capture_output=True, text=True
)
print(f"Output: '{result.stdout.strip()}'")
print(f"Stderr: '{result.stderr.strip()}'")
print(f"Exit: {result.returncode}")
print()
# What if we had both nested and non-nested?
data2 = {
"returns": {
"constructorArgs": "0xdirect"
}
}
with open('/tmp/test2.json', 'w') as f:
json.dump(data2, f)
print("=== Non-nested constructorArgs with proposed fix ===")
result = subprocess.run(
['jq', '-er', '.returns.constructorArgs.value // .returns.constructorArgs // empty', '/tmp/test2.json'],
capture_output=True, text=True
)
print(f"Output: '{result.stdout.strip()}'")
print(f"Stderr: '{result.stderr.strip()}'")
print(f"Exit: {result.returncode}")
EOFRepository: lifinance/contracts
Length of output: 285
🏁 Script executed:
# Check if there are any integration tests or example broadcasts
find . -type f \( -name "*.test.*" -o -name "*.sh" \) | xargs grep -l "run-latest" 2>/dev/nullRepository: lifinance/contracts
Length of output: 102
🏁 Script executed:
# Let's verify that the current code actually works by checking if there are any open issues or PRs related to this
git log --all --oneline --grep="constructor" -- script/deploy/deploySingleContract.sh 2>/dev/null | head -10Repository: lifinance/contracts
Length of output: 271
🏁 Script executed:
# Check if deployments have been logged successfully before to understand if the current logic works
head -20 deployments/mainnet.json 2>/dev/null || echo "File not found"Repository: lifinance/contracts
Length of output: 1317
🏁 Script executed:
# Let me check the git commits to understand what structure Foundry produces
git show bc7638f1 --stat 2>/dev/null | head -20Repository: lifinance/contracts
Length of output: 986
🏁 Script executed:
# Check the actual issue more carefully - look at the recent refactor
git log --all --oneline -p -- script/deploy/deploySingleContract.sh 2>/dev/null | head -100 | grep -A 20 -B 5 "constructorArgs"Repository: lifinance/contracts
Length of output: 45
🏁 Script executed:
# Let's also check if there's any documentation about how Forge structures return values
grep -r "forge.*run" . --include="*.md" -A 5 | head -40Repository: lifinance/contracts
Length of output: 45
🏁 Script executed:
# Test the actual proposed jq query with proper error handling
python3 << 'EOF'
import json
import subprocess
test_cases = [
("nested with .value", {"returns": {"constructorArgs": {"value": "0x1234"}}}),
("direct string", {"returns": {"constructorArgs": "0x5678"}}),
("missing field", {"returns": {}}),
]
print("Testing proposed fix: .returns.constructorArgs.value // .returns.constructorArgs // empty with || true\n")
for desc, data in test_cases:
with open('/tmp/test.json', 'w') as f:
json.dump(data, f)
# Simulate the bash || true behavior
result = subprocess.run(
'jq -er ".returns.constructorArgs.value // .returns.constructorArgs // empty" /tmp/test.json 2>/dev/null || echo ""',
shell=True,
capture_output=True,
text=True
)
print(f"{desc}:")
print(f" Output: '{result.stdout.strip()}'")
print()
EOFRepository: lifinance/contracts
Length of output: 264
🏁 Script executed:
# Check the actual learning about field names in the log entries
sed -n '50,80p' script/playgroundHelpers.shRepository: lifinance/contracts
Length of output: 1418
🏁 Script executed:
# Check if there's documentation about Foundry 1.5 broadcast format changes
find . -type f \( -name "*.md" -o -name "*.txt" \) | xargs grep -l "Foundry.*1.5\|broadcast.*json\|constructorArgs" 2>/dev/null | head -5Repository: lifinance/contracts
Length of output: 45
🏁 Script executed:
# Look at the actual recent commit changes to understand the broadcast handling
git show bc7638f1:script/deploy/deploySingleContract.sh 2>/dev/null | sed -n '365,395p' | cat -nRepository: lifinance/contracts
Length of output: 1610
🏁 Script executed:
# Compare with the current version
git diff bc7638f1 HEAD -- script/deploy/deploySingleContract.sh 2>/dev/null | head -80Repository: lifinance/contracts
Length of output: 4728
🏁 Script executed:
# Let me trace the exact issue with the proposed jq syntax more carefully
python3 << 'EOF'
import json
import subprocess
# Test the exact proposed jq query behavior
test_cases = [
("nested .value", {"returns": {"constructorArgs": {"value": "0x1234"}}}),
("direct string", {"returns": {"constructorArgs": "0x5678"}}),
("missing field", {"returns": {}}),
]
for desc, data in test_cases:
with open('/tmp/test.json', 'w') as f:
json.dump(data, f)
print(f"\n{desc}: {data}")
# Test proposed query without -e
result = subprocess.run(
['jq', '-r', '.returns.constructorArgs.value // .returns.constructorArgs // empty', '/tmp/test.json'],
capture_output=True, text=True
)
print(f" Without -e: output='{result.stdout.strip()}', stderr='{result.stderr.strip()}', exit={result.returncode}")
# Test proposed query with -e
result = subprocess.run(
['jq', '-er', '.returns.constructorArgs.value // .returns.constructorArgs // empty', '/tmp/test.json'],
capture_output=True, text=True
)
print(f" With -e: output='{result.stdout.strip()}', stderr='{result.stderr.strip()}', exit={result.returncode}")
EOFRepository: lifinance/contracts
Length of output: 628
🏁 Script executed:
# Check if there's any indication of what format Foundry currently outputs
grep -r "forge script" . --include="*.sh" -A 2 -B 2 | head -40Repository: lifinance/contracts
Length of output: 4982
Broadcast file defaults prevent fallback to raw return data parser.
When the broadcast JSON file exists but lacks .returns.constructorArgs.value, the jq query defaults to "0x" (line 369), preventing the fallback parser (line 371) from executing. This masks missing broadcast fields as valid zero-argument deployments, bypassing stdout parsing that may contain the actual constructor args.
For scripts returning bytes memory constructorArgs directly, like script/deploy/facets/DeployPolymerCCTPFacet.s.sol:13-18, this can skip both the broadcast lookup and stdout parser, logging empty constructor args instead.
🛠️ Proposed fix
local BROADCAST_JSON="broadcast/$(basename "$FULL_SCRIPT_PATH")/$(getChainId "$NETWORK")/run-latest.json"
if [[ -f "$BROADCAST_JSON" ]]; then
- CONSTRUCTOR_ARGS=$(jq -r '.returns.constructorArgs.value // "0x"' "$BROADCAST_JSON" 2>/dev/null)
+ CONSTRUCTOR_ARGS=$(jq -r '.returns.constructorArgs.value // empty' "$BROADCAST_JSON" 2>/dev/null || echo "")
fi
if [[ -z "${CONSTRUCTOR_ARGS:-}" || "$CONSTRUCTOR_ARGS" == "null" ]]; then
local JSON_DATA
JSON_DATA=$(echo "$RAW_RETURN_DATA" | sed -n '/{"logs":/,/}$/p' | tr -d '\n' | sed 's/} *$/}/')
if [[ -z "$JSON_DATA" || "$JSON_DATA" == "" ]]; then
JSON_DATA=$(echo "$RAW_RETURN_DATA" | grep -o '{"logs":.*}' | tail -1)
fi
- CONSTRUCTOR_ARGS=$(echo "$JSON_DATA" | jq -r '.returns.constructorArgs.value // "0x"' 2>/dev/null)
+ CONSTRUCTOR_ARGS=$(echo "$JSON_DATA" | jq -r '.returns.constructorArgs.value // empty' 2>/dev/null || echo "")
fi
+ CONSTRUCTOR_ARGS="${CONSTRUCTOR_ARGS:-0x}"🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 367-367: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@script/deploy/deploySingleContract.sh` around lines 367 - 378, The current
logic sets CONSTRUCTOR_ARGS from the broadcast JSON using jq with a default of
"0x", which prevents the subsequent fallback parser from running when the field
is missing; update the assignment and check so that CONSTRUCTOR_ARGS is
unset/null when the broadcast file lacks .returns.constructorArgs.value (for
example remove the jq default // "0x" or explicitly convert "null"/"0x" to
empty), and adjust the conditional that triggers the stdout/raw parser to run
when CONSTRUCTOR_ARGS is empty or "null"/"0x"; reference the BROADCAST_JSON
lookup and the CONSTRUCTOR_ARGS / JSON_DATA variables in the block so the
fallback parser (the sed/grep/jq sequence that populates JSON_DATA and then
CONSTRUCTOR_ARGS) executes when the broadcast value is absent.
Comment on lines
+381
to
+385
| if [[ ${#CONSTRUCTOR_ARGS} -gt 322 && "${CONSTRUCTOR_ARGS: -2}" == "0x" ]]; then | ||
| local WITHOUT_TRAILING="${CONSTRUCTOR_ARGS%0x}" | ||
| if [[ $((${#WITHOUT_TRAILING} % 64)) -eq 2 ]]; then | ||
| CONSTRUCTOR_ARGS="$WITHOUT_TRAILING" | ||
| fi |
Contributor
There was a problem hiding this comment.
Remove the hardcoded size gate from the trailing 0x cleanup.
Line 381 only repairs the malformed suffix for payloads longer than 322 chars. Shorter constructor-arg blobs can still arrive as ...0x, which keeps explorer verification broken even though the alignment check on WITHOUT_TRAILING is already enough to reject false positives.
🛠️ Proposed fix
- if [[ ${`#CONSTRUCTOR_ARGS`} -gt 322 && "${CONSTRUCTOR_ARGS: -2}" == "0x" ]]; then
+ if [[ "${CONSTRUCTOR_ARGS: -2}" == "0x" ]]; then
local WITHOUT_TRAILING="${CONSTRUCTOR_ARGS%0x}"
if [[ $((${`#WITHOUT_TRAILING`} % 64)) -eq 2 ]]; then
CONSTRUCTOR_ARGS="$WITHOUT_TRAILING"
fi
fi📝 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 [[ ${#CONSTRUCTOR_ARGS} -gt 322 && "${CONSTRUCTOR_ARGS: -2}" == "0x" ]]; then | |
| local WITHOUT_TRAILING="${CONSTRUCTOR_ARGS%0x}" | |
| if [[ $((${#WITHOUT_TRAILING} % 64)) -eq 2 ]]; then | |
| CONSTRUCTOR_ARGS="$WITHOUT_TRAILING" | |
| fi | |
| if [[ "${CONSTRUCTOR_ARGS: -2}" == "0x" ]]; then | |
| local WITHOUT_TRAILING="${CONSTRUCTOR_ARGS%0x}" | |
| if [[ $((${`#WITHOUT_TRAILING`} % 64)) -eq 2 ]]; then | |
| CONSTRUCTOR_ARGS="$WITHOUT_TRAILING" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@script/deploy/deploySingleContract.sh` around lines 381 - 385, The trailing
"0x" cleanup currently only runs when CONSTRUCTOR_ARGS is >322 chars; remove
that hardcoded size gate and run the cleanup whenever CONSTRUCTOR_ARGS ends with
"0x" by changing the outer condition to only check the suffix (i.e., the
"${CONSTRUCTOR_ARGS: -2}" == "0x" check) and leave the inner alignment check on
WITHOUT_TRAILING (the ($((${`#WITHOUT_TRAILING`} % 64)) -eq 2) logic) intact so
shorter blobs ending with "0x" are also fixed; refer to the CONSTRUCTOR_ARGS
variable, WITHOUT_TRAILING local, and the existing alignment check when updating
the if statement.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
script/deploy/deploySingleContract.sh (1)
365-389:⚠️ Potential issue | 🟠 MajorThe new constructor-args extraction is overwritten before it is used.
This block computes and sanitizes
CONSTRUCTOR_ARGS, but Line 387 immediately reassigns it fromJSON_DATA. When the broadcast artifact path succeeds,JSON_DATAwas never populated, so non-empty constructor args can collapse back to0x. Even in the fallback path, the newline / trailing-0xcleanup here never survives into verification or logging.🛠️ Proposed fix
if [[ -z "${CONSTRUCTOR_ARGS:-}" || "$CONSTRUCTOR_ARGS" == "null" ]]; then local JSON_DATA JSON_DATA=$(echo "$RAW_RETURN_DATA" | sed -n '/{"logs":/,/}$/p' | tr -d '\n' | sed 's/} *$/}/') if [[ -z "$JSON_DATA" || "$JSON_DATA" == "" ]]; then JSON_DATA=$(echo "$RAW_RETURN_DATA" | grep -o '{"logs":.*}' | tail -1) fi - CONSTRUCTOR_ARGS=$(echo "$JSON_DATA" | jq -r '.returns.constructorArgs.value // "0x"' 2>/dev/null) + CONSTRUCTOR_ARGS=$(echo "$JSON_DATA" | jq -r '.returns.constructorArgs.value // .returns[1].value // "0x"' 2>/dev/null) fi # Sanitize: forge stdout can embed newline + "0x" in the value, which breaks Etherscan verification CONSTRUCTOR_ARGS=$(printf '%s' "${CONSTRUCTOR_ARGS:-0x}" | tr -d '\n\r\t ') if [[ ${`#CONSTRUCTOR_ARGS`} -gt 322 && "${CONSTRUCTOR_ARGS: -2}" == "0x" ]]; then local WITHOUT_TRAILING="${CONSTRUCTOR_ARGS%0x}" if [[ $((${`#WITHOUT_TRAILING`} % 64)) -eq 2 ]]; then CONSTRUCTOR_ARGS="$WITHOUT_TRAILING" fi fi - - CONSTRUCTOR_ARGS=$(echo "$JSON_DATA" | jq -r '.returns.constructorArgs.value // .returns[1].value // "0x"' 2>/dev/null | head -1 | tr -d '\n')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/deploy/deploySingleContract.sh` around lines 365 - 389, The computed and sanitized CONSTRUCTOR_ARGS get clobbered by the later unconditional reassignment from JSON_DATA; remove or make that reassignment conditional so the earlier extraction/sanitization (the block that reads BROADCAST_JSON, RAW_RETURN_DATA, JSON_DATA and the sanitization/trailing-"0x" fix) survives; specifically, modify or remove the line that does CONSTRUCTOR_ARGS=$(echo "$JSON_DATA" | jq -r '.returns.constructorArgs.value // .returns[1].value // "0x"' 2>/dev/null | head -1 | tr -d '\n') so that it only sets CONSTRUCTOR_ARGS when CONSTRUCTOR_ARGS is empty or unset, leaving the existing sanitized CONSTRUCTOR_ARGS intact for later validation and logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@script/deploy/deploySingleContract.sh`:
- Around line 365-389: The computed and sanitized CONSTRUCTOR_ARGS get clobbered
by the later unconditional reassignment from JSON_DATA; remove or make that
reassignment conditional so the earlier extraction/sanitization (the block that
reads BROADCAST_JSON, RAW_RETURN_DATA, JSON_DATA and the
sanitization/trailing-"0x" fix) survives; specifically, modify or remove the
line that does CONSTRUCTOR_ARGS=$(echo "$JSON_DATA" | jq -r
'.returns.constructorArgs.value // .returns[1].value // "0x"' 2>/dev/null | head
-1 | tr -d '\n') so that it only sets CONSTRUCTOR_ARGS when CONSTRUCTOR_ARGS is
empty or unset, leaving the existing sanitized CONSTRUCTOR_ARGS intact for later
validation and logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fc93911a-3e06-4345-9030-527075fea66d
📒 Files selected for processing (4)
.cursor/rules/100-solidity-basics.mdcdeployments/arbitrum.diamond.staging.jsondeployments/arbitrum.staging.jsonscript/deploy/deploySingleContract.sh
✅ Files skipped from review due to trivial changes (1)
- deployments/arbitrum.staging.json
🚧 Files skipped from review as they are similar to previous changes (2)
- deployments/arbitrum.diamond.staging.json
- .cursor/rules/100-solidity-basics.mdc
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
4 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?
Why did I implement it this way?
Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)