feat(FraxFacet): add Frax HopV2 (LayerZero OFT) bridge facet (EXSC-382) [FraxFacet v1.0.0, IFraxHopV2 v1.0.0]#2048
Draft
0xDEnYO wants to merge 5 commits into
Draft
feat(FraxFacet): add Frax HopV2 (LayerZero OFT) bridge facet (EXSC-382) [FraxFacet v1.0.0, IFraxHopV2 v1.0.0]#20480xDEnYO wants to merge 5 commits into
0xDEnYO wants to merge 5 commits into
Conversation
… + deploy scripts (EXSC-382) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…+ clearSigning (EXSC-382) - swap entrypoint: reject stray msg.value on Tempo; require final swap output == sendingAssetId - Tempo: return unpulled fee-token remainder to refundRecipient (invariant holds across hop upgrades) - docs/FraxFacet.md; regenerate clearSigningProposal.json (both Frax entrypoints) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…C20-fee path (EXSC-382) - 29 Arbitrum-fork tests (real HopV2 + frxUSD self-OFT): dust floor+refund, excess-native refund, diamond-retains-nothing, oft/token mismatch, validation reverts, swap-output guard - 7 local Tempo tests via MockFraxHopV2Tempo (precompile TIP20 can't fork; rule 400): msg.value==0 guard, ERC20 fee pull, opted-in fee token, unused-fee sweep Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…o guard wording in docs (EXSC-382) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
the
requires-types
lifi-action-bot
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| /// @dev Contains the business logic for bridging via Frax HopV2 | ||
| /// @param _bridgeData The core information needed for bridging | ||
| /// @param _fraxData Data specific to Frax HopV2 | ||
| function _startBridge( |
| flooredAmount | ||
| ); | ||
|
|
||
| bytes32 recipient = bytes32(uint256(uint160(_bridgeData.receiver))); |
Contributor
|
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)
WalkthroughChangesFrax bridging integration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/Interfaces/IFraxHopV2.sol (1)
72-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the auxiliary interfaces into dedicated files.
Place
ITipFeeManagerandIFraxOFTinsrc/Interfaces/ITipFeeManager.solandsrc/Interfaces/IFraxOFT.sol, respectively.As per coding guidelines, “Solidity interfaces should be placed in separate files with the
Iprefix naming convention.”🤖 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/Interfaces/IFraxHopV2.sol` around lines 72 - 92, The auxiliary interfaces ITipFeeManager and IFraxOFT should not remain in IFraxHopV2.sol. Move each interface into its own dedicated file, src/Interfaces/ITipFeeManager.sol and src/Interfaces/IFraxOFT.sol, preserving their declarations and documentation, and update any imports or references to use the new locations.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 `@config/clearSigningProposal.json`:
- Around line 806-872: Update both Frax clear-signing entries to display
_fraxData.dstEid as the destination and _fraxData.refundRecipient as the refund
recipient, replacing the descriptive _bridgeData.destinationChainId presentation
where appropriate. Add visible field definitions for both _fraxData paths in
each entry, using suitable destination and address formatting while preserving
the existing bridge fields.
In `@docs/FraxFacet.md`:
- Around line 127-134: Update the “LiFi Data” documentation to clarify that
BridgeData is not strictly for analytics: explain that fields such as
sendingAssetId, receiver, and minAmount affect transfer behavior, while limiting
the analytics description to metadata such as transaction ID, integrator, and
referrer.
In `@script/demoScripts/demoFrax.ts`:
- Around line 243-283: Replace the warning-only sanity checks in the quote
mapping flow with validation that aborts before sending funds when any external
quote field mismatches expectations: oft, dstEid, recipient, amountLd,
approvalAddress, or transactionRequest.to. Derive the expected Hop target as
FRAX_HOP_SPOKE for both routes because Arbitrum is always the source, and
compare addresses and numeric values using appropriate normalized
representations. Preserve the existing API-derived values only after all
validations pass.
In `@src/Facets/FraxFacet.sol`:
- Line 229: Update the Tempo fee refund logic surrounding _sendViaTempo in
FraxFacet so the refund baseline accounts for the bridged token when feeToken
equals _bridgeData.sendingAssetId. Exclude _amount from the pre-snapshot or
otherwise track only the fee-funded deposit, preventing balanceOf(address(this))
- feeTokenBalanceBefore from underflowing after sendOFT transfers the bridged
tokens.
---
Nitpick comments:
In `@src/Interfaces/IFraxHopV2.sol`:
- Around line 72-92: The auxiliary interfaces ITipFeeManager and IFraxOFT should
not remain in IFraxHopV2.sol. Move each interface into its own dedicated file,
src/Interfaces/ITipFeeManager.sol and src/Interfaces/IFraxOFT.sol, preserving
their declarations and documentation, and update any imports or references to
use the new locations.
🪄 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: 93c6ffc8-add5-49b0-9067-b1b2bb107c25
📒 Files selected for processing (13)
config/clearSigningProposal.jsonconfig/frax.jsondocs/FraxFacet.mdscript/demoScripts/demoFrax.tsscript/deploy/facets/DeployFraxFacet.s.solscript/deploy/facets/UpdateFraxFacet.s.solscript/deploy/resources/deployRequirements.jsonscript/deploy/zksync/DeployFraxFacet.zksync.s.solscript/deploy/zksync/UpdateFraxFacet.zksync.s.solsrc/Facets/FraxFacet.solsrc/Interfaces/IFraxHopV2.soltest/solidity/Facets/FraxFacet.t.soltest/solidity/utils/MockFraxHopV2Tempo.sol
Comment on lines
+806
to
+872
| "startBridgeTokensViaFrax((bytes32 transactionId, string bridge, string integrator, address referrer, address sendingAssetId, address receiver, uint256 minAmount, uint256 destinationChainId, bool hasSourceSwaps, bool hasDestinationCall) _bridgeData, (address oft, uint32 dstEid, uint256 nativeFee, address refundRecipient) _fraxData)": { | ||
| "intent": "Bridge via Frax", | ||
| "interpolatedIntent": "Bridge {_bridgeData.minAmount} via Frax to chain {_bridgeData.destinationChainId} for {_bridgeData.receiver}", | ||
| "fields": [ | ||
| { | ||
| "path": "_bridgeData.minAmount", | ||
| "label": "Amount to Bridge", | ||
| "format": "tokenAmount", | ||
| "params": { | ||
| "tokenPath": "_bridgeData.sendingAssetId" | ||
| }, | ||
| "visible": "always" | ||
| }, | ||
| { | ||
| "path": "_bridgeData.destinationChainId", | ||
| "label": "Destination Chain", | ||
| "format": "raw", | ||
| "visible": "always" | ||
| }, | ||
| { | ||
| "path": "_bridgeData.receiver", | ||
| "label": "Recipient", | ||
| "format": "addressName", | ||
| "params": { | ||
| "types": [ | ||
| "eoa", | ||
| "contract" | ||
| ], | ||
| "sources": [ | ||
| "local", | ||
| "ens" | ||
| ] | ||
| }, | ||
| "visible": "always" | ||
| }, | ||
| { | ||
| "path": "_bridgeData.transactionId", | ||
| "label": "Transaction Id", | ||
| "visible": "never" | ||
| }, | ||
| { | ||
| "path": "_bridgeData.bridge", | ||
| "label": "Bridge", | ||
| "visible": "never" | ||
| }, | ||
| { | ||
| "path": "_bridgeData.integrator", | ||
| "label": "Integrator", | ||
| "visible": "never" | ||
| }, | ||
| { | ||
| "path": "_bridgeData.referrer", | ||
| "label": "Referrer", | ||
| "visible": "never" | ||
| }, | ||
| { | ||
| "path": "_bridgeData.hasSourceSwaps", | ||
| "label": "Has Source Swaps", | ||
| "visible": "never" | ||
| }, | ||
| { | ||
| "path": "_bridgeData.hasDestinationCall", | ||
| "label": "Has Destination Call", | ||
| "visible": "never" | ||
| } | ||
| ] | ||
| }, |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the reviewed config entry and related source/generator definitions.
git ls-files | rg '^config/clearSigningProposal\.json$|Frax|clearSigningProposal|signing proposal|clear signing' || true
echo '--- search for Frax-related clear signing definitions ---'
rg -n '"startBridgeTokensViaFrax|dstEid|refundRecipient|clearSigningProposal|Frax"' config src script .cursor -g '!**/dist/**' -g '!**/build/**' || true
echo '--- locate the reviewed block in config/clearSigningProposal.json ---'
rg -n -A40 -B20 '"startBridgeTokensViaFrax\(\(bytes32 transactionId, string bridge, string integrator, address referrer, address sendingAssetId, address receiver, uint256 minAmount, uint256 destinationChainId, bool hasSourceSwaps, bool hasDestinationCall\) _bridgeData, \(address oft, uint32 dstEid, uint256 nativeFee, address refundRecipient\) _fraxData\)"' config/clearSigningProposal.json || true
echo '--- find the generator/source that emits these descriptors ---'
fd -a 'clearSigningProposal*' . || true
fd -a '*signing*proposal*' . || true
fd -a '*Frax*' src script config . || trueRepository: lifinance/contracts
Length of output: 18151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- FraxFacet relevant slice ---'
cat -n src/Facets/FraxFacet.sol | sed -n '1,380p'
echo '--- search for clearSigningProposal generator/templates ---'
rg -n "clearSigningProposal|clear signing|signing proposal|proposal" .cursor src script config docs -g '!**/dist/**' -g '!**/build/**' || true
echo '--- list likely generator files with correct globs ---'
fd -a --glob '*clear*sign*' . || true
fd -a --glob '*proposal*' . || true
fd -a --glob '*signing*' . || trueRepository: lifinance/contracts
Length of output: 50375
Display _fraxData.dstEid and _fraxData.refundRecipient in both Frax clear-signing entries.
_bridgeData.destinationChainId is only descriptive here; the facet actually bridges with _fraxData.dstEid, and _fraxData.refundRecipient receives dust and any refunded native/fee leftovers. Surface those fields in both Frax entries so the signing UI reflects the real destination and recipient.
🤖 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 `@config/clearSigningProposal.json` around lines 806 - 872, Update both Frax
clear-signing entries to display _fraxData.dstEid as the destination and
_fraxData.refundRecipient as the refund recipient, replacing the descriptive
_bridgeData.destinationChainId presentation where appropriate. Add visible field
definitions for both _fraxData paths in each entry, using suitable destination
and address formatting while preserving the existing bridge fields.
| "" | ||
| ); | ||
| } else { | ||
| _sendViaTempo(_fraxData, recipient, flooredAmount); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
ast-grep outline src/Facets/FraxFacet.sol --view expanded
rg -n "_sendViaTempo|feeTokenBalanceBefore|sendOFT|feeToken" src/Facets/FraxFacet.sol src -g '*.sol'
sed -n '200,340p' src/Facets/FraxFacet.solRepository: lifinance/contracts
Length of output: 6887
🏁 Script executed:
ast-grep outline src/Interfaces/IFraxHopV2.sol --view expanded
sed -n '1,220p' src/Interfaces/IFraxHopV2.sol
rg -n "TIP_FEE_MANAGER|userTokens|quoteStatic|sendOFT|fee token|Tempo" docs src -g '*.sol' -g '*.md'Repository: lifinance/contracts
Length of output: 9436
🏁 Script executed:
sed -n '1,220p' src/Interfaces/IFraxHopV2.sol
rg -n "TIP_FEE_MANAGER|userTokens|quoteStatic|sendOFT|fee token|Tempo" docs src -g '*.sol' -g '*.md'Repository: lifinance/contracts
Length of output: 9392
Handle the bridged token in the Tempo fee refund path.
If feeToken == _bridgeData.sendingAssetId, the pre-snapshot already includes _amount, so balanceOf(address(this)) - feeTokenBalanceBefore underflows after sendOFT pulls the bridged tokens. Exclude the bridged amount from the refund baseline or track the fee-only deposit separately.
🤖 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/FraxFacet.sol` at line 229, Update the Tempo fee refund logic
surrounding _sendViaTempo in FraxFacet so the refund baseline accounts for the
bridged token when feeToken equals _bridgeData.sendingAssetId. Exclude _amount
from the pre-snapshot or otherwise track only the fee-funded deposit, preventing
balanceOf(address(this)) - feeTokenBalanceBefore from underflowing after sendOFT
transfers the bridged tokens.
Collaborator
Test Coverage ReportLine Coverage: 90.26% (3485 / 3861 lines) |
…ing mismatch; docs clarify enforced BridgeData fields (EXSC-382) - demoFrax: source-chain Hop is always the arbitrum spoke for both routes; validate oft/dstEid/recipient/amount/approvalAddress/to and throw on mismatch instead of warn - docs: BridgeData is not strictly analytics — sendingAssetId/receiver/minAmount are enforced Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
Bot-review triageApplied (pushed):
Rejected (with reason):
Note: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which Linear task belongs to this PR?
EXSC-382 (sub-task EXSC-383) — Frax bridge SC implementation.
Why did I implement it this way?
FraxFacetbridges through Frax HopV2, a LayerZero V2 OFT hub-and-spoke bridge (Fraxtal chainId 252 is the hub; every other chain is a spoke sharing one CREATE2 address). It callsHopV2.sendOFT(oft, dstEid, recipient, amountLd, 0, "")for the six Frax OFTs (frxUSD, sfrxUSD, WFRAX, frxETH, sfrxETH, FPI).One facet source for every chain, incl. Tempo. Tempo's LayerZero
EndpointV2Altrejects nativemsg.valueand charges the messaging fee in a TIP20 ERC20 gas token. Rather than a second contract, the fee mode is selected by a constructor immutable (TIP_FEE_MANAGER, non-zero only on Tempo, withPATH_USDas the default gas token) read fromconfig/frax.jsonat deploy time. This keeps one@custom:version, one audit surface, and one Deploy script; the standard-chain runtime cost is an immutable read + one branch (≈ free), the only price being a few-k gas of one-time deploy bloat for the unused branch on 26 chains.Money flow (the crux of the review):
refundRecipientviarefundExcessNative. This is a hot path, not an edge case.decimalConversionRatemultiple and only pulls the floored amount. The facet floors up front, bridges exactly that, returns the dust remainder torefundRecipientin-tx, and emitsLiFiTransferStarted.minAmount= the bridged (floored) amount. The diamond retains zero token and zero native after every bridge (asserted in tests).minAmountLD == amountLD; destination funds go straight to the recipient. No LI.FI contract ever holds funds on the destination.msg.value == 0; the fee token is resolved (TIP_FEE_MANAGER.userTokens(diamond)→PATH_USD), pulled from the caller, approved to the hop, and any unpulled remainder is swept back torefundRecipient(invariant holds even across a HopV2 proxy upgrade).Plain transfers only.
hasDestinationCall == truereverts (doesNotContainDestinationCalls); no receiver periphery, andCalldataVerificationFacetis unchanged (no compose message to validate on-chain — same asPaxosTransitFacet).Trust assumptions (documented in
docs/FraxFacet.md):dstEidis trusted from backend-generated calldata;bridgeData.destinationChainId(analytics-only) is not cross-checked against it — same model asAcrossFacetV4/PaxosTransitFacet. An unsupporteddstEidroutes funds through Fraxtal where they can be stranded pending Frax-admin recovery.recover(); the facet grants it the standardmaxApproveERC20allowance.transferFrompull; the facet pulls it from the caller viadepositAsset, so the caller must make the fee token available to the diamond. This is my best reversible inference from the verifiedRemoteHopV2Temposource and needs BE confirmation — it does not change the facet ABI, so it can be adjusted without a redesign.Testing:
test/solidity/Facets/FraxFacet.t.sol— 36 tests, 100% line/statement/function coverage onFraxFacet.sol. Standard path uses an Arbitrum mainnet fork against the real HopV2 + frxUSD self-OFT with live fee quoting; the Tempo path usesMockFraxHopV2Tempo(rule 400: Tempo's TIP20/endpoint are precompile-backed and cannot be forked). Facet compiles clean under the deploy toolchain (solc 0.8.17 + london).Follow-ups (out of scope here): SC peer review → BE integration (EXP-514) → external audit → staging then production rollout (multisig + diamondCut + diamond-log update). Aurora is excluded (absent from
config/networks.json; needs/add-network) — tracked separately.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!!!)