Skip to content

[feature] add optional slippage simulation for paper market orders#20

Closed
theghostmac wants to merge 3 commits into
krakenfx:mainfrom
theghostmac:feature/paper-slippage-simulation
Closed

[feature] add optional slippage simulation for paper market orders#20
theghostmac wants to merge 3 commits into
krakenfx:mainfrom
theghostmac:feature/paper-slippage-simulation

Conversation

@theghostmac

@theghostmac theghostmac commented Mar 23, 2026

Copy link
Copy Markdown
Contributor

Context

Market orders in paper trading filled at the exact best ask/bid with zero slippage, causing paper PnL to consistently overstate what a strategy would achieve live -- especially for larger order sizes, illiquid pairs, and volatile conditions.

This PR adds an optional --slippage flag to paper init and paper reset, following existing --fee-rate pattern.

Brief Changelog

  • Added slippage_rate: f64 to PaperState with #[serde(default)] for backward compat with existing state.json files
  • Added DEFAULT_SLIPPAGE_RATE = 0.0 constant
  • Extended with_fee_rate and reset_with to accept slippage_rate
  • Market buys now fill at ask * (1 + slippage_rate), sells at bid * (1 - slippage_rate)
  • Limit orders unaffected
  • Added --slippage CLI flag to paper init and paper reset
  • Added slippage rate to both human-readable and JSON output
  • Added tests covering buy slippage, sell slippage, zero slippage backward compat, and reset preservation/update

Usage:

Update: flags --slippage-rate or --slippage works.

Init with slippage rate:

./target/debug/kraken paper init --balance 10000 --slippage-rate 0.001
┌──────────────────┬───────────────────────────┐
│ Field            ┆ Value                     │
╞══════════════════╪═══════════════════════════╡
│ Mode             ┆ [PAPER] Simulated Trading │
│ Action           ┆ Account initialized       │
│ Starting Balance ┆ 10000.00 USD              │
│ Fee Rate         ┆ 0.26%                     │
│ Slippage Rate    ┆ 0.10%                     │
└──────────────────┴───────────────────────────┘

Reset slippage rate:

./target/debug/kraken paper reset --slippage-rate 0.002
┌──────────────────┬───────────────────────────┐
│ Field            ┆ Value                     │
╞══════════════════╪═══════════════════════════╡
│ Mode             ┆ [PAPER] Simulated Trading │
│ Action           ┆ Account reset             │
│ Starting Balance ┆ 10000.00 USD              │
│ Fee Rate         ┆ 0.26%                     │
│ Slippage Rate    ┆ 0.20%                     │
└──────────────────┴───────────────────────────┘

Closes #15

@agent-smith-k

Copy link
Copy Markdown
Contributor

Thanks for the contribution — the approach is solid and mirrors the existing --fee-rate pattern well. Core slippage math is correct and tests cover the happy paths. A few items to address before merge:

Must fix

  1. Typo in JSON output key (paper init).
    "slipage_rate" (one p) on line 238 of src/commands/paper.rs.
    paper reset correctly uses "slippage_rate". Any -o json consumer
    will see inconsistent keys depending on the command. One-character fix.

  2. Add a serde backward-compatibility test.
    The current zero_slippage_is_backward_compatible test uses PaperState::new(),
    which always sets slippage_rate via the constructor. It does not prove that an
    older state.json (without the field) deserializes correctly. Please add a test
    similar to deserialize_state_missing_next_order_id that parses JSON without
    slippage_rate and asserts it defaults to 0.0.

Should fix

  1. Missing semicolon in validation block.
    In execute_reset, the slippage validation return Err(...) omits the trailing
    semicolon, while the fee_rate validation block directly above it includes one.
    Both compile, but the inconsistency is noticeable. Add ; for consistency.

  2. Serde default style mismatch.
    fee_rate uses #[serde(default = "default_fee_rate")] with a named function.
    slippage_rate uses bare #[serde(default)], relying on f64::default() == 0.0.
    This works today, but if the default ever changes to nonzero, the serde default
    would silently diverge from DEFAULT_SLIPPAGE_RATE. Consider adding a
    default_slippage_rate function for consistency.

  3. No tests for validation edge cases.
    Both execute_init and execute_reset validate slippage bounds
    (finite, 0.0..=1.0), but there are no tests exercising those paths.
    Worth adding at least: negative rate, rate > 1.0, NaN, and Infinity.

Clarification

  1. Flag name: --slippage vs --slippage-rate.
    The issue text proposes --slippage, but this PR exposes --slippage-rate.
    --slippage-rate is arguably more precise (matches --fee-rate). Either way
    is fine, but please align the issue description or add --slippage as a
    clap alias so both work.

Non-blocking design notes (for follow-up)

  1. with_fee_rate is becoming a positional-params bag.
    It now takes four positional args, two of which are f64 and easy to swap.
    The name no longer describes the function. Not blocking, but a config struct
    or builder would prevent misuse as more options get added.

  2. paper status does not show slippage rate.
    init and reset display it, but status does not. A user who set slippage
    weeks ago has no way to check the value without reading state.json directly.
    Good candidate for a follow-up.

Items 1 and 2 should be fixed before merge. The rest are improvements that would be appreciated but won't block on.

@theghostmac

Copy link
Copy Markdown
Contributor Author

Thank you @agent-smith-k. Addressed both blocking and non-blocking.

@agent-smith-k agent-smith-k left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All findings from the initial review have been addressed. The JSON key typo is fixed, serde defaults are consistent, validation is extracted into a reusable helper with edge-case tests, backward-compat deserialization is tested, --slippage alias works, paper status now surfaces both rates, and PaperConfig eliminates the positional-params concern.

296/296 tests pass. Runtime verification confirms correct JSON output for both paper init and paper status.

One low-severity hardening note for a follow-up: persisted state.json values are trusted on reload without re-validation. Not blocking since the trust boundary is local.

Approving.

@agent-smith-k

Copy link
Copy Markdown
Contributor

Superseded by #24, which contains your changes as a signed squash commit with Co-authored-by credit. Merged to main. Thank you for the contribution!

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.

[paper] Add optional slippage simulation for market orders

2 participants