fix(adl): compute dominantSide and use it as buildAdlTransaction's default target - #345
Open
Morenikeoa wants to merge 1 commit into
Open
fix(adl): compute dominantSide and use it as buildAdlTransaction's default target#345Morenikeoa wants to merge 1 commit into
Morenikeoa wants to merge 1 commit into
Conversation
…fault target
The ADL module's own docs state "The most profitable positions on the
dominant side are deleveraged first" and "the position at rank 0 of
the dominant side is deleveraged first," but rankAdlPositions never
computed which side (long/short) actually has greater net open
interest. buildAdlTransaction, when called without an explicit
preferSide, fell through to ranking.ranked[0] — the single best PnL%
position merged across BOTH sides, not the dominant side's top
position.
Per the on-chain devnet log format ("net_long_oi=500000
net_short_oi=500000 target_side=long candidates=0"), the on-chain ADL
engine gates eligibility by net-OI dominant side and resolves ties to
"long". A caller relying on the SDK's documented default behavior
without manually passing preferSide could have the SDK pick a
position on the minority side, which the program would reject.
Note on real-world impact: encodeExecuteAdl/buildAdlInstruction
currently always throw removedInstruction() (ExecuteAdl was removed
from the v17 wrapper, see test/adl.test.ts), so buildAdlTransaction's
instruction-build path is not reachable end-to-end today. The read-side
ranking functions (rankAdlPositions, fetchAdlRankedPositions) remain
exported, though, and could mislead an external keeper/dashboard that
trusts ranking.ranked[0] as "the next ADL victim" without separately
checking OI dominance.
Fix: rankAdlPositions now computes engine.longOi vs engine.shortOi and
exposes the result as AdlRankingResult.dominantSide ("long" | "short" |
null — null only when engine state can't be parsed at all). Ties
resolve to "long" to match the on-chain convention.
buildAdlTransaction's default branch (no preferSide) now targets
ranking.dominantSide's top position, falling back to the global
ranked[0] only when dominantSide is null.
Added regression tests in test/adl.test.ts: dominantSide is "long"/
"short" correctly from engine.longOi/shortOi, ties resolve to "long",
and dominantSide is null when engine parsing fails entirely (bad
magic). Also added a pure-logic test for buildAdlTransaction's
target-side branch selection. Confirmed all new tests fail against
the prior code and pass against this fix.
Full suite: 857 passed | 31 skipped (882 -> 888, +6 new tests).
📝 WalkthroughWalkthroughAdds a ChangesADL dominant-side flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
🤖 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/solana/adl.ts`:
- Around line 205-210: The dominant-side detection in
parseEngine()/buildAdlTransaction is treating missing per-side OI as a real tie
and defaulting to "long", which incorrectly overrides the ranking.ranked[0]
fallback. Update the dominantSide assignment so it remains null unless the
detected slab layout actually includes both longOi and shortOi fields; only
derive "long"/"short" when those offsets are present, and keep the existing
fallback behavior unchanged for older layouts.
🪄 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
Run ID: c0351665-3fbb-45ed-9f5e-f908ac3996ca
📒 Files selected for processing (2)
src/solana/adl.tstest/adl.test.ts
Comment on lines
+205
to
+210
| let dominantSide: AdlSide | null = null; | ||
| try { | ||
| const engine = parseEngine(slabData); | ||
| pnlPosTot = engine.pnlPosTot; | ||
| // Ties resolve to "long" to match the on-chain target_side log convention. | ||
| dominantSide = engine.shortOi > engine.longOi ? "short" : "long"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't derive a real dominant side from missing per-side OI.
parseEngine() succeeds on some older layouts by filling engine.longOi / engine.shortOi with 0n when those offsets do not exist. This branch then turns that “unknown” state into "long", so buildAdlTransaction() stops using the ranking.ranked[0] fallback and instead biases to ranking.longs[0]. Please leave dominantSide as null unless the detected layout actually exposes both per-side OI fields.
🤖 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/solana/adl.ts` around lines 205 - 210, The dominant-side detection in
parseEngine()/buildAdlTransaction is treating missing per-side OI as a real tie
and defaulting to "long", which incorrectly overrides the ranking.ranked[0]
fallback. Update the dominantSide assignment so it remains null unless the
detected slab layout actually includes both longOi and shortOi fields; only
derive "long"/"short" when those offsets are present, and keep the existing
fallback behavior unchanged for older layouts.
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.
Summary
The ADL module's own docs state "The most profitable positions on the dominant side are deleveraged first" and "the position at rank 0 of the dominant side is deleveraged first," but
rankAdlPositionsnever computed which side (long/short) actually has greater net open interest.buildAdlTransaction, when called without an explicitpreferSide, fell through toranking.ranked[0]— the single best PnL% position merged across BOTH sides, not the dominant side's top position.Per the on-chain devnet log format (
"net_long_oi=500000 net_short_oi=500000 target_side=long candidates=0"), the on-chain ADL engine gates eligibility by net-OI dominant side and resolves ties to"long". A caller relying on the SDK's documented default behavior without manually passingpreferSidecould have the SDK pick a position on the minority side, which the program would reject.Note on real-world impact:
encodeExecuteAdl/buildAdlInstructioncurrently always throwremovedInstruction()(ExecuteAdlwas removed from the v17 wrapper, seetest/adl.test.ts), sobuildAdlTransaction's instruction-build path is not reachable end-to-end today. The read-side ranking functions (rankAdlPositions,fetchAdlRankedPositions) remain exported, though, and could mislead an external keeper/dashboard that trustsranking.ranked[0]as "the next ADL victim" without separately checking OI dominance.Fix
rankAdlPositionsnow computesengine.longOivsengine.shortOiand exposes the result asAdlRankingResult.dominantSide("long" | "short" | null—nullonly when engine state can't be parsed at all). Ties resolve to"long"to match the on-chain convention.buildAdlTransaction's default branch (nopreferSide) now targetsranking.dominantSide's top position, falling back to the globalranked[0]only whendominantSideis null.Test plan
test/adl.test.ts:dominantSideis"long"/"short"correctly fromengine.longOi/engine.shortOi, ties resolve to"long", anddominantSideisnullwhen engine parsing fails entirely (bad magic). Also added a pure-logic test forbuildAdlTransaction's target-side branch selection.Summary by CodeRabbit
New Features
Bug Fixes