Add ST0x order quote2 Base fork test#2628
Conversation
Provides a self-contained fork test with pinned order bytes, oracle signed context, and block number so others can run quote2 against the live ST0x orderbook on Base. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughA new Foundry fork test validates the ChangesOrderBookV6 Quote Function Fork Test
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/concrete/ob/OrderBookV6.quote.fork.t.sol (2)
30-34: 💤 Low valueReconsider the public RPC fallback and combine fork creation with the block.
rollFork(FORK_BLOCK)to historical block46_815_818requires archive-node access. The defaulthttps://mainnet.base.orgpublic endpoint is unlikely to serve archive data reliably and will produce confusing failures, while the PR instructions stateBASE_MAINNET_RPC_URLmust be set. Failing fast when the env var is missing makes the contract explicit. You can also create the fork directly at the pinned block.♻️ Proposed change
function setUp() public { - string memory rpc = vm.envOr("BASE_MAINNET_RPC_URL", string("https://mainnet.base.org")); - vm.createSelectFork(rpc); - vm.rollFork(FORK_BLOCK); + string memory rpc = vm.envString("BASE_MAINNET_RPC_URL"); + vm.createSelectFork(rpc, FORK_BLOCK); }🤖 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/concrete/ob/OrderBookV6.quote.fork.t.sol` around lines 30 - 34, Change setUp() to require BASE_MAINNET_RPC_URL from the environment (fail fast if missing instead of using the public https://mainnet.base.org fallback), and create the fork at the pinned block in one call rather than calling vm.rollFork(FORK_BLOCK); specifically, stop using vm.envOr(...) with a public default, read the env var directly and revert if empty, and call vm.createSelectFork(rpc, FORK_BLOCK) (replacing the separate vm.createSelectFork + vm.rollFork pattern) so the fork is created at FORK_BLOCK and avoids relying on archive access from a public RPC.
74-76: ⚡ Quick winRe-enable the output assertions or document why they're disabled.
The test currently asserts only
success, butquote2can returnsuccess == truewith a zeromaxOutput/ioRatio, so this leaves the test asserting little about the actual quote. The commented-out checks at Lines 75-76 are exactly what makes the result meaningful. Please enable them, or if they intentionally cannot hold for this pinned order/context, leave a comment explaining why rather than leaving dead assertions.💚 Proposed change
assertTrue(success, "quote2"); - // assertFalse(maxOutput.isZero(), "max output"); - // assertFalse(ioRatio.isZero(), "io ratio"); + assertFalse(maxOutput.isZero(), "max output"); + assertFalse(ioRatio.isZero(), "io ratio");🤖 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/concrete/ob/OrderBookV6.quote.fork.t.sol` around lines 74 - 76, The test currently only asserts success for the quote call (assertTrue(success, "quote2")) while the meaningful checks for non-zero outputs (assertFalse(maxOutput.isZero(), "max output") and assertFalse(ioRatio.isZero(), "io ratio")) are commented out; either re-enable those two assertions in the same test (restoring assertFalse(maxOutput.isZero(), "max output") and assertFalse(ioRatio.isZero(), "io ratio")) so the quote result is validated, or if zero outputs are valid for this pinned order/context, leave the assertions commented and add a short inline comment referencing the reason (e.g., “zero output expected for pinned orders because ...”) adjacent to the assert lines; locate the checks around the assertTrue(success, "quote2") in test/concrete/ob/OrderBookV6.quote.fork.t.sol to apply 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.
Nitpick comments:
In `@test/concrete/ob/OrderBookV6.quote.fork.t.sol`:
- Around line 30-34: Change setUp() to require BASE_MAINNET_RPC_URL from the
environment (fail fast if missing instead of using the public
https://mainnet.base.org fallback), and create the fork at the pinned block in
one call rather than calling vm.rollFork(FORK_BLOCK); specifically, stop using
vm.envOr(...) with a public default, read the env var directly and revert if
empty, and call vm.createSelectFork(rpc, FORK_BLOCK) (replacing the separate
vm.createSelectFork + vm.rollFork pattern) so the fork is created at FORK_BLOCK
and avoids relying on archive access from a public RPC.
- Around line 74-76: The test currently only asserts success for the quote call
(assertTrue(success, "quote2")) while the meaningful checks for non-zero outputs
(assertFalse(maxOutput.isZero(), "max output") and assertFalse(ioRatio.isZero(),
"io ratio")) are commented out; either re-enable those two assertions in the
same test (restoring assertFalse(maxOutput.isZero(), "max output") and
assertFalse(ioRatio.isZero(), "io ratio")) so the quote result is validated, or
if zero outputs are valid for this pinned order/context, leave the assertions
commented and add a short inline comment referencing the reason (e.g., “zero
output expected for pinned orders because ...”) adjacent to the assert lines;
locate the checks around the assertTrue(success, "quote2") in
test/concrete/ob/OrderBookV6.quote.fork.t.sol to apply the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f3e33bd-977d-4b7f-ae6f-3b4f0ed1bc4d
📒 Files selected for processing (1)
test/concrete/ob/OrderBookV6.quote.fork.t.sol
1 participant
Summary
test/concrete/ob/OrderBookV6.quote.fork.t.sol, a fork test that quotes a live ST0x order on Base viaquote2with pinnedorderBytes, oracle signed context, block number, and orderbook address.How to run
Pinned constants in the test (update when reproducing a different order):
0xe522cB4a5fCb2eb31a52Ff41a4653d85A4fd7C9D468158180x4a051ad4567935a5a0570b3e2e77714c44405bb58e5b83e3cc484de1cee0747eOracle signed context and signature are embedded in the test; refresh from the ST0x API/subgraph when they expire.
Test plan
BASE_MAINNET_RPC_URLset to a Base mainnet RPCforge test --match-contract OrderBookV6QuoteForkTest -vvvvpasses and logs oracle + quote valuesMade with Cursor
Summary by CodeRabbit
Tests