fix: refresh IdentityBeacon bytecode (constructor was skipping BindingLib.bindComponent)#60
fix: refresh IdentityBeacon bytecode (constructor was skipping BindingLib.bindComponent)#60koko1123 wants to merge 3 commits into
Conversation
…tructor calls BindingLib.bindComponent Every beacon created via /create_beacon_with_ecdsa on mainnet has been deployed with verifier.authorizedCaller == address(0), so the verifier rejects every update with UnauthorizedCaller (0x5c427cd9). Confirmed on-chain for the two Hormuz beacons created 2026-05-29: 0x1b37de2b…ef884 (count) verifier 0x930E46b…6BF83 authorizedCaller=0x0 0x853f7ce…b55152 (tonnage) verifier 0x7e720E0…53ccc authorizedCaller=0x0 Root cause: abis/IdentityBeacon.bytecode was generated before BindingLib landed in beacons (commit 'add caller binding (#59)' in the v0.0.1 tag). The IdentityBeacon constructor at v0.0.1 calls `BindingLib.bindComponent(_verifier)` as its final step, which binds the verifier to the new beacon atomically. But the beaconator's create_identity_beacon raw-deploys bytecode from state.contracts.identity_beacon_bytecode, which was the pre-BindingLib build (Mar 2). Re-deploying with that bytecode skips the bind entirely. Fix: - Regenerate abis/IdentityBeacon.bytecode from beacons@v0.0.1 (7994 -> 8665 bytes, the delta is the BindingLib staticcall + bind call). - Extend scripts/refresh-abis.sh to write the deploy-time bytecode in the same pass as the ABIs, so a future beacons-side change can't silently desync the bytecode again. Adds a generic inspect_bytecode_to helper next to inspect_abi_to. After this lands and the beaconator is rebuilt + redeployed every NEW beacon created via /create_beacon_with_ecdsa will be correctly bound at deploy time. The two existing Hormuz beacons are being fixed manually out-of-band with two cast send bind(beacon) calls — that's not blocked by this PR. Tested: make fmt + make lint + make test-fast all green.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughrefresh-abis.sh is refactored to create reusable detached worktrees and modular inspect helpers; the script regenerates ABIs and IdentityBeacon deploy-time bytecode. New Rust unit tests validate the on-disk IdentityBeacon.bytecode artifact. ChangesABI and Bytecode Refresh Tooling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/refresh-abis.sh`:
- Around line 105-106: The ABI/bytecode writes are currently redirected straight
into "$ABIS_DIR/$out" which can leave empty/partial files if `forge inspect`
fails (with set -e). Change each direct redirection (the `( cd "$wt" && forge
inspect "$contract" ... ) > "$ABIS_DIR/$out"` and the corresponding bytecode
line) to write to a temporary file (e.g. "$ABIS_DIR/$out.tmp.$$"), run the `( cd
"$wt" && forge inspect ... )` into that temp, check success, then mv the temp
into "$ABIS_DIR/$out"; ensure the temp is removed on failure to avoid orphan
files. This preserves atomicity for the commands that reference "$wt",
"$contract" and "$ABIS_DIR/$out".
- Around line 94-96: The git config token in scripts/refresh-abis.sh uses mixed
quoting which triggers ShellCheck SC2140; replace the mixed-quote -c
url."https://github.com/".insteadOf="git@github.com:" argument with a
single-quoted key=value token, e.g. change the git invocation (the line using
git -C "$wt" -c ...) to use -c
'url."https://github.com/".insteadOf=git@github.com:' so the entire config is
one argument and SC2140 is resolved.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: feafdb9d-792b-49b9-887c-5582b0407560
📒 Files selected for processing (2)
abis/IdentityBeacon.bytecodescripts/refresh-abis.sh
Addresses CodeRabbit's two findings on #60: - scripts/refresh-abis.sh: ShellCheck SC2140 — the inline `git -c` key=value argument with double-quote sandwiches now passes through as a single-quoted string ('url.https://github.com/.insteadOf=git@github.com:'), which both quiets the linter and removes the double-quote-inside-double-quote edge case in the original form. - scripts/refresh-abis.sh: `forge inspect ... > $ABIS_DIR/$out` truncated the artefact before forge ran, so a failed inspect could silently leave a partial file in abis/. Both inspect helpers now share a single `inspect_to` core that streams into a mktemp under $ABIS_DIR and atomically mv's it into place on success only. Adds tests/unit_tests/identity_beacon_bytecode_tests.rs to lock in exactly the regression this PR fixes: - identity_beacon_bytecode_is_non_trivial_creation_code asserts the file is on the order of 4-5 KiB and starts with PUSH1, so a future atomic-write hiccup leaving a truncated artefact in tree gets caught at `make test-fast` time instead of in production. - identity_beacon_constructor_calls_bindcomponent reads the on-disk bytecode and searches it for the two selectors BindingLib needs: authorizedCaller() (0xfaa7d4b0) and bind(address) (0x0a3b0a4f). Both must be present as PUSH4 immediates in the constructor for the beacon to bind its verifier at deploy time. The selectors are derived via keccak256 at test time so the assertion stays canonical even if some renaming happens on the contracts side. Tested locally: make fmt + make lint + the two new tests all green.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/refresh-abis.sh (1)
70-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCleanup logic never executes; worktrees leak.
Line 72 checks
-d "$wt", but$wtis the composite string"repo::path"(e.g.,"../beacons::/tmp/beacons-v0.0.1-xyz"), which is never a valid directory. The condition always fails, sogit worktree removenever runs and temporary worktrees accumulate in/tmp.🔧 Proposed fix
Extract
repoandpathbefore the directory check:cleanup() { for wt in "${WORKTREES[@]:-}"; do - if [[ -n "$wt" && -d "$wt" ]]; then - # First arg is the repo, second is the worktree path. - local repo="${wt%%::*}" - local path="${wt##*::}" + local repo="${wt%%::*}" + local path="${wt##*::}" + if [[ -n "$path" && -d "$path" ]]; then git -C "$repo" worktree remove "$path" --force >/dev/null 2>&1 || true fi done }🤖 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/refresh-abis.sh` around lines 70 - 80, The cleanup function is checking -d on the composite WORKTREES entry (variable wt like "repo::path") so the condition always fails; change cleanup() to first split each wt into repo="${wt%%::*}" and path="${wt##*::}" and then test -n "$path" and -d "$path" (or -e if you prefer) before calling git -C "$repo" worktree remove "$path" --force, preserving the existing git command and the loop over WORKTREES.
🧹 Nitpick comments (1)
scripts/refresh-abis.sh (1)
120-120: ⚡ Quick winPrefer word-splitting over
evalfor theforgecommand.The
evalon line 120 works correctly but is unnecessary. Since$whatis always a hardcoded string ("abi --json"or"bytecode"), you can rely on unquoted word-splitting instead:if ( cd "$wt" && forge inspect "$contract" $what ) > "$tmp"; thenThis is simpler, avoids the nested quoting complexity, and eliminates a potential future ShellCheck SC2086 warning.
♻️ Proposed simplification
- if ( cd "$wt" && eval "forge inspect \"$contract\" $what" ) > "$tmp"; then + # shellcheck disable=SC2086 + if ( cd "$wt" && forge inspect "$contract" $what ) > "$tmp"; then(The
disable=SC2086acknowledges intentional word-splitting of$what.)🤖 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/refresh-abis.sh` at line 120, Replace the unnecessary eval in the subshell that runs the Forge inspection: instead of using eval "forge inspect \"$contract\" $what" use plain word-splitting so the line becomes ( cd "$wt" && forge inspect "$contract" $what ) > "$tmp"; this relies on $what being a hardcoded token like "abi --json" or "bytecode"; if you want to document the intentional word-splitting for linters, add a nearby shellcheck disable comment for SC2086.
🤖 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 `@tests/unit_tests/identity_beacon_bytecode_tests.rs`:
- Around line 46-48: Update the error message string in the
identity_beacon_bytecode_tests.rs test that currently says "expected creation
code on the order of 4-5 KiB" so it reflects the actual bytecode size (~8665
bytes); change it to something like "expected creation code on the order of 8-9
KiB (≈8.7 KiB / 8665 bytes)" so the formatted message emitted by the test
matches the PR's bytecode size expectation.
---
Outside diff comments:
In `@scripts/refresh-abis.sh`:
- Around line 70-80: The cleanup function is checking -d on the composite
WORKTREES entry (variable wt like "repo::path") so the condition always fails;
change cleanup() to first split each wt into repo="${wt%%::*}" and
path="${wt##*::}" and then test -n "$path" and -d "$path" (or -e if you prefer)
before calling git -C "$repo" worktree remove "$path" --force, preserving the
existing git command and the loop over WORKTREES.
---
Nitpick comments:
In `@scripts/refresh-abis.sh`:
- Line 120: Replace the unnecessary eval in the subshell that runs the Forge
inspection: instead of using eval "forge inspect \"$contract\" $what" use plain
word-splitting so the line becomes ( cd "$wt" && forge inspect "$contract" $what
) > "$tmp"; this relies on $what being a hardcoded token like "abi --json" or
"bytecode"; if you want to document the intentional word-splitting for linters,
add a nearby shellcheck disable comment for SC2086.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 68514573-0efc-4f9a-936d-6be5ae1da63b
📒 Files selected for processing (3)
scripts/refresh-abis.shtests/unit_tests/identity_beacon_bytecode_tests.rstests/unit_tests/mod.rs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary
Every beacon created via
POST /create_beacon_with_ecdsaon mainnet was deployed withverifier.authorizedCaller == address(0). The ECDSAVerifier'sverify()starts with_checkAuthorizedCaller(), so every subsequentupdate()call from the beacon reverts withUnauthorizedCaller(selector0x5c427cd9) — the beaconator surfaces that asHTTP 500 Internal Server Errorto the updater.Confirmed on Arbitrum One for the two Hormuz beacons created today:
0x1b37de2b…ef884(count)0x930E46b…6BF830x0← bug0x853f7ce…b55152(tonnage)0x7e720E0…53ccc0x0← bugRoot cause
abis/IdentityBeacon.bytecodewas generated beforeBindingLiblanded in beacons (commit "add caller binding (#59)", which shipped in tagv0.0.1).IdentityBeacon's constructor at v0.0.1 ends with:…which binds the verifier to the new beacon atomically as part of the create tx. But the beaconator's
create_identity_beaconraw-deploys bytecode fromstate.contracts.identity_beacon_bytecode— which was the pre-BindingLib build (file mtimeMar 2). The constructor in that older bytecode never callsbindComponent, so the verifier sits withauthorizedCaller = 0x0forever.Fix
abis/IdentityBeacon.bytecodefrombeacons@v0.0.1. Size went from 7994 → 8665 bytes; the delta is theBindingLib.bindComponent(staticcall toauthorizedCaller, conditionalbind()call) at the end of the constructor.scripts/refresh-abis.shto write the deploy-time bytecode in the same pass as the ABIs. Adds aninspect_bytecode_tohelper alongsideinspect_abi_toand refactors the per-repo worktree setup so each worktree is created exactly once. This way a future beacons-side change can't silently desync the bytecode again.Effect
After this lands and the beaconator is rebuilt + redeployed, every new beacon created via
/create_beacon_with_ecdsais correctly bound at deploy time. No application code changes (the bytecode is loaded from disk at startup intoAppState.contracts.identity_beacon_bytecode).Test plan
make fmtcleanmake lintcleanmake test-fastcleanmake refresh-abisreproducible on a clean clone (script ran end-to-end, produced expected file deltas)/create_beacon_with_ecdsa, confirmverifier.authorizedCaller()returns the new beacon address — not0x0.Existing Hormuz beacons (not in this PR)
The two beacons already deployed on mainnet (count
0x1b37de2b…, tonnage0x853f7ce…) need a one-time manualbind()to recover them.bind()has no access control beyond "not already bound", so any wallet with Arbitrum ETH can run twocast sendcalls. Handled out-of-band — not blocked by this PR.Follow-up worth considering (not in this PR)
create_identity_beaconshould probably also confirm the binding after deploy (oneeth_calltoverifier.authorizedCaller()matched against the new beacon address) so any future bytecode-vs-source desync is caught at create time, not on the nextupdate(). Happy to ship that in a follow-up if you want it.Summary by CodeRabbit
Chores
Tests