fix!: size maker liquidity to the on-chain margin check#55
Merged
Conversation
estimateLiquidity used the naive amount1 formula, which computes the liquidity whose notional exactly equals the margin. With the deployed maker initial margin ratio of 100% and the contract rounding pulled amounts up while flooring position value, the estimate landed 1-2 units past the health check boundary and openMaker reverted with MarginRatioTooLow. It also ignored where the range sits relative to the current price, so ranges above the current price (pure perp exposure valued at mark) were sized wrong entirely. estimateLiquidity now takes the perp address and: - reads poolState, the maker initial margin ratio, and the beacon index - values below-range positions exactly, replicating the contract rounding (validated against mainnet: max healthy liquidity is floor((margin - 2) * Q96 / sqrtPriceDiff)) - values straddling and above-range positions at the larger of the AMM price and beacon index, since the mark price is not directly readable - verifies the estimate with an eth_call simulation of openMaker (the health check runs before the margin transfer, so MarginRatioTooLow is conclusive) and bisects down to the true boundary if the mark price sits above both proxies Also fixes getSqrtRatioAtTick to round up the final shift like the contract TickMath. BREAKING CHANGE: estimateLiquidity now requires the perp address as its second argument. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR changes liquidity estimation to use on-chain margin checks, rewrites the unit tests around a mocked context, and updates release metadata for version 0.11.0. ChangesMargin-based liquidity estimation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant estimateLiquidity
participant PublicClient
participant PerpContract
Caller->>estimateLiquidity: estimateLiquidity(context, perpAddress, tickLower, tickUpper, usdScaled)
estimateLiquidity->>PublicClient: readContract poolState/makerMarginRatios/index
estimateLiquidity->>estimateLiquidity: maxLiquidityForMargin(candidate)
estimateLiquidity->>PublicClient: call openMaker(candidate)
PublicClient->>PerpContract: simulate openMaker
PerpContract-->>estimateLiquidity: success or MarginRatioTooLow revert
alt candidate fails
estimateLiquidity->>estimateLiquidity: bisect between 0 and candidate
estimateLiquidity->>PublicClient: call openMaker(mid) repeatedly
end
estimateLiquidity-->>Caller: return max passing liquidity
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/liquidity.ts (1)
48-51: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate tick bounds before calling
getSqrtRatioAtTick.estimateLiquidityonly checkstickLower < tickUpper; non-integer or out-of-range ticks are still coerced by the bitwise math ingetSqrtRatioAtTick, which can produce wrong estimates. Reject non-integers and ticks outsideMIN_TICK/MAX_TICKup front.🤖 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/utils/liquidity.ts` around lines 48 - 51, estimateLiquidity currently only checks that tickLower is less than tickUpper, but it still passes invalid tick values into getSqrtRatioAtTick. Update estimateLiquidity in liquidity.ts to validate both ticks are integers and within MIN_TICK/MAX_TICK before any sqrt-ratio calls, and keep the existing tickLower/tickUpper ordering check after those bounds checks. Use the existing getSqrtRatioAtTick, MIN_TICK, and MAX_TICK symbols to locate the validation path.
🧹 Nitpick comments (1)
src/__tests__/unit/liquidity.test.ts (1)
21-49: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert mocked reads are routed to the expected contracts.
The harness currently keys only on
functionName, so a regression that ignoresperpAddressor reads beacon/margin data from the wrong address would still pass. Checkaddressin the mock to cover the breaking per-market API contract.Proposed test-harness tightening
+const MARGIN_RATIOS = "0x8afca53c52b1f02d76aefb811c6b08f4bd3e4cf9"; +const BEACON = "0xd3ac79e96148b420f86fb5fc57573a767d00ec16"; + - const readContract = vi.fn(async ({ functionName }: { functionName: string }) => { + const readContract = vi.fn(async ({ address, functionName }: { address: string; functionName: string }) => { if (functionName === "poolState") { + expect(address).toBe(PERP); return [0, sqrtPriceX96, ammPriceX96, 0n]; } if (functionName === "makerMarginRatios") { + expect(address).toBe(MARGIN_RATIOS); return [options.makerInitRatio ?? 1_000_000, 900_000, 800_000]; } if (functionName === "index") { + expect(address).toBe(BEACON); return options.beaconIndexX96 ?? 0n; } throw new Error(`Unexpected readContract call: ${functionName}`); }); const call = vi.fn(options.callImpl ?? (async () => ({ data: "0x" }))); const context = { publicClient: { readContract, call }, - getPerpConfig: vi.fn(async () => ({ - marginRatios: "0x8afca53c52b1f02d76aefb811c6b08f4bd3e4cf9", - beacon: "0xd3ac79e96148b420f86fb5fc57573a767d00ec16", + getPerpConfig: vi.fn(async (perpAddress: PerpAddress) => ({ + ...(expect(perpAddress).toBe(PERP), {}), + marginRatios: MARGIN_RATIOS, + beacon: BEACON, })),As per coding guidelines, “Perp-address-facing SDK parameters and data fields should use the
PerpAddressalias instead of genericHex.”🤖 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/__tests__/unit/liquidity.test.ts` around lines 21 - 49, The test harness in makeContext only matches mocked reads by functionName, so it can miss regressions where perpAddress is ignored or data is read from the wrong contract. Update the readContract mock to also validate the address argument for each expected call (poolState, makerMarginRatios, index), and fail when the contract address is not the expected one. This will tighten coverage around the publicClient.readContract usage and the PerpCityContext setup without relying on function name alone.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 `@package.json`:
- Line 5: The version bump in package.json is not reflected in the rest of the
release metadata. Update package-lock.json so its root version matches 0.11.0,
and add a 0.11.0 entry to CHANGELOG.md to keep the release state consistent. Use
the existing version and release sections in package-lock.json and CHANGELOG.md
as the places to update.
In `@src/utils/liquidity.ts`:
- Around line 195-200: The liquidity check in the try/catch around
context.publicClient.call is too broad and currently marks any
non-MARGIN_RATIO_TOO_LOW failure as healthy. Narrow the catch handling in the
health-check path so only the known post-margin failure selectors are treated as
a passing liquidity result, and rethrow unexpected RPC, malformed-call, or
unrelated contract revert errors from errorChainContains/error selector checks.
---
Outside diff comments:
In `@src/utils/liquidity.ts`:
- Around line 48-51: estimateLiquidity currently only checks that tickLower is
less than tickUpper, but it still passes invalid tick values into
getSqrtRatioAtTick. Update estimateLiquidity in liquidity.ts to validate both
ticks are integers and within MIN_TICK/MAX_TICK before any sqrt-ratio calls, and
keep the existing tickLower/tickUpper ordering check after those bounds checks.
Use the existing getSqrtRatioAtTick, MIN_TICK, and MAX_TICK symbols to locate
the validation path.
---
Nitpick comments:
In `@src/__tests__/unit/liquidity.test.ts`:
- Around line 21-49: The test harness in makeContext only matches mocked reads
by functionName, so it can miss regressions where perpAddress is ignored or data
is read from the wrong contract. Update the readContract mock to also validate
the address argument for each expected call (poolState, makerMarginRatios,
index), and fail when the contract address is not the expected one. This will
tighten coverage around the publicClient.readContract usage and the
PerpCityContext setup without relying on function name alone.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 739afb71-b2b3-4340-b666-02f5c46bb323
📒 Files selected for processing (3)
package.jsonsrc/__tests__/unit/liquidity.test.tssrc/utils/liquidity.ts
- passesMarginCheck no longer treats every non-margin-check failure as healthy: only the probe sender's post-health-check margin-transfer reverts (solady TransferFromFailed selector, ERC20 allowance reason strings) count as passing; transport failures and unrelated reverts are rethrown. Validated against mainnet, where the healthy path resolves via the real TransferFromFailed revert. - test harness readContract mock now validates the contract address for each read (perp poolState, marginRatios module, beacon index) - sync package-lock.json to 0.11.0 and add the CHANGELOG entry Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
3 tasks
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
1 participant
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.
Problem
LP opens were failing with
MarginRatioTooLow(0xb2c649db) even at sensible sizes (reported: $200 on price range 30-55 on mainnet).estimateLiquidityused the naive amount1 formula: it solves for the liquidity whose USD notional exactly equals the margin. The deployed maker initial margin ratio is 100%, and insideopenMakerthe pool pulls amounts rounded up while the health check values the position rounded down — so the estimate consistently landed 1-2 units past the boundary. Empirically (binary-searched on mainnet across five margin/range configs): the max healthy liquidity for a below-range maker is exactlyfloor((margin - 2) * Q96 / sqrtPriceDiff); the old formula returns that + 1.Two further latent bugs:
Fix
estimateLiquiditynow takes the perp address and:poolState, the maker initial margin ratio, and the beacon index.margin - 2slack), price-independent.max(ammPrice, beaconIndex)— the true mark blends AMM/index/EMAs and is not directly readable — with a small buffer.eth_callsimulation ofopenMaker: the health check runs before the margin transfer, soMarginRatioTooLowis conclusive while any later revert means healthy. If the analytic estimate overshoots (mark above both proxies), it bisects to the true boundary.Also fixes
getSqrtRatioAtTickto round up the final shift like the contract's TickMath.BREAKING:
estimateLiquidity(context, tickLower, tickUpper, usd)→estimateLiquidity(context, perpAddress, tickLower, tickUpper, usd). Version bumped to 0.11.0. The client'scalculateMakerLiquidity(apps/webOrderSummary.tsx) needs the one-line call-site update when bumping.Validation
Against the live mainnet market
0x8ac0...7b6c:All estimates land within 1% of the true on-chain boundary. The straddle case exercised the bisection path: the real mark (58.60) sat 5.2% above the AMM price (55.70).
Unit suite: 261 tests pass, including a mainnet-derived regression test pinning the exact boundary value.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores