feat(allbridge): dynamic chainId mapping + Stellar destination (EXSC-650) [AllBridgeFacet v2.2.0]#2075
feat(allbridge): dynamic chainId mapping + Stellar destination (EXSC-650) [AllBridgeFacet v2.2.0]#20750xDEnYO wants to merge 3 commits into
Conversation
…idgeFacet v2.2.0] (EXSC-650) Replace the hardcoded LI.FI-chainId -> AllBridge-chainId if/else in AllBridgeFacet with an owner-updatable on-chain mapping (init / set / unset / get), mirroring the PolymerCCTPFacet EXSC-254 pattern, and add Stellar (LIFI_CHAIN_ID_STELLAR -> allBridgeChainId 7) so any->Stellar bridging works via the existing non-EVM path. Config gains a `mappings` array, the update script seeds it in the diamond cut, and a new proposeAllBridgeChainIdMappings.ts syncs mappings across chains. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
the
requires-types
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughChangesAllBridge mapping rollout
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
lifi-action-bot
changed the title
Test Coverage ReportLine Coverage: 90.20% (3351 / 3715 lines) |
🤖 GitHub Action: Security Alerts Review 🔍🟢 Dismissed Security Alerts with Comments 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: ✅ No unresolved security alerts! 🎉 |
…sc, mainnet, optimism, polygon) (EXSC-650) Deployment logs for the staging diamondCut + initAllBridge of AllBridgeFacet v2.2.0 (dynamic chainId mapping + Stellar) at 0x1c1488c16d9d790e378B7354b35B0F12Dc9D4B49. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
script/tasks/proposeAllBridgeChainIdMappings.ts (1)
371-371: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeclare the CLI callback return type.
- async run({ args }) { + async run({ args }): Promise<void> {As per coding guidelines, TypeScript functions must use explicit return types.
🤖 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 `@script/tasks/proposeAllBridgeChainIdMappings.ts` at line 371, Update the async run method to declare an explicit Promise-based return type in its signature, preserving the existing callback behavior and implementation.Source: Coding guidelines
🤖 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 `@script/tasks/proposeAllBridgeChainIdMappings.ts`:
- Around line 258-263: The scheduleBatch proposal currently uses Date.now() in
the salt, changing the Safe calldata and defeating deduplication on reruns.
Update the proposal-building logic around the salt and the corresponding batch
flow to use a stable salt for the intended batch, while supporting an explicit
operator-controlled epoch or salt when intentional replay is required; preserve
duplicate detection for identical proposals.
In `@src/Facets/AllBridgeFacet.sol`:
- Around line 96-104: Enforce the reserved zero sentinel at every AllBridge
mapping boundary: in src/Facets/AllBridgeFacet.sol lines 96-104, update
initAllBridge to reject zero chainId or allBridgeChainId with InvalidConfig
before writes; in lines 117-133, apply the same validation in the update path
before storage writes and events. In test/solidity/Facets/AllBridgeFacet.t.sol
lines 592-600, add InvalidConfig coverage for both zero fields during
initialization and updates.
In `@test/solidity/Facets/AllBridgeFacet.t.sol`:
- Around line 592-600: Expand
testRevert_SetChainIdToAllBridgeChainIdWithEmptyConfig coverage to verify
InvalidConfig for zero-valued chainId and allBridgeChainId mappings during both
initialization and updates. Add cases for each field being zero, while
preserving the existing empty-configuration assertion and using the established
setup and revert-checking patterns.
---
Nitpick comments:
In `@script/tasks/proposeAllBridgeChainIdMappings.ts`:
- Line 371: Update the async run method to declare an explicit Promise-based
return type in its signature, preserving the existing callback behavior and
implementation.
🪄 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: 051f49ab-b12c-4f69-98eb-fe82a3d785e1
📒 Files selected for processing (16)
config/allbridge.jsondeployments/arbitrum.diamond.staging.jsondeployments/arbitrum.staging.jsondeployments/bsc.diamond.staging.jsondeployments/bsc.staging.jsondeployments/mainnet.diamond.staging.jsondeployments/mainnet.staging.jsondeployments/optimism.diamond.staging.jsondeployments/optimism.staging.jsondeployments/polygon.diamond.staging.jsondeployments/polygon.staging.jsondocs/AllBridgeFacet.mdscript/deploy/facets/UpdateAllBridgeFacet.s.solscript/tasks/proposeAllBridgeChainIdMappings.tssrc/Facets/AllBridgeFacet.soltest/solidity/Facets/AllBridgeFacet.t.sol
🔍 QA Review — EXSC-650
🔄 Resolution Status
✅ Finding #1 — Resolved
if (chainId == 0 || allBridgeChainId == 0) revert InvalidConfig();The
Coverage and correctness verified from current file state. ✅ ❌ Finding #2 — Still Open
Current code: function testRevert_FailsToSetChainIdToAllBridgeChainIdIfNotInitialized()
public
{
vm.startPrank(address(0));
TestAllBridgeFacet uninitializedFacet = new TestAllBridgeFacet(
ALLBRIDGE_ROUTER
);
// ... calls setChainIdToAllBridgeChainId
// expects NotInitializedThe commit The tests are functionally correct today (fresh diamond storage has The suggested fix (from the original review) is to use a properly constructed diamond — as // Suggested replacement pattern:
function testRevert_FailsToSetChainIdToAllBridgeChainIdIfNotInitialized() public {
LiFiDiamond testDiamond = createDiamond(USER_DIAMOND_OWNER, USER_PAUSER);
// add facet without calling initAllBridge
// ...
vm.startPrank(USER_DIAMOND_OWNER);
vm.expectRevert(NotInitialized.selector);
AllBridgeFacet(address(testDiamond)).setChainIdToAllBridgeChainId(chainIdConfigs);
vm.stopPrank();
}To resolve: Either apply this pattern to the two ❌ Finding #3 — Still OpenNo test for The NatSpec documents: "Re-initialization overwrites the provided mappings and leaves the rest untouched." This is a non-trivial semantic — a second call with a subset of mappings updates those specific entries while leaving other mappings intact (it is NOT a full reset). No test verifies this. The 4 tests added in To resolve: Add a test that:
Or post an explicit acceptance comment if the team prefers to rely on the NatSpec documentation alone.
|
![lifi-qa-agent[bot]](https://avatars.githubusercontent.com/in/3411599?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes on 4 items — each requires either a code fix or an explicit acceptance comment with justification before this review is considered complete.
| # | Severity | Type | Issue / File |
|---|---|---|---|
| 1 | 🟢 Low | Security | initAllBridge / setChainIdToAllBridgeChainId accept allBridgeChainId = 0, silently producing an unmapped entry |
| 2 | 🟢 Low | Test gap | src/Facets/AllBridgeFacet.sol — fragile vm.startPrank(address(0)) owner hack in NotInitialized tests |
| 3 | 🟢 Low | Test gap | test/solidity/Facets/AllBridgeFacet.t.sol — no test for initAllBridge re-initialization (partial-overlap semantics) |
| 4 | 🟢 Low | Process | requires-types label — confirm ABI type regeneration step is in merge checklist |
1. [Low] allBridgeChainId = 0 not validated in setters
Neither initAllBridge nor setChainIdToAllBridgeChainId guards against allBridgeChainId == 0 in the loop body. A zero value is indistinguishable from an unmapped entry: _getAllBridgeChainId would revert UnsupportedAllBridgeChainId, silently deleting the chain without emitting ChainIdToAllBridgeChainIdUnset. The off-chain proposeAllBridgeChainIdMappings.ts script validates > 0, but there is no on-chain guard. Add if (allBridgeChainId == 0) revert InvalidConfig(); inside both loops.
File: src/Facets/AllBridgeFacet.sol
2. [Low] Test gap — fragile address(0) owner in NotInitialized tests
testRevert_FailsToSetChainIdToAllBridgeChainIdIfNotInitialized and its unset counterpart prank as address(0) to pass LibDiamond.enforceIsContractOwner() on a freshly deployed standalone facet (where diamond contractOwner defaults to zero). The test reaches and asserts NotInitialized correctly, but the setup is fragile: if LibDiamond ever initialises contractOwner to a non-zero default, these tests break at the wrong revert. Replace with a properly constructed diamond that has a real owner, call the setter without first calling initAllBridge.
File: test/solidity/Facets/AllBridgeFacet.t.sol
3. [Low] Test gap — initAllBridge re-initialization not tested
initAllBridge can be called multiple times by the owner; its NatSpec documents "Re-initialization overwrites the provided mappings and leaves the rest untouched." No test covers this: a call with a subset of mappings should overwrite those entries and leave the others unchanged. Add a test that calls initAllBridge twice with partially overlapping configs and asserts the merge semantics.
File: test/solidity/Facets/AllBridgeFacet.t.sol
4. [Low] Process — type regeneration for requires-types
The requires-types label signals that the ABI change (new external functions + pure→view on _getAllBridgeChainId) requires downstream TypeScript/ABI type regeneration. Confirm this is handled in the merge process and remove the label once complete.
💡 Once you've addressed the items above, re-apply the "Agent Review Request" label to trigger an automated re-review.
… (EXSC-650) Enforce the reserved-zero sentinel in initAllBridge and setChainIdToAllBridgeChainId: a zero chainId or allBridgeChainId now reverts InvalidConfig instead of silently writing a mapping that _getAllBridgeChainId treats as unmapped. Adds init/set regression tests for both zero fields. Addresses CodeRabbit review on PR #2075. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Re-review of post-review commit ca77b1d — Finding #1 resolved; 2 of the original 4 items remain open.
| # | Severity | Type | Issue / File |
|---|---|---|---|
| 2 | 🟢 Low | Test quality | test/solidity/Facets/AllBridgeFacet.t.sol — fragile address(0) owner pattern in NotInitialized revert tests |
| 3 | 🟢 Low | Test gap | test/solidity/Facets/AllBridgeFacet.t.sol — no re-initialization test for initAllBridge |
2. [Low] Test quality — fragile address(0) owner in NotInitialized revert tests
testRevert_FailsToSetChainIdToAllBridgeChainIdIfNotInitialized and testRevert_FailsToUnsetChainIdToAllBridgeChainIdIfNotInitialized both deploy a standalone TestAllBridgeFacet and prank as address(0) to satisfy LibDiamond.enforceIsContractOwner() (which works because fresh diamond storage defaults contractOwner to address(0)). The ca77b1d commit extended this same pattern to the two new zero-field tests. The tests are functionally correct today but will silently test the wrong revert if LibDiamond is ever updated to initialise contractOwner to a non-zero value. The recommended fix is to use a properly constructed diamond with createDiamond(USER_DIAMOND_OWNER, USER_PAUSER) (as test_InitAllBridge already does) and call the setter/unsetter without first calling initAllBridge. Alternatively, post an explicit acceptance comment explaining why the address(0) approach is preferred.
3. [Low] Test gap — no re-initialization test for initAllBridge
The NatSpec documents that a second initAllBridge call overwrites the provided mappings while leaving others untouched (partial overwrite, not a full reset). No test verifies this semantic. The 4 new tests in ca77b1d all cover zero-field validation, not re-init behaviour. Add a test that calls initAllBridge twice with overlapping configs and asserts that (a) the second call's entries take effect and (b) entries from the first call absent from the second call are preserved. Alternatively, post an explicit acceptance comment if the team prefers to rely on the NatSpec and code clarity alone.
💡 Once you've addressed the items above, re-apply the "Agent Review Request" label to trigger an automated re-review.
3 participants
Which Linear task belongs to this PR?
EXSC-650 — sub-issue of EXSC-610 (Stellar facet adjustments), sibling of EXSC-616 (Polymer).
Why did I implement it this way?
Daniela needs
AllBridgeFaceton staging with Stellar as a destination so she can drive the any→Stellar flow. Two things were needed:1. Stellar support. AllBridge identifies chains by its own internal IDs; the facet translates
bridgeData.destinationChainId→ AllBridge chain ID. Stellar isallBridgeChainId 7(keySRB/Soroban), taken from AllBridge's owntoken-infoendpoint (it fills the gap at 7 in the existing 1–17 sequence; 15 = Algorand, 16 = Stacks). Stellar is non-EVM, so it rides the facet's existing non-EVM path (receiver == NON_EVM_ADDRESS, real recipient as a non-zerobytes32inAllBridgeData.recipient) — no bridge-logic change, only the chain-ID entry.2. Dynamic mapping (the "while we're here"). The mapping was a hardcoded
if/elsein_getAllBridgeChainId, so every new chain meant a facet redeploy + audit. I moved it to an owner-updatable diamond-storage mapping, mirroring exactly what EXSC-254 / PR #1847 did forPolymerCCTPFacet:initAllBridge(seeded in the diamond cut, selector excluded from the facet),setChainIdToAllBridgeChainId,unsetChainIdToAllBridgeChainId,getChainIdToAllBridgeChainId,NotInitializedguard,UnsupportedAllBridgeChainIdon unmapped. Values live inconfig/allbridge.jsonundermappings. Version bumped 2.1.2 → 2.2.0 (MINOR: new external functions + new supported network), matching the Polymer precedent.One deliberate deviation from the Polymer pattern: no
+1storage offset. AllBridge chain IDs start at 1 (Ethereum), so a stored0already means "unmapped" — the offset Polymer needs (its Ethereum domain is0) would be noise here. Documented in theStoragestruct.config/allbridge.jsonandscript/tasks/proposeAllBridgeChainIdMappings.ts(production mapping sync across chains, adapted from the Polymer analog) round it out.Testing. 34 Solidity tests pass, including the full admin suite (owner-gating,
NotInitialized, set/unset/get, init event) and the Stellar mapping assertion. Note: an on-fork live any→Stellar funds test isn't feasible — AllBridge's router only registered the Stellar corridor after this suite's pinned fork block, and its destination-token registry can't be reproduced on a fork (verified:getTransactionCost(7)returns 0 at the pinned block, and swapAndBridge revertsBridge: unknown chain or tokenwith a synthetic Stellar token). The non-EVM funds path is covered by the existing Solana fork test (identical code path); end-to-end any→Stellar is the staging QA step.CalldataVerificationFacetdoes not decode AllBridge calldata, and the entrypoint/AllBridgeDatasignatures are unchanged, so no CVF bump and no clear-signing regeneration are needed.Scope / follow-ups. This PR is the code change. Staging deploy is next (I can run
deploy-contract, or Daniela can). Production rollout requires an audit first (facet logic change) — separate follow-up.Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)