fix(hyperliquid): dedup userFills to prevent position/PnL double-count - #18
Open
jenish-25 wants to merge 3 commits into
Open
fix(hyperliquid): dedup userFills to prevent position/PnL double-count#18jenish-25 wants to merge 3 commits into
jenish-25 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
1 issue found across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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
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
The Hyperliquid adapter double-counts fills, corrupting position and realized-PnL tracking.
Message::UserFillsemitted aTradefor every fill with no guard, but Hyperliquid'suserFillssubscription sends a historical snapshot (is_snapshot == true) on the initial subscribe and re-sends it on every transparent reconnect. SinceTradeis the only canonical source of position/PnL:get_positions(), then the snapshot replays those historical fills on top → position ~doubles + phantom PnL.The Bullet adapter already guards this via a
RecentIdsdedup keyed ontrade_id; Hyperliquid didn't. Fixes #17.Fix
RecentIdsFIFO dedup set intobb_core::helpers::RecentIdsand use it from both adapters (removes duplication).fills_to_emit:tids but don't emit them (already in the REST seed);tid-dedup everything after — drops reconnect replays while still surfacing a genuinely new fill that landed during a disconnect gap.Behavior
is_snapshot, first message)tid)tid)tidin a later snapshot)tidTests
RecentIds' dedup + eviction tests tobb-core.fills_to_emitunit tests: initial-snapshot suppression, live-fill-once, reconnect-replay-dropped + gap-fill-emitted, other-coin filtered.cargo test(workspace),cargo clippy --all-targets -- -D warnings, andcargo +nightly fmt --checkall pass.Out of scope
Re-seeding inventory from
get_positions()on reconnect would be a further robustness improvement (strategy-layer, larger) — not needed for this correctness fix. Happy to follow up if you'd prefer that direction.Summary by cubic
Deduplicates Hyperliquid
userFillsto stop position and realized PnL from being double-counted on startup and reconnects. Extracts a shared FIFORecentIdshelper tobb-core, clamps its capacity, and uses it in both adapters.Bug Fixes
tids, do not emit.trade_idafter that to drop reconnect replays; emit gap fills once and fills withouttid.RecentIdscapacity to >= 1 socap = 0can’t disable dedup.fills_to_emitscenarios.Refactors
RecentIdstobb-core::helpersand reused inbulletandhyperliquid.MAX_SEEN_TRADE_IDS = 8192to bound memory while covering reconnect windows.Written for commit 7c4c4f3. Summary will update on new commits.