From 6edf3d2ef9855483b7be672a820ff0cb758a759b Mon Sep 17 00:00:00 2001 From: luisleo526 Date: Tue, 30 Jun 2026 20:26:35 +0800 Subject: [PATCH] fix(orders): close(id) closes logical unclosed qty, not physical lots (grid bots) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit strategy.close(id) under the default FIFO close-entries rule summed every physical open lot carrying `id` to decide the close quantity. TradingView instead closes the quantity entered under `id` that a prior close(id) has not already targeted — it does not re-sum physical lots. The two agree when each id maps to one open lot, but diverge for grid bots that re-use a single entry id across sequential buy/sell cycles: the FIFO trade-record drain removes the oldest lot (often a different id), leaving the id-tagged lot physically open, so the next close(id) double-counts it (over-close) — and a TP whose id-lot was already drained finds no physical match and is skipped (under-close). Add a per-id `id_unclosed_qty_` ledger ("entered qty for id minus already-closed-by-close(id) qty") maintained at every entry site and consumed by compute_close_target_qty under the FIFO rule only. The ANY rule keeps the physical id-matched path untouched. 3commas grid corpus: xrp-grid 26.7%->99.3%, pol-grid 21.5%->99.1% trade-for- trade match vs TradingView (price-exact 100%). Corpus held 251 excellent / 1 anomaly; ctest 76/76 incl. new test_strategy_close_reused_id_closes_one_logical_slot (fails pre-fix). No new C-ABI export. Co-Authored-By: Claude Opus 4.8 (1M context) --- include/pineforge/engine.hpp | 17 ++++++++++ src/engine_fills.cpp | 3 ++ src/engine_orders.cpp | 4 +++ src/engine_strategy_commands.cpp | 38 ++++++++++++++++++++++ tests/test_integration.cpp | 55 ++++++++++++++++++++++++++++++++ 5 files changed, 117 insertions(+) diff --git a/include/pineforge/engine.hpp b/include/pineforge/engine.hpp index 9334e6d..d4eb22e 100644 --- a/include/pineforge/engine.hpp +++ b/include/pineforge/engine.hpp @@ -267,6 +267,23 @@ class BacktestEngine { int position_entry_count_ = 0; // number of entries in current direction (for pyramiding) int position_open_bar_ = -1; // bar_index_ when position was opened (for exit delay) std::vector pyramid_entries_; // individual entries for trade reporting + // Per-entry-id UNCLOSED quantity ledger, used ONLY by strategy.close(id) + // under the default FIFO close-entries rule to decide how much to close. + // + // TradingView's strategy.close(id) closes the quantity of the entries + // tagged `id` that have NOT already been targeted by a prior close(id) — + // it does NOT re-sum the physical open lots. That distinction is + // invisible when each id maps to one lot (the common case), but it is + // load-bearing for strategies that re-use one entry id across sequential + // buy/sell cycles (grid bots): there, the FIFO trade-record drain removes + // the OLDEST physical lot — which may carry a different id — leaving the + // id-tagged lot physically present. Summing physical lots would then + // double-count it on the next close(id) and over-close. This ledger + // tracks "entered qty for id minus already-closed-by-close(id) qty for + // id", so close(id) closes exactly the right amount (the whole position + // for a same-id DCA pyramid; one slot for a grid cycle). It is never read + // under the ANY rule (which closes id-matched physical lots directly). + std::unordered_map id_unclosed_qty_; // --- Strategy parameters (set from strategy() declaration) --- double initial_capital_ = 1000000.0; diff --git a/src/engine_fills.cpp b/src/engine_fills.cpp index 355b560..836f979 100644 --- a/src/engine_fills.cpp +++ b/src/engine_fills.cpp @@ -811,7 +811,9 @@ void BacktestEngine::apply_raw_order_fill(PendingOrder& order, double fill_price position_open_bar_ = bar_index_; trail_best_price_ = fill_price; pyramid_entries_.clear(); + id_unclosed_qty_.clear(); pyramid_entries_.push_back({fill_price, current_bar_.timestamp, qty, order.id, bar_index_}); + id_unclosed_qty_[order.id] += qty; if (!std::isnan(order.stop_price) || !std::isnan(order.limit_price)) { set_entry_fill_excursion_masks(pyramid_entries_.back(), current_bar_, fill_price); } @@ -860,6 +862,7 @@ void BacktestEngine::apply_raw_order_fill(PendingOrder& order, double fill_price position_entry_count_++; trail_best_price_ = fill_price; pyramid_entries_.push_back({fill_price, current_bar_.timestamp, new_qty, order.id, bar_index_}); + id_unclosed_qty_[order.id] += new_qty; if (is_priced_entry) { set_entry_fill_excursion_masks(pyramid_entries_.back(), current_bar_, fill_price); } diff --git a/src/engine_orders.cpp b/src/engine_orders.cpp index 905ac15..2692c69 100644 --- a/src/engine_orders.cpp +++ b/src/engine_orders.cpp @@ -450,6 +450,7 @@ void BacktestEngine::reset_position_state_to_flat() { position_open_bar_ = -1; trail_best_price_ = std::numeric_limits::quiet_NaN(); pyramid_entries_.clear(); + id_unclosed_qty_.clear(); consumed_partial_exit_ids_.clear(); } @@ -488,8 +489,10 @@ void BacktestEngine::open_fresh_position(PositionSide requested, double fill_pri position_open_bar_ = bar_index_; trail_best_price_ = fill_price; pyramid_entries_.clear(); + id_unclosed_qty_.clear(); consumed_partial_exit_ids_.clear(); pyramid_entries_.push_back({fill_price, current_bar_.timestamp, qty, id, bar_index_}); + id_unclosed_qty_[id] += qty; } @@ -644,6 +647,7 @@ void BacktestEngine::add_to_pyramid_market(const std::string& id, bool is_long, position_entry_count_++; trail_best_price_ = fill_price; pyramid_entries_.push_back({fill_price, current_bar_.timestamp, new_qty, id, bar_index_}); + id_unclosed_qty_[id] += new_qty; } diff --git a/src/engine_strategy_commands.cpp b/src/engine_strategy_commands.cpp index 435b061..6082075 100644 --- a/src/engine_strategy_commands.cpp +++ b/src/engine_strategy_commands.cpp @@ -452,6 +452,44 @@ bool BacktestEngine::compute_close_target_qty(const std::string& id, double& matching_qty_out, double& qty_to_close_out, bool& all_entries_match_out) { + const double eps0 = kQtyEpsilon; + // Default FIFO close-entries rule: strategy.close(id) with no explicit + // qty/qty_percent closes the UNCLOSED quantity tagged `id` + // (id_unclosed_qty_) and FIFO-attributes the resulting trade records to + // the OLDEST open entries (handled downstream by the plain FIFO drain). + // + // This is what TradingView does: close(id) closes the quantity entered + // under `id` and not yet targeted by a prior close(id) — it does NOT + // re-sum the physical open lots carrying that id. The two agree when each + // id maps to a single open lot. They diverge for grid bots that re-use + // one entry id across sequential buy/sell cycles: the FIFO trade-record + // drain removes the oldest lot (often a DIFFERENT id), so the id-tagged + // lot stays physically open even though a prior close(id) already + // accounted for it. Summing physical lots then double-closes it (engine + // over-closes 2x), while a TP whose id-lot was drained away by an earlier + // close would find no physical match and be skipped (engine under-closes) + // — both fixed here by consulting the logical ledger instead. + // + // The ANY rule keeps the physical id-matched path (closes_any_qty). + if (!close_entries_rule_any_ && !id.empty() + && std::isnan(qty) && std::isnan(qty_percent)) { + auto it = id_unclosed_qty_.find(id); + double unclosed = (it != id_unclosed_qty_.end()) ? it->second : 0.0; + double target = std::min(unclosed, position_qty_); + all_entries_match_out = false; // FIFO drain may span lots of other ids + if (target <= eps0) { + return false; + } + matching_qty_out = target; + qty_to_close_out = target; + // Consume the id's unclosed ledger now that the close commits. + it->second -= target; + if (it->second <= eps0) { + id_unclosed_qty_.erase(it); + } + return true; + } + bool has_matching_entry = id.empty(); all_entries_match_out = id.empty() ? true : !pyramid_entries_.empty(); matching_qty_out = id.empty() ? position_qty_ : 0.0; diff --git a/tests/test_integration.cpp b/tests/test_integration.cpp index 02afc6a..2c22432 100644 --- a/tests/test_integration.cpp +++ b/tests/test_integration.cpp @@ -3771,6 +3771,60 @@ static void test_strategy_close_pooc_fifo_only_closes_requested_leg_size() { CHECK(strat.get_open_entry_id() == "Buy2"); } +// Grid-bot pattern: the SAME entry id is re-used across sequential +// buy/close cycles. Under the default FIFO close-entries rule, the trade +// record drains the OLDEST physical lot (a different id), so the id-tagged +// lot stays physically open after its close. A later re-entry of that id then +// leaves TWO physical lots carrying it, while only ONE is logically unclosed. +// strategy.close(id) must close ONE slot (the logical/unclosed quantity), not +// the physical sum of both lots — otherwise it over-closes 2x (the bug this +// guards). Mirrors the 3Commas grid-bot corpus strategies. +static void test_strategy_close_reused_id_closes_one_logical_slot() { + std::printf("test_strategy_close_reused_id_closes_one_logical_slot\n"); + + class Strat : public BacktestEngine { + public: + Strat() { + initial_capital_ = 1'000'000; + default_qty_type_ = QtyType::FIXED; + default_qty_value_ = 1.0; + commission_value_ = 0.0; + slippage_ = 0; + process_orders_on_close_ = true; + pyramiding_ = 10; + } + void on_bar(const Bar&) override { + switch (bar_index_) { + case 0: strategy_entry("A", true); break; // older, different id + case 1: strategy_entry("L", true); break; // L lot #1 + case 2: strategy_close("L"); break; // drains A (FIFO), L#1 stays + case 3: strategy_entry("L", true); break; // L lot #2 (re-use id) + case 4: strategy_close("L"); break; // must close ONE slot, not two + default: break; + } + } + double pos() const { return signed_position_size(); } + int open_lots() const { return static_cast(pyramid_entries_.size()); } + }; + + Strat strat; + Bar bars[] = { + {100.0, 101.0, 99.0, 100.0, 50, 900'000}, + { 90.0, 91.0, 89.0, 90.0, 50, 1'800'000}, + {110.0, 111.0, 109.0, 110.0, 50, 2'700'000}, + { 80.0, 81.0, 79.0, 80.0, 50, 3'600'000}, + {120.0, 121.0, 119.0, 120.0, 50, 4'500'000}, + }; + strat.run(bars, 5); + + // Two close("L") calls each close exactly one unit: 2 closed trades, and + // one L lot (qty 1) remains open. The pre-fix sum-of-id behaviour closed + // both physical L lots on bar 4 (3 trades, flat) — that is the regression. + CHECK(strat.trade_count() == 2); + CHECK(near(strat.pos(), 1.0, 1e-9)); + CHECK(strat.open_lots() == 1); +} + static void test_strategy_close_pooc_sets_exit_comment() { std::printf("test_strategy_close_pooc_sets_exit_comment\n"); @@ -4081,6 +4135,7 @@ int main() { test_strategy_close_pooc_partial_close_keeps_other_exit_bracket(); test_strategy_close_fifo_only_closes_requested_leg_size(); test_strategy_close_pooc_fifo_only_closes_requested_leg_size(); + test_strategy_close_reused_id_closes_one_logical_slot(); test_strategy_close_pooc_sets_exit_comment(); test_strategy_close_qty_percent_reduces_position(); test_strategy_close_qty_reduces_position();