[fix] gmx-sol - exclude points-farming volume from GMTrade - #8085
Conversation
WalkthroughGMX Solana daily volume now comes from paginated ChangesGMX Solana volume calculation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Adapter
participant GMXGraphQL
participant WalletAggregation
participant DailyVolume
Adapter->>GMXGraphQL: Request paginated tradeEvents
GMXGraphQL-->>Adapter: Return daily trade events
Adapter->>WalletAggregation: Apply position and volume changes
WalletAggregation-->>DailyVolume: Return non-farmer wallet volumes
DailyVolume-->>Adapter: Return dailyVolume
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches✨ Simplify code
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 |
|
The gmx-sol.ts adapter exports: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@dexs/gmx-sol.ts`:
- Line 109: Update the error message in the events-length check to remove the
stray period after the exclamation mark, preserving the existing error condition
and behavior.
- Around line 101-109: Cap the pagination loop in the trade-event retrieval flow
around the events accumulator and request call by enforcing a maximum number of
pages and a timeout for each GraphQL request. Stop or fail clearly when either
limit is reached, while preserving the existing short-page termination and “Not
found daily data!” behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 33888a54-c3ba-488f-866a-fdf1ff2c3e4a
📒 Files selected for processing (1)
dexs/gmx-sol.ts
|
Both fair. Added a cap on the pagination loop so it cannot spin if the subgraph keeps handing back full pages, and reworded the error. The daily numbers are unchanged. |
|
The gmx-sol.ts adapter exports: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dexs/gmx-sol.ts (1)
99-151: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftAdapter should be
version: 2— the subgraph supports timestamp filtering.The SQD subgraph accepts
from/totimestamp parameters (line 105), confirming it supports granular time filtering. Per coding guidelines, subgraphs should useversion: 2withpullHourlyexplicitly set. The currentversion: 1means the adapter runs once daily and usesoptions.startOfDay(a v1-only time anchor), which the guidelines say v2 adapters should not use.Migrating to v2 would require:
- Replacing
options.startOfDaywithoptions.startTimestamp/options.fromTimestamp- Adding
pullHourly: true(orfalsewith a comment if hourly pulling is impractical for farmer detection)- Adapting the farmer classification logic to work on hourly windows (a wallet with 20 trades/day may only have 1–2 trades/hour, falling below the
trades >= 20threshold)If hourly farmer detection is genuinely impractical,
version: 2withpullHourly: falseand an inline comment explaining why would still be more guideline-compliant thanversion: 1.Based on learnings, version selection should be based on data source capabilities (timestamp filtering), not on query expense. Subgraphs explicitly fall into the v2 category. As per coding guidelines, "Use Version 2 when the source supports granular time filtering, such as on-chain logs, contract calls, subgraphs."
🤖 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 `@dexs/gmx-sol.ts` around lines 99 - 151, Update the adapter definition that uses fetch to version 2 because the subgraph supports timestamp filtering. Replace the v1 options.startOfDay anchor in fetch with the v2 start/from timestamp field, and explicitly configure pullHourly; if disabling hourly pulls, add the required rationale comment. Adjust isVolumeFarmer-based classification as needed so farmer detection remains correct for the selected window size.Sources: Coding guidelines, Learnings
🤖 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.
Outside diff comments:
In `@dexs/gmx-sol.ts`:
- Around line 99-151: Update the adapter definition that uses fetch to version 2
because the subgraph supports timestamp filtering. Replace the v1
options.startOfDay anchor in fetch with the v2 start/from timestamp field, and
explicitly configure pullHourly; if disabling hourly pulls, add the required
rationale comment. Adjust isVolumeFarmer-based classification as needed so
farmer detection remains correct for the selected window size.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: af8d10ea-c548-4dc6-8176-0d3a8dbdeecb
📒 Files selected for processing (1)
dexs/gmx-sol.ts
|
Fair. Ran it myself: 07-17 comes out 1531M with and without the filter, because nothing hits the thresholds that day. The farmers just stopped. The filter is after the flat-book GT churn from #7120, and that was big in May/June and has died off: date raw$M removed So it strips a lot on the days they're active (39% on 05-20), and there's nothing of that shape left now. The 1531M on 07-17 is ~4700 wallets, biggest ones at 3-18x turnover, none running a flat book at 20x+ the way the farmers do. The couple of flat-book wallets still around are at 12-14x and about 1% of the day, so dropping the threshold wouldn't move the number and starts catching normal traders. The vol/OI thing is separate. This only pulls the farmers, it doesn't touch the ratio. If you want the ratio itself down that's a wider call on what counts as non-economic and needs its own rule. Either way works for me, can share the day by day if you want it. |
…lpers The flat-book test assumed a points farmer runs a hedged book. Not all do. On 2026-06-01 the largest wallet on the venue turned over 24.6x on $28.6M with imbalance 0.36, so it passed: the filter caught 18.2% of that day and left 39.8% behind. Dropping the test outright is worse. It also protects a different population that clears 20x turnover, directional scalpers working one or two positions with small capital, and those dominate early history. On 2025-03-15 and 2026-02-15 turnover alone takes 60.7% and 71.7% of the day off wallets holding 1 to 7 positions on $39k to $755k of capital. Backfill runs to 2025-02-12, so that would have rewritten real history. So the rule is churn plus either a flat book or a wide one: turnover >= 20 and (imbalance <= 0.15 or positions >= 8). day before after 2025-03-15 8.0% 8.0% 2026-02-15 0.0% 0.0% 2026-05-20 38.7% 45.0% 2026-06-01 18.2% 55.4% 2026-06-04 3.8% 12.6% 2026-07-10 0.0% 0.0% 2026-07-17 0.0% 0.0% Thresholds of 8, 10 and 15 positions give identical results, so the cut is structural rather than fitted.
openSize.size is the number of distinct positions the wallet touched, because closing sets the entry to 0 rather than removing the key. Reading it next to 'current size in usd' suggests it means positions still open, which is not what the threshold is doing. Renamed to positionSize and said in the comment that deleting closed keys would change the filter.
Two claims in the comment were measured before the position disjunct existed and no longer hold. Quiet days are not insensitive to turnover any more. At 10 the 2026-07-17 day goes 1.53B to 1.37B, so 20 is load bearing on the low side as well as the high side, and the note now says so. The position threshold is flat from 8 to 20 rather than just 8, 10 and 15, and 6 is where it starts reaching the 2025 scalpers. That is the useful part: it gives the floor.
Recomputed the comment's figures from the subgraph with a probe that does not share code with the adapter. Two were wrong. The protected wallets hold $3k to $755k, not $39k to $755k. The maxima were right and the minimum was an order of magnitude out, which is what happens when a number gets carried forward from an earlier subset instead of re-measured. Added the 2026-02-15 leader as the concrete case, 221x turnover on $78k across one position at imbalance 1.00. The methodology string said wallets turning over 'more than 20x' are excluded. The test is >= 20 and the smallest wallet in the 2026-05-20 set is at exactly 20.0x, so that sentence excluded a wallet the code includes.
The comment claimed flagged wallets carry no untouched positions into the day and called it nil in practice. That cannot be checked: an untouched position produces no trade event and the subgraph has no position entity, so its capital is invisible either way. The claim had also been checked against the old rule's 15 wallets and was never rechecked after the rule started flagging 42. It is false as stated. On 2026-06-01 the flagged wallets carry in a median 54.8% of their peak capital and none end the day flat. Measured capital is a lower bound, so turnover is an upper bound, and the result is sensitive to it: inflating peak capital gives 55.4% removed at +0%, 52.7% at +10%, 32.7% at +25%, 9.3% at +50%. The comment now carries those numbers plus what limits the risk, which is that these wallets open 88% of their positions fresh in-day and the leader trades 3079 times across 76 of them.
The pagination cap I added earlier exits the loop by running out of budget when every page comes back full. There is no short page and no break, so the day is only partly read and the adapter goes on to report that partial total as the whole day. Forcing it with MAX_EVENTS=100 on 2026-06-01 returns 2.64M for a day that is 1.97B, no error, no warning. A wrong number that looks like a plausible number is worse than a crash. A short page is the only evidence the day was fully read, so track it and throw otherwise. The busiest day so far is 129k events against a cap of 1m, so this is headroom rather than a live problem, but this venue went from single-digit millions a day to billions inside a year.
MIN_TRADES never did anything. No single trade can move a position by more than the wallet's peak exposure, since the position's own size is bounded by the peak it contributed to, so volume <= trades * peak and turnover <= trades. Requiring turnover >= 20 therefore already requires 20 trades. Measured it too: across six days including every farming day the guard excludes zero wallets and moves the result by 0.0000pp. Removing it takes the trades counter with it. The comment was a 57 line block, the longest run in dexs/ by twenty lines, in a directory where most adapters carry none. Much of it was per-day statistics that belong in the PR and would be stale in a month. Kept the metric definitions, why both branches of the test exist, and the idle-capital limit, which is the one thing a future reader has to know before touching the thresholds. Also folded the position-size update into a single map lookup instead of has/get/set, which drops the 'as number' assertion. No behaviour change: 06-01 1.97B, 05-20 2.89B, 07-17 1.53B, 2025-03-15 368.67M, 2026-02-15 46.56M, 07-10 618.75M, all identical before and after.
|
The gmx-sol.ts adapter exports: |
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 `@dexs/gmx-sol.ts`:
- Around line 102-105: Update the from timestamp construction in fetch to use
options.startTimestamp instead of options.startOfDay, while preserving the
existing conversion to milliseconds and ISO format. Keep the to timestamp based
on options.endTimestamp unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9a2c6223-3caa-4d75-b7e9-e2e9668f18cb
📒 Files selected for processing (1)
dexs/gmx-sol.ts
| const fetch = async (options: FetchOptions) => { | ||
| const from = new Date(options.startOfDay * 1000).toISOString(); | ||
| const to = new Date(options.endTimestamp * 1000).toISOString(); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use options.startTimestamp instead of options.startOfDay for time windows.
As per coding guidelines, use options.startTimestamp (or fromTimestamp) for event-based time windows rather than startOfDay. If this adapter runs hourly, startOfDay will incorrectly aggregate all events from midnight up to the end of the current hour, leading to grossly inflated day-to-date cumulative volume being reported for a single hour.
🛠 Proposed fix
const fetch = async (options: FetchOptions) => {
- const from = new Date(options.startOfDay * 1000).toISOString();
+ const from = new Date(options.startTimestamp * 1000).toISOString();
const to = new Date(options.endTimestamp * 1000).toISOString();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fetch = async (options: FetchOptions) => { | |
| const from = new Date(options.startOfDay * 1000).toISOString(); | |
| const to = new Date(options.endTimestamp * 1000).toISOString(); | |
| const fetch = async (options: FetchOptions) => { | |
| const from = new Date(options.startTimestamp * 1000).toISOString(); | |
| const to = new Date(options.endTimestamp * 1000).toISOString(); |
🤖 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 `@dexs/gmx-sol.ts` around lines 102 - 105, Update the from timestamp
construction in fetch to use options.startTimestamp instead of
options.startOfDay, while preserving the existing conversion to milliseconds and
ISO format. Keep the to timestamp based on options.endTimestamp unchanged.
Source: Coding guidelines
mkzung
changed the title
|
Following up on my own numbers above - the 18% I gave for 06-01 was too low and The filter wanted a flat book, on the assumption a farmer hedges. Not all do. The That moves the two active days in my table: 05-20 goes from 39% to 45% removed and My first attempt was to just drop the flat-book test, which was wrong - it also |
2 participants
Closes #7120.
GMTrade volume on GMX Solana includes wallets farming the GT points programme.
This excludes them.
How it works
Three numbers per wallet per day:
Flagged when
turnover >= 20and eitherimbalance <= 0.15orpositions >= 8.On 2026-05-20 that is 17 wallets holding $2.36B, the thirteen largest carrying 94%
of it: $3.9M to $11.0M of capital each, 29 to 41 positions, 20.0x to 32.8x
turnover. That is one operation rather than a population of traders.
The second branch exists because not every farmer hedges. On 2026-06-01 the
largest wallet on the venue ran 24.6x turnover on $28.6M across 76 positions at
imbalance 0.36, and a flat-book test alone let it through. The position count is
what keeps the wider test away from directional scalpers on one or two positions,
which is the other population that clears 20x turnover and which dominates the
early backfill.
Effect
Sampling every 14th day from
startto now instead of days I chose: 38 days, 2change at all, none above 60% removed, every 2025 day identical.
Thresholds
Both are bounded on both sides rather than fitted.
Positions 8, 10, 15 and 20 give identical output on every day tested; 6 starts
reaching into 2025-03-15 (368.67 M to 348.97 M), so 8 sits just above the floor.
Turnover at 20 leaves quiet and historical days identical to the unfiltered
adapter. At 10 an ordinary day loses real volume (2026-07-17, 1.53 B to 1.37 B).
Above 30 the signal itself starts to go.
Limitation
A position that never trades during the day emits no event, and the subgraph has
no position entity, so its capital is invisible. Capital carried in is counted,
idle capital is not, which makes measured turnover an upper bound. Inflating peak
capital to stand in for idle positions, 2026-06-01 removes 55.4% at +0%, 52.7% at
+10%, 32.7% at +25%, 9.3% at +50%, and the 06-01 leader is only 23% clear of the
threshold at 24.6x. What limits this is that flagged wallets open a median 88% of
their positions fresh in-day and the leader trades 3,079 times across 76 of them.
Wallets near 20x have no such margin, which is the reason not to lower it.
Why not the alternatives
Fee rate does not work:
totalFeesis a bucket (price impact, borrowing,liquidation, funding), as
fees/gmx-sol.tsdocuments, so a threshold on it fitsnoise.
Hold time does not work at the scale suggested in #7120. Implementing ">= 90% of
closes held under 60s" literally flags 4 wallets and 0.095% of the 2026-05-20 day,
because only 1.9% of that day's 37,003 closes were held under 60s. The variable is
pointing at something real, but the cut is orders of magnitude off.
This is not self-matched wash trading. GMX Solana is pool-based with no orderbook,
so a wallet cannot trade against itself. It is real trading against the pool that
exists to earn points, which is why the question of whether to exclude it is a
judgement call rather than an obvious bug.
Verification
Recomputing volume from
tradeEventsreproduces the subgraph's ownvolumeRecordDailies.tradeVolumeexactly, so with nothing flagged the adapterreturns precisely the number it returns today.