Skip to content

fix backtest legacy replacements, log tweak #528

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 1, 2025

Conversation

dvush
Copy link
Contributor

@dvush dvush commented Mar 31, 2025

πŸ“ Summary

πŸ’‘ Motivation and Context


βœ… I have completed the following steps:

  • Run make lint
  • Run make test
  • Added tests (if applicable)

@Copilot Copilot AI review requested due to automatic review settings March 31, 2025 15:17
@dvush dvush requested review from ZanCorDX and ferranbt as code owners March 31, 2025 15:17
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes legacy replacements in the backtest module and adjusts log levels for non-positive block values and unique coinbase profits.

  • Change log levels from warn to info in the redistribution module.
  • Add a new enum variant "Replaced" and update order filtering logic based on order inclusion and replacement.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
crates/rbuilder/src/backtest/redistribute/mod.rs Updated log severity from warn to info when block value or profit isn’t positive.
crates/rbuilder/src/backtest/mod.rs Added a new "Replaced" enum variant and refined order filtering logic to correctly handle replaced orders.
Comments suppressed due to low confidence (3)

crates/rbuilder/src/backtest/redistribute/mod.rs:783

  • The log level was changed from warn to info for a condition that might indicate an issue. Please confirm that the lowered severity is intentional and that monitoring tools do not rely on warning-level alerts here.
info!(?address1, ?address2, newly_included_orders = ?result.new_orders_included, "Joint block value delta is not positive");

crates/rbuilder/src/backtest/mod.rs:161

  • The log message has been updated to 'order was replaced' when filtering orders. Confirm that this change clearly reflects the new filtering reason and that downstream log processing or alerting logic is updated accordingly.
trace!(order = ?id, "order was replaced");

crates/rbuilder/src/backtest/mod.rs:116

  • Consider adding tests to verify that orders present in the included orders set are not filtered out, and that the new replacement logic behaves correctly in various scenarios.
// we never filter included orders even by timestamp

@dvush dvush merged commit 13536be into develop Apr 1, 2025
3 of 4 checks passed
@dvush dvush deleted the redistribution_tweaks_2025_03_31 branch April 1, 2025 16:38
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.

2 participants