Skip to content

Return realized swap deltas on trade results; fix TakerClosed binding#59

Merged
Praz314159 merged 2 commits into
mainfrom
feat/realized-swap-results
Jun 29, 2026
Merged

Return realized swap deltas on trade results; fix TakerClosed binding#59
Praz314159 merged 2 commits into
mainfrom
feat/realized-swap-results

Conversation

@Praz314159

@Praz314159 Praz314159 commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

open_taker/adjust_taker now decode the taker swap from the receipt (reusing the feed's decode_log + SwapInfo) and return realized perp_delta/usd_delta on OpenResult/AdjustTakerResult — actual fills, not estimates. Both 0.0 for maker opens / margin-only adjusts (no swap).

Fixes the TakerClosed binding: the deployed contract (ahead of the v0.2.1 tag) unifies close+liquidation, so the event carries two extra fields (uint256 liqFee, bool isLiquidation). Verified against the live topic0 0xc6d156…; added a SIGNATURE_HASH assertion to abi_lock so deployment drift is caught (the prior test only checked binding self-consistency). Fork test now asserts realized deltas on open/adjust/close.

Summary by CodeRabbit

  • New Features
    • Added realized swap details to taker position results; taker opens, adjustments, and closes now return perp_delta and usd_delta.
  • Bug Fixes
    • Maker opens now consistently report perp_delta/usd_delta as 0.0.
    • Improved receipt/event handling for taker open/adjust/close so realized deltas are sourced from contract events, and close details are enriched when available.
  • Tests
    • Expanded fork-based tests to assert expected signs/magnitudes for realized perp_delta/usd_delta, including tolerance checks.

open_taker/adjust_taker now decode the taker swap from the receipt
(reusing the feed's decode_log + SwapInfo) and return realized
perp_delta/usd_delta on OpenResult/AdjustTakerResult — actual fills, not
estimates. Both 0.0 for maker opens / margin-only adjusts (no swap).

Fixes the TakerClosed binding: the deployed contract (ahead of the v0.2.1
tag) unifies close+liquidation, so the event carries two extra fields
(uint256 liqFee, bool isLiquidation). Verified against the live topic0
0xc6d156…; added a SIGNATURE_HASH assertion to abi_lock so deployment
drift is caught (the prior test only checked binding self-consistency).
Fork test now asserts realized deltas on open/adjust/close.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 25fd7730-3736-4c3e-95c1-611aa6878f8e

📥 Commits

Reviewing files that changed from the base of the PR and between dbd966e and cc51535.

📒 Files selected for processing (1)
  • src/client/trades.rs

📝 Walkthrough

Walkthrough

Adds perp_delta and usd_delta to taker result types, updates the TakerClosed event ABI, decodes receipt logs to fill the new fields, and extends fork tests to verify realized swap values.

Changes

Taker Swap Delta Extraction

Layer / File(s) Summary
TakerClosed ABI and signature update
src/contracts.rs
TakerClosed gains liqFee and isLiquidation, and the ABI-lock test checks the updated signature and deployed topic0 hash.
OpenResult and AdjustTakerResult extended with swap deltas
src/types.rs
OpenResult and AdjustTakerResult gain perp_delta and usd_delta; AdjustTakerResult drops Eq; serde round-trip tests include the new fields.
Receipt log parsing and result population
src/client/trades.rs
Adds receipt-log decoding for taker swap events and populates realized deltas in open_taker, open_maker, and adjust_taker.
Fork test assertions for swap deltas
tests/anvil_fork.rs
The fork test now logs and asserts realized perp_delta and usd_delta after open, adjust, and close actions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Possibly related PRs

Poem

🐇 I hopped through logs and found the trace,
Two deltas blinked in their place.
Open, adjust, and close all sing,
With perp and usd in sync they spring.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately summarizes the main changes: realized trade deltas and the TakerClosed ABI fix.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/realized-swap-results

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
tests/anvil_fork.rs (1)

402-409: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the close delta magnitude too.

The close test only checks signs, so a wrong decoded negative swap would pass. Since close_taker(pos_id, 0.0005, ...) should realize about -0.0005, mirror the open/adjust magnitude check here.

Proposed test tightening
-    assert!(
-        close_result.perp_delta < 0.0 && close_result.usd_delta > 0.0,
+    assert!(
+        (close_result.perp_delta - (-0.0005)).abs() < 1e-4 && close_result.usd_delta > 0.0,
         "close should sell perp and receive USD, got perp_delta={} usd_delta={}",
         close_result.perp_delta,
         close_result.usd_delta
     );
🤖 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 `@tests/anvil_fork.rs` around lines 402 - 409, The close assertion in the taker
close test only checks delta signs, so a wrongly decoded negative swap could
still pass. Tighten the check in the close-path test around close_taker and the
TakerClosed decoding by asserting the perp_delta magnitude matches the expected
close size (mirroring the open/adjust magnitude check) in addition to keeping
the sign checks. Use the existing close_result.perp_delta and
close_result.usd_delta assertions to validate both direction and approximate
size.
🤖 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/client/trades.rs`:
- Around line 122-127: `open_taker` is silently treating a missing taker swap
decode as a valid zero fill by using `parse_taker_swap(&receipt).unwrap_or((0.0,
0.0))`, which can mask ABI or log-decoding issues. Update the taker flow in
`src/client/trades.rs` so `open_taker` and non-zero `adjust_taker` require a
decoded `TakerOpened`/`TakerAdjusted`/`TakerClosed` event and fail or surface an
error when parsing is missing, while keeping the `(0.0, 0.0)` fallback only for
maker opens and margin-only adjusts. Use the existing `parse_taker_swap`,
`open_taker`, and `adjust_taker` paths to apply the stricter handling
consistently.

---

Nitpick comments:
In `@tests/anvil_fork.rs`:
- Around line 402-409: The close assertion in the taker close test only checks
delta signs, so a wrongly decoded negative swap could still pass. Tighten the
check in the close-path test around close_taker and the TakerClosed decoding by
asserting the perp_delta magnitude matches the expected close size (mirroring
the open/adjust magnitude check) in addition to keeping the sign checks. Use the
existing close_result.perp_delta and close_result.usd_delta assertions to
validate both direction and approximate size.
🪄 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: a0c7fc7b-8248-499f-8724-31cd4cb3d393

📥 Commits

Reviewing files that changed from the base of the PR and between 0a3f56d and dbd966e.

📒 Files selected for processing (4)
  • src/client/trades.rs
  • src/contracts.rs
  • src/types.rs
  • tests/anvil_fork.rs

Comment thread src/client/trades.rs Outdated
open_taker / adjust_taker no longer fall back to (0.0, 0.0) when
parse_taker_swap returns None — that fallback silently masked the
TakerClosed binding bug (a full close reported a zero fill). Both taker
paths always emit a decodable event (a margin-only adjust emits a
zero-delta TakerAdjusted), so a missing decode is an ABI error: surface
ContractError::EventNotFound instead. open_maker's explicit 0.0 (it emits
no taker swap) is unchanged. Addresses CodeRabbit review.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@Praz314159
Praz314159 merged commit 6f74cbc into main Jun 29, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant