Skip to content

Reset fill history when editing partially filled orders - #690

Open
Taz33m wants to merge 1 commit into
opexdev:mainfrom
Taz33m:codex/modify-after-fill-regression
Open

Reset fill history when editing partially filled orders#690
Taz33m wants to merge 1 commit into
opexdev:mainfrom
Taz33m:codex/modify-after-fill-regression

Conversation

@Taz33m

@Taz33m Taz33m commented Jul 20, 2026

Copy link
Copy Markdown

Fixes the matching-semantic part of #688. The separate performance observations in that issue are intentionally out of scope here.

What changed

  • Reset filledQuantity when an edit rebuilds an order through the existing cancel-and-reinsert path.
  • Add a native lifecycle regression: rest 10, fill 4, edit the remaining order to quantity 6 at a new price, then consume all 6 with an IOC.

Before the fix, the edited order carried the old fill count of 4 and exposed only 2 units. Against the original line, the new test fails with expected: <0> but was: <4>; with the fix, the requested 6 units rest and fill.

Verification

JAVA_HOME=<jdk-17> mvn -B -pl matching-engine/matching-engine-core -am \
  -Dskip.unit.tests=false -Dsurefire.failIfNoSpecifiedTests=false \
  -Dtest=SimpleOrderBookUnitTest test

Result: 18 passed, 0 failed under Java 17.

@Taz33m

Taz33m commented Jul 27, 2026

Copy link
Copy Markdown
Author

@fatemeh-i, could you review this two-file partial-fill → edit → IOC regression/fix when you have a chance?

@fatemeh-i

fatemeh-i commented Jul 28, 2026

Copy link
Copy Markdown
Member

@Taz33m Thank you so much for creating this PR and for all your help and contributions to the project. We really appreciate your support!

As you can see in [PR #693](#693) (Fix/order acceptance consistency), we are currently working on refactoring the SimpleOrderBook. One of the approaches we are considering is removing the EditOrderCommand completely.

Our business team has suggested that, from a user-experience perspective, exchanges should use a cancel + create new order flow instead of editing an existing order. We are still discussing and evaluating this decision with the team.

Once we finalize the decision, if we decide to keep the edit functionality, we will definitely apply your PR as well.

Thanks again for taking the time to identify this issue, add a regression test, and provide the fix. We really appreciate it!

@Taz33m

Taz33m commented Jul 28, 2026

Copy link
Copy Markdown
Author

Thanks for the context—that makes sense. If EditOrderCommand is removed, please feel free to close this PR as superseded. If edit remains part of the API, the regression and fix are ready to rebase onto the refactor. No action needed from you until that decision is made.

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