feat(PaxosTransitFacet): Paxos Transit bridge facet (EXSC-547) [PaxosTransitFacet v1.0.0, IPaxosTransit v1.0.0]#1996
feat(PaxosTransitFacet): Paxos Transit bridge facet (EXSC-547) [PaxosTransitFacet v1.0.0, IPaxosTransit v1.0.0]#19960xDEnYO wants to merge 29 commits into
Conversation
…EXSC-547) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…del (EXSC-547) Addresses adversarial review: - document destEID/wantAsset are trusted from the signed quote (AcrossV4 trust model) - MockTransitStation now gates on msg.value (LayerZero fee) like the real station - add tests: native-fee underpaid revert, non-zero fees, swap-output-below-offerAmount revert - assert no funds stranded in / returned to the Diamond - demo exercises the fee gate; honest success message Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…Diamond (EXSC-547) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…un.lock churn (EXSC-547) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
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 ChangesPaxos Transit Facet
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…gainst live deployment (EXSC-547) - config/paxosTransit.json: real station address (0x49AAA987...) on mainnet + outlaw - pin IPaxosTransit.submitOrder selector to the deployed+verified contract (0x3784896a) - docs: deployed addresses + verified funds-flow (msg.sender pull, public submitOrder) + Paxos-side route/signature deps Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…(EXSC-547) Deployed PaxosTransitFacet v1.0.0 to the mainnet STAGING diamond (0xbEbCDb5093B47Cd7add8211E4c77B6826aF7bc5F) to validate the deploy pipeline. Facet at 0x65cdFdA46FBfA8C584C441F396cd71972aa2520c, verified on Etherscan; selectors startBridgeTokensViaPaxosTransit (0x8a911d00) and swapAndStartBridgeTokensViaPaxosTransit (0xb33b5788) registered via diamondCut. 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: 1
🧹 Nitpick comments (1)
script/demoScripts/demoPaxosTransit.ts (1)
119-119: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd an explicit return type to
main.
waitForAnvilanddeploydeclare explicit return types;mainshould too for consistency.As per coding guidelines: "Use explicit return types for functions in TypeScript".Proposed change
-async function main() { +async function main(): Promise<void> {🤖 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/demoPaxosTransit.ts` at line 119, The main function in demoPaxosTransit should have an explicit TypeScript return type for consistency with waitForAnvil and deploy. Update the async main declaration to include its return type, and verify any awaited calls inside main still align with that signature.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 `@docs/PaxosTransitFacet.md`:
- Around line 103-104: Fix the wording in the PaxosTransitFacet documentation
sentence so it reads naturally: hyphenate “swap-specific” and replace the
ungrammatical “to can be run” phrasing with a correct construction. Update the
prose in the affected doc section only, keeping the meaning the same and
preserving the references to the swap library, calldata array, and DEX examples.
---
Nitpick comments:
In `@script/demoScripts/demoPaxosTransit.ts`:
- Line 119: The main function in demoPaxosTransit should have an explicit
TypeScript return type for consistency with waitForAnvil and deploy. Update the
async main declaration to include its return type, and verify any awaited calls
inside main still align with that signature.
🪄 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: 8ba0e418-9aac-478a-9f24-61bb26c5c31f
📒 Files selected for processing (12)
config/paxosTransit.jsondeployments/mainnet.diamond.staging.jsondeployments/mainnet.staging.jsondocs/PaxosTransitFacet.mdscript/demoScripts/demoPaxosTransit.tsscript/deploy/facets/DeployPaxosTransitFacet.s.solscript/deploy/facets/UpdatePaxosTransitFacet.s.solscript/deploy/resources/deployRequirements.jsonsrc/Facets/PaxosTransitFacet.solsrc/Interfaces/IPaxosTransit.soltest/solidity/Facets/PaxosTransitFacet.t.soltest/solidity/utils/MockTransitStation.sol
…547) Adds the startBridgeTokensViaPaxosTransit / swapAndStartBridgeTokensViaPaxosTransit entries so the verify-clear-signing CI check passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…merging main (EXSC-547) The verify-clear-signing check builds the PR merge commit, so it sees both main's LiFiIntentEscrowFacetV2 entries (#1964) and this branch's PaxosTransit entries (105 total). Merge main and regenerate so the committed file matches. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
melianessa
added
the
requires-types
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/clearSigningProposal.json (1)
1910-1912: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
nativeFeeamount isn't surfaced in the clear-signing UI.Both new formats hide all
_paxosDatafields, so the LayerZeronativeFeethe user actually pays viamsg.valueis never shown in the confirmation screen — only_bridgeData.minAmount(ERC20) is displayed. This mirrors the existing convention for other native-fee-bearing facets (Stargate, Glacis, Hop) in this same file, so it's not a regression introduced by this PR, but worth considering for a future UX pass since it's real ETH the signer spends.
[optional_refactor]Also applies to: 5172-5174
🤖 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 1910 - 1912, The PaxosTransit clear-signing entries currently hide all _paxosData fields, so the LayerZero nativeFee paid via msg.value is not shown in the confirmation UI. Update the clear-signing metadata for startBridgeTokensViaPaxosTransit to surface the nativeFee alongside the existing _bridgeData.minAmount display, following the same UI pattern used by other native-fee facets like Stargate, Glacis, and Hop. Make the change in the relevant intent/interpolatedIntent definitions so users can see the actual ETH cost before signing.
🤖 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.
Nitpick comments:
In `@config/clearSigningProposal.json`:
- Around line 1910-1912: The PaxosTransit clear-signing entries currently hide
all _paxosData fields, so the LayerZero nativeFee paid via msg.value is not
shown in the confirmation UI. Update the clear-signing metadata for
startBridgeTokensViaPaxosTransit to surface the nativeFee alongside the existing
_bridgeData.minAmount display, following the same UI pattern used by other
native-fee facets like Stargate, Glacis, and Hop. Make the change in the
relevant intent/interpolatedIntent definitions so users can see the actual ETH
cost before signing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0705bc90-2dd5-487d-b0ba-315568ea635a
📒 Files selected for processing (2)
config/clearSigningProposal.jsonscript/deploy/resources/deployRequirements.json
🚧 Files skipped from review as they are similar to previous changes (1)
- script/deploy/resources/deployRequirements.json
- docs: hyphenate 'swap-specific' and fix 'to can be run' grammar - demo: add explicit Promise<void> return type to main() Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nt (EXSC-547) - add refundRecipient to PaxosTransitData; positive slippage and swap leftovers now go there instead of msg.sender (which may be a relayer or the Permit2Proxy); zero-address check reverts InvalidCallData - move the minAmount == offerAmount check from _startBridge into the non-swap entrypoint (the swap path assigns it, making the old check a tautology there) - require nativeFee <= msg.value on both entrypoints so the LayerZero fee can never be paid from diamond balance - pin the emitted LiFiTransferStarted minAmount to the quote's offerAmount in the positive-slippage test; add revert tests for the new guards (facet line/branch coverage: 100%) - update docs, demo script, and clearSigningProposal tuple signatures for the new struct field Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/Facets/PaxosTransitFacet.sol`:
- Around line 69-72: The swap flow in PaxosTransitFacet is using
PaxosData.quote.offerAmount as the effective output floor without rejecting
zero, so update the validation in the offerAmount/minAmount check and the
deposit-and-swap path to explicitly reject a zero offerAmount before it reaches
_depositAndSwap. Make sure the logic in the swap branch preserves the
bridge-side _bridgeData.minAmount as the enforced floor for source swaps, and
only allows the Paxos-signed offerAmount to match it when it is nonzero; apply
the same fix in the later swap handling referenced by the PaxosTransitFacet
flow.
🪄 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: eea615b5-7449-4da1-a25d-016370af21f0
📒 Files selected for processing (6)
config/clearSigningProposal.jsondocs/PaxosTransitFacet.mdscript/demoScripts/demoPaxosTransit.tsscript/deploy/resources/deployRequirements.jsonsrc/Facets/PaxosTransitFacet.soltest/solidity/Facets/PaxosTransitFacet.t.sol
✅ Files skipped from review due to trivial changes (1)
- docs/PaxosTransitFacet.md
🚧 Files skipped from review as they are similar to previous changes (4)
- script/deploy/resources/deployRequirements.json
- config/clearSigningProposal.json
- script/demoScripts/demoPaxosTransit.ts
- test/solidity/Facets/PaxosTransitFacet.t.sol
… pre-swap The nativeFee <= msg.value guard broke the supported flow where the LayerZero fee is paid from an ERC20->native pre-swap output (kept in the diamond via _depositAndSwap's nativeReserve) with msg.value = 0. Keep the guard on the non-swap path only, where msg.value is the sole native source; add a test proving the pre-swap-funded flow end to end. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…for facets Lessons from the PaxosTransitFacet review (EXSC-547): user token value (positive slippage + swap leftovers) goes to an explicit refundRecipient, never msg.sender (relayer/Permit2Proxy); nativeFee <= msg.value may only be enforced on non-swap entrypoints because swap paths can fund the fee via an ERC20->native pre-swap kept by _depositAndSwap's nativeReserve. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…/quote, note >$5 min order (EXSC-547) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…h new ABI New facet 0x4DE53512F866ed4e5aa46806d63eB04C56FD04C3 registered via direct diamondCut; stale selectors of the pre-refundRecipient build (0x65cdFdA46FBfA8C584C441F396cd71972aa2520c) removed from the diamond and the diamond log. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… path (EXSC-547) The swap entrypoint used quote.offerAmount as the swap floor without any non-zero check - validateBridgeData only covered the incoming minAmount, which was then overwritten. Enforcing minAmount == offerAmount on both entrypoints extends the non-zero guarantee to the swap floor and makes the overwrite redundant (removed). Flagged by CodeRabbit on PR #1996. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ransitFacet entries (EXSC-547) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…h swap-path minAmount validation (EXSC-547) New facet 0xFC28E13e221771E93a2Adb8E4e38a32fE44BdB93 (salt 02072026) registered via direct diamondCut, replacing 0x4DE53512F866ed4e5aa46806d63eB04C56FD04C3 whose selectors were superseded on-chain; stale entry removed from the diamond log. Same version 1.0.0 (pre-release churn, facet not yet in production). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| external | ||
| payable | ||
| nonReentrant | ||
| refundExcessNative(payable(msg.sender)) |
There was a problem hiding this comment.
Issue: LZ fee refund is misrouted for the Permit2Proxy path — on essentially every tx
TransitStation charges the actual LZ fee at inclusion and reverts if underpaid, while the on-chain fee drifts up with gas between quote-time and inclusion. So the backend must always send a native buffer, or txs revert. That means a nonzero refund is the normal, every-transaction outcome — not something that only happens on a backend mistake or a rare fee spike. The refund path is exercised on virtually every bridge.
Why that matters. Both facet entrypoints refund excess native via refundExcessNative(payable(msg.sender)):
- Direct EOA / relayer call → fine, goes back to the payer.
- Via Permit2Proxy → msg.sender is the proxy. The refund lands in Permit2Proxy (caught by its receive()) but is never forwarded — no sweep after the diamond call. It strands as owner-recoverable dust, not returned to whoever funded the buffer.
Combined with the point above: for the Permit2Proxy path this stranding happens every time, not occasionally. It's a steady leak, not a corner case.
Fix. Refund native to the backend-supplied refundRecipient instead of msg.sender, on both paths — as SupersetFacet (the closest analog: LZ-messaged, ERC20 offer, native = LZ fee) already does with a single refundAddress for excess native + swap leftovers + positive slippage. Strictly more expressive than msg.sender (backend picks the correct payee), and closes the leak without touching shared periphery.
Required with the change:
- Add a refundRecipient != address(0) guard to the non-swap path (only the swap path validates it today) — else excess native burns to address(0).
- Accept the minor footgun: a refundRecipient that rejects native reverts the bridge (self-inflicted; Superset accepts the same).
- Remove the now-inverted "Excess native stays with msg.sender — the caller funds the fee" comment in the swap path and the matching line in the PR description.
There was a problem hiding this comment.
Addressed in 005857d:
- Both entrypoints now use
refundExcessNative(payable(_paxosData.refundRecipient))— nothing refunds tomsg.senderanymore. - Added the
refundRecipient != address(0)guard to the non-swap path (InvalidCallData, consistent with the swap path). One correction on the rationale there: a zero recipient wouldn't burn —LibAsset.transferNativeAssetreverts onaddress(0). The actual failure mode was a data-dependent late revert (tx success depending on whether LZ fee drift left an excess to refund); the guard makes it deterministic and early. - Fixed the inverted swap-path comment; struct NatSpec +
docs/PaxosTransitFacet.mdnow document the routing and the accepted native-rejecting-recipient footgun (mirrorsSupersetFacet.refundAddress). - Codified in
[CONV:FACET-REFUNDS](.agents/rules/102-facets.md); tests added: excess native →refundRecipienton both paths, zerorefundRecipientrevertsInvalidCallDataon the non-swap path.
One factual nit on the mechanism: the TransitStation doesn't charge-and-refund itself — it forwards the whole msg.value to the LZ endpoint with refundAddress = msg.sender, so it's the endpoint that reverts on underpay / refunds the overage to the diamond mid-call. Same conclusion (a refund is the every-tx norm given the backend's buffer), different plumbing.
…h entrypoints (EXSC-547) Excess source-side native (LZ fee overage the endpoint refunds to the diamond mid-call) previously went back to msg.sender, stranding it in Permit2Proxy as owner-recoverable dust on essentially every tx routed through it. Route it to the explicit refundRecipient instead, mirroring SupersetFacet's refundAddress. Also adds the refundRecipient zero-check to the non-swap path: without it a zero recipient reverts only late inside refundExcessNative (LibAsset.transferNativeAsset reverts on address(0), it does not burn) and only when LZ fee drift actually left an excess - a data-dependent late revert. The guard makes it a deterministic early InvalidCallData, consistent with the swap path. Updates [CONV:FACET-REFUNDS], facet NatSpec, docs and tests accordingly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…C-547) The network was renamed outlaw -> robinhood (#2006, merged to main); the deploy script looks up config/paxosTransit.json's transitStation address by the live network key, so this branch's stale "outlaw" key would miss on lookup post-rebase. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🤖 GitHub Action: Security Alerts Review 🔍🟢 Dismissed Security Alerts with Comments 🟢 View Alert - File: 🟢 View Alert - File: ✅ No unresolved security alerts! 🎉 |
lifi-action-bot
changed the title
…h refund-routing fix (EXSC-547) New facet 0x3A5ef52B3aEEaA29e98C19806C187A8a774F5591 registered via direct diamondCut, replacing 0x4DE53512F866ed4e5aa46806d63eB04C56FD04C3. That address was itself stale: a prior redeploy attempt in this worktree read bytecode from an un-rebuilt `out/` dir, silently re-deriving the CREATE3 salt of an even older, pre-minAmount-validation build and landing back on an already-deployed, superseded address instead of the current source. Caught by manually re-deriving the salt against a clean `forge build` and comparing predicted addresses. Both now-stale PaxosTransitFacet entries removed from the diamond log. Same version 1.0.0 (pre-release churn, facet not yet in production).
Test Coverage ReportLine Coverage: 90.09% (3420 / 3796 lines) |
…tic import (EXSC-547) MockTransitStation.sol and TestToken.sol live under test/solidity/utils/, which the validate-scripts CI job's typechain step never compiles (it only runs `forge build src`). The demo's static `import ... from '../../out/...json'` therefore fails TS2307 the moment that job actually runs a type-check - previously masked because the job unconditionally skips while the PR is in draft. Load the three artifacts via fs.readFileSync + JSON.parse at runtime instead, sidestepping TS module resolution entirely; a full local `forge build` still produces the files for `bunx tsx` to read when the demo is actually run. Verified: tsc-files --noEmit, script/utils/validateScripts.ts, and a live run of the demo against a local anvil fork all pass.
… tests against the real TransitStation (EXSC-547) Per [CONV:FORK-FIRST]: pin the fork to a block where Paxos has the USDC->USDG route to Robinhood (EID 30416) fully configured, rotate the station's quoteSigner to a test key via its real owner-gated setter, and sign Quote structs in-test against the station's live EIP-712 domain (new TestPaxosTransitBackendSig helper). Fund the LayerZero fee from the station's own quoteSend. New real-station coverage a mock could not provide: protocolFee/ integratorFee/net splits to the real Paxos recipients, replay guard (SignatureAlreadyUsed + usedDigests), QuoteExpired, InvalidSigner, LZ_InsufficientFee on underpaid fee, and endpoint fee-overage refunds flowing back to the refundRecipient via refundExcessNative. The demo script now runs the same flow against the real station on its anvil mainnet fork instead of deploying a mock. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… test mocks are acceptable Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…gate-lifting on the fork Fork tests against mocks prove nothing a plain local node would not. Expand the gate-lifting toolbox (owner prank on real setters, stdstore/ vm.store overrides, deal(), block re-pinning, protocol-quoted live params), ban disguised mocking (vm.etch / vm.mockCall on the counterparty), require setUp() guards on pinned config, and require mock NatSpec + PR to document which techniques were ruled out. Validated in practice by the PaxosTransitFacet fork-test rewrite (EXSC-547). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🔍 QA Review — EXSC-547
🧠 What this ticket doesPaxosTransitFacet — a bridge facet for Paxos Transit (ETH ↔ Robinhood Chain via a Paxos-signed, LayerZero-settled quote). Diamond pulls the offer asset, approves the
Issue Resolution Status
Resolved by main mergeIssues #2 + #7 — Config key mismatch: ✅ Resolved The merge commit ( Outstanding items#1 — [Medium] New Facet Contract Checklist checkbox still unchecked The PR body still shows: The merge did not change the PR body. This is a mandatory pre-merge gate for new facets. The author must work through the checklist and tick it — or provide an explicit acceptance comment documenting which items were reviewed and any that were intentionally deferred. #3 — [Medium] No zksync counterpart deploy script The PR file list contains zero zksync-pattern files. Per repo convention, new deployable contracts require a #4 — [Medium]
Current struct: struct PaxosTransitData {
IPaxosTransit.Quote quote; // 1st
bytes signature; // 2nd
uint256 nativeFee; // 3rd
address refundRecipient; // 4th ← convention violation
}The opaque-calldata exception in the rule governs the on-chain receiver validation requirement (EIP-712 signed quote binds receiver). It does not grant an exception to the field-ordering convention — these are two separate requirements. Reading the rule as written, Please either: (a) reorder so a #5 — [Medium] CodeRabbit The CodeRabbit comment at Please reply to the thread pointing to the commit that resolved this, and mark the conversation as resolved. 🛡️ Version & Audit State
🧪 Test Coverage
CI coverage: 90.09% line, 93.78% function, 72.97% branch — above the 89% repo threshold. 🔗 Downstream ImpactBlocks: EXP-560 (parent Paxos Transit integration) — unblocks backend integration once merged and deployed. Operational: QA Agent — 2026-07-03 |
![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 7 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 | 🟡 Medium | Process | Author's "New Facet Contract Checklist" left unchecked in PR author checklist |
| 2 | 🟡 Medium | Config | config/paxosTransit.json key "robinhood" ≠ networks.json slug "outlaw" — deploy to chain 4663 fails |
| 3 | 🟡 Medium | Convention | script/deploy/zksync/DeployPaxosTransitFacet.zksync.s.sol missing |
| 4 | 🟡 Medium | Convention | PaxosTransitData.refundRecipient at position 4 — SC2 requires receiverAddress first |
| 5 | 🟡 Medium | Convention | Unresolved CodeRabbit thread (zero offerAmount) — no author reply |
| 6 | 🟡 Medium | Process | Ticket AC absent — EXSC-547 has no verifiable acceptance criteria |
| 7 | 🟢 Low | Convention | config/paxosTransit.json "robinhood" key unrecognised — will confuse future contributors |
1. [Medium] Author's "New Facet Contract Checklist" left unchecked
In the author's pre-review checklist, "For new facets: I have checked all points from this list" is explicitly unchecked (- [ ]). The checklist caption reads "DO NOT DEPLOY and contracts BEFORE CHECKING THIS". For a new bridge facet handling real user funds this is a mandatory pre-merge gate. Either complete the Notion checklist and tick the box, or provide a comment documenting which items were skipped and why.
2. [Medium] config/paxosTransit.json "robinhood" key does not match networks.json slug "outlaw" — hard deployment blocker
networks.json registers chain 4663 as "outlaw". DeployPaxosTransitFacet.s.sol resolves the constructor address with _getConfigContractAddress(path, ".transitStation." + network). Running forge script ... --network outlaw produces a null jq result → address(0) → constructor reverts InvalidConfig(). Rename the key to "outlaw" to match the authoritative slug, or open a separate PR renaming the chain in networks.json first.
3. [Medium] No script/deploy/zksync/DeployPaxosTransitFacet.zksync.s.sol
Every other deployed facet in this repo has a counterpart in script/deploy/zksync/. S7 convention requires the counterpart for every new deployable contract. Either add DeployPaxosTransitFacet.zksync.s.sol, or explicitly document in deployRequirements.json or the PR body that Paxos Transit is exempt from zkSync deployment.
4. [Medium] PaxosTransitData.refundRecipient at struct position 4 — SC2 requires receiverAddress first
.agents/rules/102-facets.md [CONV:FACET-REQS] mandates receiverAddress as the first field in {FacetName}Data. The refundRecipient field sits at position 4. The opaque-calldata exception satisfies the receiver-safety requirement but is separate from the field-ordering mandate. Either move refundRecipient to position 1, or confirm with the team that the exception waives ordering, document it in the facet NatSpec, and update the rules file. An explicit acceptance comment is required either way.
5. [Medium] Unresolved CodeRabbit inline thread — no author reply to zero offerAmount comment
CodeRabbit posted an inline comment on 2026-07-02 requesting a zero offerAmount guard before _depositAndSwap. There is no author reply and no fix commit. Even though the finding is a false positive (the validateBridgeData modifier covers this via minAmount == 0 ↔ offerAmount == 0 tie), the thread must be explicitly closed with a reply explaining the existing protection. Unresolved /pr-ready threads are a signal the review was incomplete.
6. [Medium] Ticket AC absent — EXSC-547 has no verifiable acceptance criteria
The ticket was written as a feasibility assessment and carries no AC. The PR delivers a full production facet. Update the Linear ticket with 3-5 verifiable AC items so the QA sign-off has a concrete contract to verify against (e.g. successful submitOrder on-chain, funds routing, deploy readiness, audit coverage).
7. [Low] config/paxosTransit.json "robinhood" key is not a recognised network slug
Once the key is corrected to "outlaw" (item #2), the config entry will be valid. A _comment field explaining the purpose of the Robinhood Chain entry would prevent future confusion for contributors unfamiliar with the chain's current slug.
💡 Once you've addressed the items above, re-apply the "Agent Review Request" label to trigger an automated re-review.
There was a problem hiding this comment.
Re-review after merge from main (a854c24, 2026-07-03T08:23:32Z). Issues #2 and #7 (config key mismatch) are now resolved by the merge — networks.json registers chain 4663 as "robinhood" and config/paxosTransit.json agrees. Four items remain open.
| # | Severity | Type | Issue / File |
|---|---|---|---|
| 1 | 🟡 Medium | Process | "New Facet Contract Checklist" checkbox still unchecked in PR description |
| 2 | 🟡 Medium | Convention | No zksync counterpart deploy script for PaxosTransitFacet |
| 3 | 🟡 Medium | Convention | PaxosTransitData.refundRecipient is last field; .agents/rules/102-facets.md requires receiverAddress first |
| 4 | 🟡 Medium | Process | CodeRabbit offerAmount inline thread has no author reply — unresolved thread |
1. [Medium] New Facet Contract Checklist checkbox still unchecked
The PR body still has - [ ] For new facets: I have checked all points from this list: ... unchecked. This is a mandatory pre-merge gate for new facets. Please work through the checklist and tick it, or provide an explicit acceptance comment documenting any items intentionally deferred.
2. [Medium] No zksync counterpart deploy script
Zero zksync-pattern files appear in the PR file list. If Paxos Transit is not intended for zkSync chains, document this explicitly (e.g. a note in docs/PaxosTransitFacet.md or in script/deploy/resources/deployRequirements.json) so the absence reads as intentional rather than an oversight.
3. [Medium] PaxosTransitData struct — receiverAddress must be the first field
.agents/rules/102-facets.md ([CONV:FACET-REQS]) states: "receiverAddress first in {facetName}Data." The opaque-calldata exception covers the on-chain receiver-validation requirement — it does not waive the field-ordering convention. Current struct has refundRecipient last. Please either reorder so a receiverAddress-named field is first, or get SC team sign-off on a formal rule exception for opaque-calldata facets and document it in the rule file.
4. [Medium] CodeRabbit offerAmount inline thread — no author reply
The CodeRabbit comment at src/Facets/PaxosTransitFacet.sol (2026-07-02T06:33:56Z) has no author reply. The code fix (minAmount == offerAmount enforcement) was made in commit 90ab11efccf1. Please reply to the thread pointing to that commit and mark the conversation resolved.
💡 Once you've addressed the items above, re-apply the "Agent Review Request" label to trigger an automated re-review.
5 participants
Which Linear task belongs to this PR?
EXSC-547
Why did I implement it this way?
Implements
PaxosTransitFacet— a bridge facet for Paxos Transit (ETH ↔ Robinhood Chain via a Paxos-signed quote), plus 100%-covered unit tests, deploy scaffolding, docs, and a runnable local demo.Design (per Paxos' answers to our integration questionnaire):
submitOrder'smsg.senderalways provides the offer asset, and the want asset goes toquote.receiverregardless of submitter. So the facet keeps our normal funds flow: deposit/swap into the Diamond →maxApproveERC20the station →submitOrder, which pulls the offer asset from the Diamond.quote.receiver(validated== bridgeData.receiver) directs the output to the end user. NosubmitOrderWithPermit/on-behalf call is needed — theQuotestruct has no payer field, onlyreceiver. Modeled onGlacisFacet.noNativeAsset);msg.valuecarries the LayerZero messaging fee (nativeFee), forwarded assubmitOrder{value: nativeFee}.quote.offerAmount, refunds positive slippage, and bridges exactlyofferAmount.refundRecipient(review feedback from @gvladika): swap leftovers, positive slippage, and excess source-side native — including LZ fee overage, since the TransitStation forwards the wholemsg.valueto the LZ endpoint withrefundAddress = msg.sender, so the endpoint refunds overage to the Diamond mid-call on essentially every tx (the backend must buffer against fee drift).msg.sendermay be a relayer or the Permit2Proxy (where a native refund would strand as owner-recoverable dust), so nothing refunds tomsg.senderanymore. Both entrypoints zero-checkrefundRecipient(InvalidCallData) — without the guard a zero recipient would revert only when fee drift actually left an excess (LibAsset.transferNativeAssetreverts onaddress(0), it does not burn), i.e. data-dependently and late. MirrorsSupersetFacet'srefundAddress; codified as[CONV:FACET-REFUNDS]in.agents/rules/102-facets.md. Known accepted footgun: arefundRecipientthat rejects native reverts the bridge (self-inflicted; same as Superset)._startBridge:sendingAssetId/minAmount/receivermust match the signed quote, anddistributorCode == "LIFI"(0x4c49464900…) for attribution; elseInformationMismatch.destEID/wantAssetare trusted from the signed quote (not cross-checked againstdestinationChainId) — funds always follow the signed quote, same trust model asAcrossFacetV4'soutputAmount. Documented in code + docs.Verified against the live deployment. Paxos deployed + verified the
TransitStationon 2026-06-29 at0x49AAA987b1a7e9E4AE091dcD8332c39F322D7d28on Ethereum (1) and Robinhood Chain (4663). I checked the verified source + on-chain state and confirmed:submitOrderpulls the offer asset frommsg.sender(so the Diamond is the payer — no on-behalf/permit needed); its capability is public (Authority.canCall→ true, the Diamond can call it); the on-chain ABI matchesIPaxosTransitexactly (exercised directly — the test suite calls the deployed station on a mainnet fork); and no failure path refunds the caller. Real addresses are now inconfig/paxosTransit.json.Verification: 39 tests, 100% facet coverage (lines/statements/branches/functions), now run as mainnet-fork tests against the real
TransitStationper[CONV:FORK-FIRST]— no mock. (The rule itself ships in this PR:.agents/rules/400-solidity-tests.mdgains the fork-first section — mocks are a last resort behind gate-lifting on the fork.) The station's Paxos-signer gate is lifted on the fork (owner-prankedsetQuoteSignerto a test key; quotes EIP-712-signed in-test via the newTestPaxosTransitBackendSighelper against the station's live domain), the LZ fee comes from the station's ownquoteSend, and the fork is pinned to a block where Paxos has the USDC→USDG route to EID 30416 configured (guarded insetUp). This adds real-behavior coverage a mock couldn't provide: protocolFee/integratorFee/net splits to the real Paxos recipients, replay guard (SignatureAlreadyUsed+usedDigests),QuoteExpired,InvalidSigner,LZ_InsufficientFeeon underpaid fee, and LZ fee-overage refunds flowing back torefundRecipient.MockTransitStationis deleted; the demo (demoPaxosTransit.ts) now runs the same real-station flow end-to-end on an anvil mainnet fork (verified locally).Operational dependencies (Paxos / backend, not this facet): the route
(destEID, offerAsset, wantAsset)must be approved by Paxos (setRouteApprovals, routes go live ~2026-07-01); quotes must be backend-signed. Out of scope: on-chain deployment; backend integration.Reviewed locally with an adversarial code-review pass (funds-safety / correctness) and a self-review pass. Local
/pr-ready(CodeRabbit CLI) ran on the latest push: two findings were against an untracked local dev script that is not part of this PR; the one in-scope suggestion is deferred to reviewers below.Deferred local-CodeRabbit suggestion (explicit reviewer decision wanted): harden
swapAndStartBridgeTokensViaPaxosTransitwith a native-balance-delta check (require the post-swap native balance gain to covernativeFeebefore_startBridge), so the LZ fee could never be paid from pre-existing stray diamond native balance. Not applied pre-push: it is a logic change to the already-reviewed fee-funding design (nativeFeemay be funded bymsg.valueor by the ERC20→native pre-swap's reserved output; the Diamond holds no native between txs by invariant, andrefundExcessNativeonly refunds balance gained within the call), so it should be a deliberate reviewer call rather than a silent pre-push edit.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!!!)