feat(EXP-428): add SupersetFacet v1.0.0 [SupersetFacet v1.0.0, ISupersetHubPoolManager v1.0.0, ISupersetSpokePoolManager v1.0.0]#1865
Open
gvladika wants to merge 48 commits into
Open
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 SupersetFacet (contract, interfaces, admin mappings) enabling hub/spoke Superset pool-manager swaps with LayerZero delivery, plus config, deploy/update scripts, staging manifests, docs, a demo CLI, and comprehensive Foundry tests. ChangesSupersetFacet Cross-Chain Bridge
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 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)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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: 4
🧹 Nitpick comments (3)
script/demoScripts/demoSuperset.ts (1)
239-242: ⚡ Quick winPre-swap path is hardcoded to Base chain id.
The
8453literal makes the pre-swap branch fragile if anotherpreSwapscenario is added later. Derive chain id from the active client/scenario (or explicitly guard and fail fast whensourceChain !== 'base').🤖 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/demoScripts/demoSuperset.ts` around lines 239 - 242, The pre-swap uses a hardcoded chain id (8453) in the call to getUniswapDataERC20toExactERC20 which makes the preSwap branch fragile; replace the literal with a derived chain id from the active scenario/client (e.g., use the scenario's sourceChainId or client.chainId) when constructing srcSwap (refer to srcSwap, getUniswapDataERC20toExactERC20, ADDRESS_UNISWAP_BASE, ps.fromToken), or add an explicit guard that throws/fails fast if the scenario's sourceChain is not the expected 'base' before calling getUniswapDataERC20toExactERC20. Ensure the change uses the derived variable everywhere the literal was used so future scenarios don't break.test/solidity/Facets/SupersetFacet.t.sol (2)
40-40: 💤 Low valueConsider adding an inline comment explaining the test-only inputToken extraction.
The extraction
address(uint160(uint256(bytes32(_path[0:32]))))works because the hub test at line 421 embeds a raw address in the first 32 bytes of_path, whereas the real Superset hub expects omniTokenIds. An inline comment here would clarify this test-only shortcut without requiring readers to jump to line 420.🤖 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/solidity/Facets/SupersetFacet.t.sol` at line 40, Add a short inline comment next to the inputToken extraction (address inputToken = address(uint160(uint256(bytes32(_path[0:32]))));) explaining this is a test-only shortcut: the test embeds a raw EVM address in the first 32 bytes of _path (see the hub test that constructs _path) whereas the real Superset hub uses omniTokenIds, so this extraction is only valid for the test harness and not production.
170-174: ⚖️ Poor tradeoffConsider adding a unit test for the
amountOutMinadjustment inswapAndStartBridgeTokensViaSuperset.The facet recalculates
amountOutMinas(minAmount * amountOutMinPercent) / 1e18after the source-side swap to propagate positive slippage to the destination floor (SupersetFacet.sol lines 167-172). This calculation is not exercised by unit tests due to the overridden test here. While the demo script provides end-to-end coverage, a focused unit test would increase confidence in this critical slippage logic without relying on fork liquidity or integration complexity.One approach: mock the swap outcome by directly setting
bridgeData.minAmountto a post-swap value and asserting the adjustedamountOutMinforwarded to the pool manager matches the expected calculation.🤖 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/solidity/Facets/SupersetFacet.t.sol` around lines 170 - 174, Add a unit test exercising the amountOutMin adjustment in swapAndStartBridgeTokensViaSuperset: construct a bridgeData with a known minAmount, set the facet’s amountOutMinPercent to a known value, simulate/force the post-swap minAmount (e.g., by directly setting bridgeData.minAmount or mocking the swap result), call swapAndStartBridgeTokensViaSuperset, then assert that the value forwarded to the pool manager equals (bridgeData.minAmount * amountOutMinPercent) / 1e18; target symbols: swapAndStartBridgeTokensViaSuperset, SupersetFacet.sol, amountOutMinPercent, bridgeData.minAmount and the pool manager call that receives the adjusted amount.
🤖 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 `@deployments/arbitrumsepolia.json`:
- Around line 23-24: Update the stale flat-map entry for LiFiIntentEscrowFacet
by replacing the old address value for "LiFiIntentEscrowFacet" with the upgraded
address 0x0629DBbF0Cdd9042a0Ce85Ac3d6744A536f405C4 so it matches the recorded
version (1.1.1) in the diamond JSON; locate the "LiFiIntentEscrowFacet" key in
the deployments JSON map and swap the value to the new address, then save and
run any config validation/formatting checks.
In `@docs/SupersetFacet.md`:
- Around line 77-80: The docs incorrectly instruct hub-origin callers to set
SupersetData.refundAddress to address(0) while the facet's validation
(_validateSupersetData) rejects zero addresses; update docs/SupersetFacet.md to
require a non-zero refundAddress for both hub and spoke flows (remove the
suggestion to use address(0)), and likewise stop recommending empty string for
SupersetData.options for hub-origin quotes so the written guidance matches the
_validateSupersetData behavior.
In `@script/demoScripts/demoSuperset.ts`:
- Around line 84-87: The hardcoded addresses (receiver, refundAddress,
fallbackEoA) in demoSuperset.ts should be replaced so demos don't misroute
funds: change these to default to the transaction caller/deployer or read from
environment/CLI inputs (e.g., process.env.DESTINATION or a parsed CLI arg) and
validate they're present; update the code paths where receiver, refundAddress,
and fallbackEoA are set (also the repeated block around the other occurrences at
the commented range) to throw or prompt if no valid address is provided, and
keep deadlineSeconds unchanged; ensure type casting to Address remains but uses
the runtime-provided values instead of repository constants.
In `@src/Facets/SupersetFacet.sol`:
- Around line 161-181: The _validateSupersetData function must reject an empty
trade path early: add a check in _validateSupersetData (before forwarding to
pool manager and ideally near the top of the function) that _supersetData.path
is non-empty (e.g. require/_supersetData.path.length != 0) and revert
InvalidConfig() when empty so callers with empty paths are rejected immediately;
keep this alongside the existing refundAddress, fallbackEoA, and lzFee checks.
---
Nitpick comments:
In `@script/demoScripts/demoSuperset.ts`:
- Around line 239-242: The pre-swap uses a hardcoded chain id (8453) in the call
to getUniswapDataERC20toExactERC20 which makes the preSwap branch fragile;
replace the literal with a derived chain id from the active scenario/client
(e.g., use the scenario's sourceChainId or client.chainId) when constructing
srcSwap (refer to srcSwap, getUniswapDataERC20toExactERC20,
ADDRESS_UNISWAP_BASE, ps.fromToken), or add an explicit guard that throws/fails
fast if the scenario's sourceChain is not the expected 'base' before calling
getUniswapDataERC20toExactERC20. Ensure the change uses the derived variable
everywhere the literal was used so future scenarios don't break.
In `@test/solidity/Facets/SupersetFacet.t.sol`:
- Line 40: Add a short inline comment next to the inputToken extraction (address
inputToken = address(uint160(uint256(bytes32(_path[0:32]))));) explaining this
is a test-only shortcut: the test embeds a raw EVM address in the first 32 bytes
of _path (see the hub test that constructs _path) whereas the real Superset hub
uses omniTokenIds, so this extraction is only valid for the test harness and not
production.
- Around line 170-174: Add a unit test exercising the amountOutMin adjustment in
swapAndStartBridgeTokensViaSuperset: construct a bridgeData with a known
minAmount, set the facet’s amountOutMinPercent to a known value, simulate/force
the post-swap minAmount (e.g., by directly setting bridgeData.minAmount or
mocking the swap result), call swapAndStartBridgeTokensViaSuperset, then assert
that the value forwarded to the pool manager equals (bridgeData.minAmount *
amountOutMinPercent) / 1e18; target symbols:
swapAndStartBridgeTokensViaSuperset, SupersetFacet.sol, amountOutMinPercent,
bridgeData.minAmount and the pool manager call that receives the adjusted
amount.
🪄 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: eb385f0c-491c-4491-b21a-a4e707532ed5
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
config/superset.jsondeployments/arbitrum.diamond.staging.jsondeployments/arbitrum.staging.jsondeployments/arbitrumsepolia.diamond.jsondeployments/arbitrumsepolia.jsondeployments/base.diamond.staging.jsondeployments/base.staging.jsondocs/SupersetFacet.mdscript/demoScripts/demoSuperset.tsscript/deploy/facets/DeploySupersetFacet.s.solscript/deploy/facets/UpdateSupersetFacet.s.solscript/deploy/resources/deployRequirements.jsonsrc/Facets/SupersetFacet.solsrc/Interfaces/ISupersetHubPoolManager.solsrc/Interfaces/ISupersetSpokePoolManager.soltest/solidity/Facets/SupersetFacet.t.sol
Contributor
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)
src/Facets/SupersetFacet.sol (1)
233-237:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard against
amountOutMinPercent == 0on the swap path.In the swap branch the backend-supplied
amountOutMinis overwritten byminAmount * amountOutMinPercent / 1e18. IfamountOutMinPercentis0(or unset), the destination slippage floor silently becomes0, removing slippage protection on the cross-chain swap. Unlike the direct-bridge path, there is no other way to set a floor here, so a zero percent can't be intentional.🛡️ Proposed guard
+ if (_supersetData.amountOutMinPercent == 0) { + revert InvalidConfig(); + } + // Adjust destination slippage floor for positive source-side slippage SupersetData memory modifiedSupersetData = _supersetData; modifiedSupersetData.amountOutMin = (_bridgeData.minAmount * _supersetData.amountOutMinPercent) / 1e18;As per coding guidelines: "Validate all external inputs and function parameters."
🤖 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/Facets/SupersetFacet.sol` around lines 233 - 237, Validate that _supersetData.amountOutMinPercent is non-zero before overwriting modifiedSupersetData.amountOutMin: add a guard (e.g., require(_supersetData.amountOutMinPercent > 0, "amountOutMinPercent must be >0")) or, if you prefer non-reverting behavior, fall back to the original _supersetData.amountOutMin when amountOutMinPercent == 0; update the assignment that sets modifiedSupersetData.amountOutMin = (_bridgeData.minAmount * _supersetData.amountOutMinPercent) / 1e18 to only run when amountOutMinPercent > 0 and reference modifiedSupersetData, _supersetData.amountOutMinPercent, and _bridgeData.minAmount to locate the change.
🤖 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 `@deployments/base.diamond.staging.json`:
- Around line 80-87: The manifest contains two mappings for the same Name
"SupersetFacet" under keys "0xdAB548Fd9075b7dABf720c2335B61ED8061b9071" and
"0x8151776fFCefDE00b06BCC9688638b5049855Bee"; remove the duplicate so there is a
single SupersetFacet mapping—keep the intended active Base staging address
(choose one of the two addresses above) and delete the other entry, then
validate the JSON and any deployment/audit references that resolve Name→address
to ensure they point to the retained address.
---
Outside diff comments:
In `@src/Facets/SupersetFacet.sol`:
- Around line 233-237: Validate that _supersetData.amountOutMinPercent is
non-zero before overwriting modifiedSupersetData.amountOutMin: add a guard
(e.g., require(_supersetData.amountOutMinPercent > 0, "amountOutMinPercent must
be >0")) or, if you prefer non-reverting behavior, fall back to the original
_supersetData.amountOutMin when amountOutMinPercent == 0; update the assignment
that sets modifiedSupersetData.amountOutMin = (_bridgeData.minAmount *
_supersetData.amountOutMinPercent) / 1e18 to only run when amountOutMinPercent >
0 and reference modifiedSupersetData, _supersetData.amountOutMinPercent, and
_bridgeData.minAmount to locate the change.
🪄 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: c818364d-6433-420a-9a34-1e08baf682b4
📒 Files selected for processing (8)
deployments/arbitrum.diamond.staging.jsondeployments/arbitrum.staging.jsondeployments/base.diamond.staging.jsondeployments/base.staging.jsondocs/SupersetFacet.mdscript/demoScripts/demoSuperset.tssrc/Facets/SupersetFacet.soltest/solidity/Facets/SupersetFacet.t.sol
✅ Files skipped from review due to trivial changes (2)
- deployments/arbitrum.staging.json
- docs/SupersetFacet.md
🚧 Files skipped from review as they are similar to previous changes (3)
- deployments/base.staging.json
- script/demoScripts/demoSuperset.ts
- deployments/arbitrum.diamond.staging.json
Collaborator
🤖 GitHub Action: Security Alerts Review 🔍🟢 Dismissed Security Alerts with Comments 🟢 View Alert - File: ✅ No unresolved security alerts! 🎉 |
lifi-action-bot
changed the title
gvladika
changed the title
Collaborator
Test Coverage ReportLine Coverage: 90.13% (3317 / 3680 lines) |
Adds a thin Diamond facet that bridges stablecoins via Superset's hub-and-spoke virtual pools (hub on Arbitrum; spokes on Base/Unichain). Mirrors the LayerSwapFacet PR (#1711) layout: facet + interface + tests + deploy/update scripts + config + docs + demo + deployRequirements entry. Notes: - Built against Superset's `develop` branch ABI (recipient/refundAddress/ fallbackEoA arguments on `multiHopSwapWithOutputChain`). Not yet on mainnet; do not deploy until Superset ships the redeploy. - ERC-20 + native ETH (facet wraps to WETH on source). - Positive-slippage handled via SupersetData.amountOutMinPercent per [CONV:FACET-REQS]. - Non-EVM destinations rejected (Superset has no non-EVM spokes). - 24/24 unit tests passing against a mock spoke pool manager.
… branch Previously the facet only supported spoke chains (Base, Unichain) and treated Arbitrum (hub) as out of scope. That meant bridging FROM Arbitrum would have required a second facet for the same underlying protocol — wasteful given the ABIs only differ by two parameters. Replace `spokePoolManager` with a generic `POOL_MANAGER` + `IS_HUB` immutable. `IS_HUB` is derived at construction from `block.chainid == 42161`, so the same facet bytecode picks the matching Superset entrypoint: - Spoke (Base/Unichain): SpokePoolManager.multiHopSwapWithOutputChain (9 args) - Hub (Arbitrum): HubPoolManager.multiHopSwapWithOutputChain (7 args) `SupersetData.refundAddress` and `SupersetData.options` are ignored on the hub branch (the hub processes failures synchronously and has no source → hub LZ leg). Storage layout is identical across deployments. - Added ISupersetHubPoolManager interface for the 7-arg ABI - config/superset.json: renamed `spokePoolManager` -> `poolManager`, added arbitrum - deployRequirements.json: matching key rename - Tests: 26/26 passing; added hub-branch forwarding test and IS_HUB chainId test
Demo no longer imports Superset addresses, ABIs, or path-encoding logic. Top-of-file constants hold the route and the values retrieved out-of-band (path, amountOutMin, lzFee, options). assertConfigured() rejects unset placeholders before submit. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Superset does not support native as a source asset, so the facet now
rejects it via the shared noNativeAsset modifier on both entry points.
- Remove WRAPPED_NATIVE immutable, IWETH interface, wrap branch in
_startBridge.
- Simplify constructor to a single _poolManager argument; update deploy
script and deployRequirements.json accordingly.
- Drop native test helpers; override testBase_CanBridgeNativeTokens and
testBase_CanSwapAndBridgeNativeTokens per rule 400 (intentionally
unsupported). Add testRevert_NativeAssetNotSupported_{Bridge,SwapAndBridge}.
- Document the constraint in docs/SupersetFacet.md.
forge test --match-contract SupersetFacetTest: 26/26 pass.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…dress LZ fees are a quote that drifts with gas between quote and execution, so the backend overpays msg.value on essentially every call — making a native refund the normal case, not an edge. Refunding to msg.sender strands those funds when the call is routed through Permit2Proxy (its msg.sender, with no sweep). Route refundExcessNative and the _depositAndSwap leftover receiver to SupersetData.refundAddress instead, which is a source-chain user-controlled address by Superset's design. Require it non-zero (InvalidConfig), and also reject a zero fallbackEoA. forge test --match-contract SupersetFacetTest: 29/29 pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Move the msg.value >= lzFee check into _validateSupersetData so both entry points enforce it with a clean InsufficientNativeValue revert, instead of swapAndStart relying on _depositAndSwap's reserve to panic on underflow. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Use named-arg syntax for both pool-manager calls (hub 7-arg, spoke 9-arg). - Drop non-EVM-receiver guard + LiFiData inheritance (EVM-only facet, matches AcrossFacetV3/LayerSwapFacet pattern). - Drop the 67-byte path-length pre-check (protocol validates path itself). - Collapse InvalidFallbackEoA into the generic InvalidConfig. - Remove vestigial @dev notes and unused solhint disables. forge test --match-contract SupersetFacetTest: 27/27 pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Single SCENARIOS record keyed by name; --scenario picks one. Three scenarios prefilled: - base-to-unichain (spoke → hub → spoke; known-good tx in header) - arbitrum-to-base (hub → spoke; deploy pending on Arb staging) - base-to-unichain-w-swap (pre-swap stub; paste DEX calldata before run) SHARED block for receiver / refundAddress / fallbackEoA / deadline. Pre-swap path dispatches to swapAndStartBridgeTokensViaSuperset. Header comment slots track per-scenario tx hashes after each run. Also drop a stale `[CONV:FACET-REQS]` source-comment from the facet. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ap V2 Mirrors the pattern in demoAcrossV3 / demoAcrossV4 / demoEco / demoGarden: the scenario specifies only the ETH input amount, and the swap router call data, slippage floor, and deadline are computed on each run so the demo stays executable without re-pasting calldata. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…a shared helper Drop the inline Uniswap V2 exact-input plumbing (and the chain-aware WETH + ABI constants it required) in favour of the existing getUniswapDataERC20toExactERC20 helper used by every other source-swap demo (Across, Eco, Garden). Exact-output semantics make bridgeData.minAmount known upfront — no need to plumb minAmountOut back through the helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
All three scenarios (base-to-unichain, arbitrum-to-base, base-to-unichain-w-swap) now have recorded source/destination tx hashes in the header. fallbackEoA moved to a clean burner EOA so it passes the code.length==0 check on Arbitrum where the caller wallet is EIP-7702 delegated. Includes staging deployment artifacts for the Arb cut and the Base redeploy of SupersetFacet@1.0.0. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…et.md:135) The section claimed the facet rejects bridgeData.receiver == NON_EVM_ADDRESS with InvalidNonEVMReceiver, but no such check exists in the facet. Removing rather than rewording — the constraint isn't enforced on-chain today and shouldn't be documented as a permanent limitation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…upersetFacet.t.sol:274) Adds testRevert_DestinationCallNotSupported to verify the doesNotContainDestinationCalls modifier reverts with InformationMismatch when bridgeData.hasDestinationCall is true. Closes a coverage gap flagged by CodeRabbit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Switches SupersetFacetTest from a local-fork mock to a Base mainnet fork
backed by Superset's deployed SpokePoolManager. Most assertions become
implicit: if any arg were wrong (path, fallbackEoA, deadline, options)
the live spoke would revert. The hub-branch test keeps its mock since a
single test file can only attach to one fork chain.
- customRpcUrlForForking = "ETH_NODE_URI_BASE", block pinned at 46595000
- real omniPath (omniId 2 → fee 3000 → omniId 3) matching demoSuperset
- override setDefaultSwapDataSingle{DAItoUSDC,DAItoETH} to stub swap data
without querying getAmountsIn (Base V2 lacks DAI/USDC liquidity)
- override testBase_CanSwapAndBridgeTokens (no V2 happy path on Base)
- replace mockSpoke.lastCall() assertions with USDC balance + event check
- refund-excess test uses assertGe (LayerZero may also refund unused fee)
All 28 tests pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ation Owner-managed diamond storage (mirrors PolymerCCTPFacet v2.1.0) lets new spokes onboard without redeploying the facet. Validates that backend-supplied toEid matches the LZ endpoint configured for bridgeData.destinationChainId, making the off-chain pair structurally consistent with on-chain config. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CR says: docs say hub callers can set refundAddress=address(0), but _validateSupersetData rejects it unconditionally. Update guidance to require a non-zero refundAddress on every path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…cet.sol:293) CR says: _validateSupersetData should reject an empty path early instead of relying on a downstream revert from the pool manager. Added length check at the top of the function plus a focused test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ts/SupersetFacet.t.sol:40) CR says: extracting the inputToken from the first 32 bytes of _path is a test-only shortcut and should be commented so readers don't expect the real hub to behave that way. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…y/Facets/SupersetFacet.t.sol:170) CR says: the (minAmount * amountOutMinPercent) / 1e18 adjustment in swapAndStartBridgeTokensViaSuperset is not exercised by unit tests. Added a mock spoke + fixed-rate swap router so the math can be asserted without depending on Uniswap quotes at the fork block. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LayerZero never assigns EID 0 (v1 starts at 101, v2 at 30000), so we can store the raw value and treat 0 as 'unset' directly. The +1 pattern was inherited from PolymerCCTPFacet, where CCTP domain 0 (Ethereum) is a real domain; that constraint doesn't apply here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replaces the `for (...; ) { ...; unchecked { ++i; } }` gas-optimization
pattern with the standard `for (...; ++i)` form. The two loops here are
owner-only admin functions that iterate over a small ChainIdConfig array
(currently 3 entries), so the savings don't justify the readability hit.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The original @dev pointed at https://github.com/superset-finance/virtual-pools, which is private and returns 404 for anyone outside the Superset org. Rephrased to reference Superset's virtual-pools contracts by name only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CR follow-up: docs claimed the toEid↔destinationChainId mismatch reverts with InformationMismatch, but the implementation uses InvalidConfig (the same generic error as the other validation reverts in the facet). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…persetFacet.sol:68) CR follow-up: original struct NatSpec said "receives amountIn if the swap fails" without qualification. That's spoke-branch behaviour; on a hub origin there is no async failure path, so Superset doesn't use refundAddress at all. Reworded to call out which uses apply on which branch and why the facet still requires it to be non-zero on both. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ersetFacet.t.sol:613) CR follow-up: test_CanSetChainIdToEid only exercised the "add new entry" path. Extended it to overwrite the same chainId with a different EID and assert the new value (+ emit) — closes the coverage gap on the update branch without a separate test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
gvladika
force-pushed
the
worktree-superset-tech-assessment
branch
from
306bcd3 to
f31fe09
Compare
lifi-action-bot
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
script/deploy/facets/DeploySupersetFacet.s.sol (1)
13-19: DeployScriptBase already appends constructor args during deployment
run()passing onlytype(SupersetFacet).creationCodeintodeploy(...)is correct:script/deploy/facets/utils/DeployScriptBase.sol’sdeploy(bytes)computesconstructorArgs = getConstructorArgs()internally and deploysbytes.concat(creationCode, constructorArgs).
Minor:run()computesconstructorArgsfor the return value, sogetConstructorArgs()is executed twice; avoid the redundant call if unnecessary.🤖 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/deploy/facets/DeploySupersetFacet.s.sol` around lines 13 - 19, The run() currently calls getConstructorArgs() and DeployScriptBase.deploy() also calls getConstructorArgs(), causing duplicate execution; compute constructorArgs once and pass it into deploy to avoid the redundant call. Change DeployScriptBase to expose an overload deploy(bytes creationCode, bytes constructorArgs) (or modify its existing deploy to accept constructorArgs) that uses the provided constructorArgs instead of calling getConstructorArgs() internally, then in DeploySupersetFacet.run() do constructorArgs = getConstructorArgs(); deployed = SupersetFacet(deploy(type(SupersetFacet).creationCode, constructorArgs)); so getConstructorArgs() runs only once.
🤖 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/deploy/facets/UpdateSupersetFacet.s.sol`:
- Around line 18-37: getExcludes() currently returns an array that excludes
SupersetFacet.initSuperset, which makes initSuperset non-callable after the cut
and contradicts tests/docs that treat initSuperset as an owner/admin diamond
method; to fix, stop excluding that selector from getExcludes() so initSuperset
remains exposed (remove the excludes[0] assignment and return an empty bytes4[]
or otherwise not include SupersetFacet.initSuperset), leaving getCallData() and
the initialization flow unchanged; alternatively, if you prefer the selector to
stay hidden, update the tests/docs to reflect that initSuperset is only runnable
during the cut.
---
Nitpick comments:
In `@script/deploy/facets/DeploySupersetFacet.s.sol`:
- Around line 13-19: The run() currently calls getConstructorArgs() and
DeployScriptBase.deploy() also calls getConstructorArgs(), causing duplicate
execution; compute constructorArgs once and pass it into deploy to avoid the
redundant call. Change DeployScriptBase to expose an overload deploy(bytes
creationCode, bytes constructorArgs) (or modify its existing deploy to accept
constructorArgs) that uses the provided constructorArgs instead of calling
getConstructorArgs() internally, then in DeploySupersetFacet.run() do
constructorArgs = getConstructorArgs(); deployed =
SupersetFacet(deploy(type(SupersetFacet).creationCode, constructorArgs)); so
getConstructorArgs() runs only once.
🪄 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: 96d5efcc-155f-4c72-9edd-adbae4fa527d
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
config/clearSigningProposal.jsonconfig/superset.jsondeployments/arbitrum.diamond.staging.jsondeployments/arbitrum.staging.jsondeployments/arbitrumsepolia.diamond.jsondeployments/arbitrumsepolia.jsondeployments/base.diamond.staging.jsondeployments/base.staging.jsondocs/SupersetFacet.mdscript/demoScripts/demoSuperset.tsscript/deploy/facets/DeploySupersetFacet.s.solscript/deploy/facets/UpdateSupersetFacet.s.solscript/deploy/resources/deployRequirements.jsonsrc/Facets/SupersetFacet.solsrc/Interfaces/ISupersetHubPoolManager.solsrc/Interfaces/ISupersetSpokePoolManager.soltest/solidity/Facets/SupersetFacet.t.sol
✅ Files skipped from review due to trivial changes (1)
- deployments/arbitrum.staging.json
🚧 Files skipped from review as they are similar to previous changes (8)
- script/deploy/resources/deployRequirements.json
- deployments/base.staging.json
- src/Interfaces/ISupersetHubPoolManager.sol
- src/Interfaces/ISupersetSpokePoolManager.sol
- config/clearSigningProposal.json
- config/superset.json
- deployments/arbitrumsepolia.diamond.json
- src/Facets/SupersetFacet.sol
…tFacet.md:43) CodeRabbit thread on UpdateSupersetFacet.s.sol:37 read the `getExcludes()` line as a bug. It isn't — every facet with an init function in this repo does exactly the same thing (PolymerCCTPFacet, OptimismBridgeFacet, MegaETHBridgeFacet, CelerCircleBridgeFacet, HopFacet). Init runs atomically as the diamondCut init payload and the selector is intentionally not registered post-cut; ongoing changes go through setChainIdToEid. The doc previously didn't spell that out, so the bullet now states the one-shot semantics explicitly and points at setChainIdToEid as the post-deploy update path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mirooon
reviewed
Jun 1, 2026
…acet.sol:246) mirooon flagged that Superset only enforces `deadline` on the hub via `HubSwapRouterWithFee.checkDeadline`, after the source → hub LayerZero message is already paid for. A past or zero deadline would therefore burn the user's `lzFee` before the hub rejects the request. Other facets (UnitFacet, NEARIntentsFacet, EcoFacet) already perform this source-side check; SupersetFacet now does too. New DeadlineExpired error keeps the revert reason precise. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LiFi staging runs on mainnets, not testnets, so the arbitrumsepolia and basesepolia poolManager entries had no path to being deployed against. Removing them avoids the documentation drift mirooon flagged (poolManager entries without matching mappings) and keeps the config aligned with the chains we actually deploy SupersetFacet on. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…SupersetFacet.sol:134) mirooon flagged that an indexer subscribed to ChainIdToEidSet would miss the seed entries written during initSuperset, because init only emitted the batch SupersetChainMappingsInitialized event. Now the loop emits ChainIdToEidSet per entry alongside writing the mapping, followed by the batch event as a transaction-level confirmation. One event signal covers both seed and ongoing updates. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ersetFacet.sol:87) mirooon flagged that the SupersetData struct exposes both amountOutMin and amountOutMinPercent but each entry point only uses one, and the demo's swap scenario was passing a non-zero amountOutMin that the facet then discarded. - NatSpec on each field now states which entry point reads it and works through a concrete example for the percent. - docs/SupersetFacet.md gains a "Positive Slippage Handling" section with the same example. - demoSuperset.ts swap scenario sets amountOutMin to 0 so nobody copy-pastes a dead value, and the assertConfigured() guard now matches that semantics (require amountOutMin for bridge-only, amountOutMinPercent for swap+bridge). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t.md:99) mirooon flagged that the raw code.length != 0 check rejects EIP-7702 delegated EOAs, which most code treats as EOAs (LibAsset.isContract returns false for them). Looking at Superset's SwapDelivery.sol the check is identical there, so the facet must match — switching to LibAsset.isContract would just push the revert downstream. Updated the fallbackEoA section to call this out explicitly so backends serving 7702 users know they need to supply a separate pure-EOA address rather than the delegated one. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…cets/SupersetFacet.sol:88) mirooon flagged that the SupersetData struct had two slippage fields that overlapped, and that amountOutMinPercent was named like a percentage but actually encoded a price+decimal multiplier scaled by 1e18 (the demo's 1.348e15 makes no sense as a percentage). The two discussion comments boiled down to the same root: encoding the positive-slippage adjustment as a backend-supplied multiplier was the wrong shape. Drops amountOutMinPercent. swapAndStartBridgeTokensViaSuperset now saves the pre-swap bridgeData.minAmount (the swap floor) and after _depositAndSwap scales amountOutMin by actualPostSwap / preSwapFloor. _depositAndSwap reverts on a swap that misses the floor so the ratio is always >= 1; validateBridgeData rejects minAmount == 0 so the divisor is always > 0. Equivalent math to the multiplier shape but both fields are now self-documenting absolute amounts and there's no cross-decimal overflow class (the ratio uses same-decimal pairs). Tests, demo, docs, clear-signing updated accordingly. Test now sets an explicit swap floor of 100 DAI and an actual swap output of 200 DAI to exercise the 2× scaling path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ersetFacet.sol:107) Every other check in _validateSupersetData reverts with the generic InvalidConfig (empty path, zero refundAddress, code-bearing fallbackEoA, toEid mismatch). Following the same precedent set earlier in this PR (InformationMismatch → InvalidConfig), the deadline check now reverts InvalidConfig too. Drops the facet-specific DeadlineExpired error declaration for consistency. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…upersetFacet.sol:88) One-line addition to the struct NatSpec so the constraint is visible at the field declaration, not just in docs/SupersetFacet.md. EIP-7702 delegated EOAs are rejected because the 23-byte delegation designator counts as code.length != 0. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…h-assessment # Conflicts: # config/clearSigningProposal.json
…page scaling Refreshes lzFee + options from a live PoolManagerMessagingQuoter quote (+20% buffer); derives amountOutMin from HubOmniTokensQuoterV2WithFee.quoteExactInputOmni with a 1% slippage floor. Restructures the swap+bridge scenario to use the exact-input Uniswap helper so actualPostSwap > declared minAmount and the facet's positive-slippage scaling path is actually exercised on-chain. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…sitive-slippage run Replaces the stale SupersetFacet entries on Arbitrum/Base staging diamonds with the redeployed 0x54AE… addresses (drops the obsolete 0x8151… entries). Updates demoSuperset.ts to use an off-band amountOutMin calibrated against the declared bridge floor (preSwap minAmountOut) so the facet's positive-slippage scaling produces an on-chain floor the hub can actually meet — see the linked 3-tx run in the docblock for verification. Comments stripped of any Superset-internal naming. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mirooon
reviewed
Jun 4, 2026
mirooon
reviewed
Jun 4, 2026
mirooon
reviewed
Jun 4, 2026
Adds a JSON-key reference (mirrors the existing LayerZero EIDs source pattern) pointing to Superset's official deployments manifest, since their public docs don't list mainnet contracts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduces a generic DeadlineExpired error in GenericErrors and uses it in SupersetFacet's deadline check, where InvalidConfig was misleading (the params were valid; the tx just sat in the mempool too long). Bumps GenericErrors @Custom:version to 1.0.4 and updates the SupersetFacet test that asserted the old InvalidConfig revert. EcoFacet/UnitFacet/ NEARIntentsFacet still use their own deadline reverts — left a TODO next to the new error to migrate them in a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mirooon
approved these changes
Jun 8, 2026
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 Linear task belongs to this PR?
EXP-428
Why did I implement it this way?
Adds
SupersetFacetv1.0.0 — a hub-and-spoke cross-chain integration with Superset Labs' virtual-pool DEX (hub on Arbitrum, spokes on Base and Unichain).A few design decisions worth calling out:
SpokePoolManager(9-argmultiHopSwapWithOutputChain) andHubPoolManager(7-arg version) differ in shape because the hub flow doesn't propagaterefundAddressoroptions. Rather than ship two facets, the constructor readsblock.chainidand sets anIS_HUBimmutable;_startBridgepicks the right call shape from the same facet code. This keeps one selector pair across all chains and avoids deploying two near-identical facets.refundAddressfrom_supersetData, notmsg.sender— and refunds are routine, not exceptional. ThelzFeefield is an off-chain LayerZero quote that drifts with destination-chain gas price between quote time and execution, so the backend deliberately overpaysmsg.valueby ~20% to absorb the drift — meaning a refund of the unused buffer happens on essentially every call, not just on failure. For Permit2 / smart-account flows the diamond'smsg.sendercan be the Permit2Proxy or a session-key wallet, neither of which is a recoverable destination. The facet therefore routes source-side native excess (the LZ buffer + any_depositAndSwapswap leftovers) to_supersetData.refundAddress— the same address Superset itself uses for its async failure refunds — so refund delivery is consistent end-to-end and the user actually receives the change.fallbackEoAvalidation. Both Superset (SwapDelivery.sol:30) and our facet rejectfallbackEoA.code.length != 0. We duplicate the check because a clean revert at the facet boundary is cheaper than walking throughsafeTransferFrom+ spoke validation. This catches EIP-7702 delegated EOAs at the source — which the demo run on Arbitrum confirmed is a real risk for accounts that have been smart-account-upgraded.SupersetFacetTestexercises the liveSpokePoolManagerat0x57C155a15a9CA0A6C1F759ac6988b4fCa3663Ea4, so arg-forwarding correctness is implicit (any wrong arg makes the real spoke revert). The hub-branch test still uses a mock — a single Foundry test file can only attach to one fork chain. Replacing that with an Arbitrum fork is a follow-up.End-to-end validated on mainnet: see the three recorded source/destination tx pairs in
script/demoScripts/demoSuperset.ts:7-13(Base→Unichain spoke source; Arbitrum→Base hub source; Base→Unichain with WETH→USDC pre-swap).Deferred follow-ups:
MockSupersetHubPoolManagerwith a real Arbitrum fork in a separate test file.Checklist before requesting a review
/pr-ready(local CodeRabbit) on this branch and resolved (or explicitly documented) all findings — see.agents/commands/pr-ready.mdChecklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)