Skip to content

fix(order): skip non-positive match candidates - #44

Merged
phroi merged 3 commits into
masterfrom
review/order-nonpositive-gain
May 27, 2026
Merged

phroi merged 3 commits into
masterfrom
review/order-nonpositive-gain

Conversation

@phroi

@phroi phroi commented May 27, 2026

Copy link
Copy Markdown
Member

Why

OrderManager.bestMatch counted non-positive-gain partial candidates as rejected, but the same candidate could still become the returned best match when the empty frontier was allowance-rejected. That allowed a match with zero or negative economic gain to be selected.

Changes

  • Keep match-search frontier advancement separate from the returned best profitable match.
  • Leave no-match diagnostics at bestGain: 0n instead of the sentinel value.
  • Add a regression test covering a non-positive candidate after an allowance-rejected empty frontier.
  • Align the exhaustive test oracle with the production invariant that non-empty selected matches must have positive gain.

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request refactors the order matching logic in the order manager. It decouples the frontier advancement tracking from the best match tracking by introducing a separate advance state variable, and initializes bestGain to 0n instead of a large negative number. It also ensures that candidates with non-positive gains are rejected early when partial matches exist. A corresponding unit test has been added to verify that non-positive candidates are not selected after rejecting empty allowances.

@phroi

phroi commented May 27, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request refactors the order matching logic in OrderManager.bestMatch and exhaustiveSequentialBestMatch to ensure non-positive candidates are not selected after rejecting empty allowances. It decouples the search frontier progression (using a new advance state) from the tracking of the best match. A unit test was also added to verify this behavior. The review feedback points out that ckbAllowance and udtAllowance are initialized in the best object but never used, suggesting their removal to simplify the code.

Comment thread packages/order/src/order.ts
@phroi

phroi commented May 27, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request refactors the order matching logic in the order manager and its tests. It separates the tracking of the best match from the search frontier advancement, initializes the best gain to 0n, and ensures that candidates with non-positive gains are rejected. A review comment points out a redundant condition in the exhaustive sequential best match logic where checking for positive gain is unnecessary because the best gain is already initialized to 0n.

Comment thread packages/order/src/order.test.ts
@phroi

phroi commented May 27, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist 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.

Code Review

This pull request refactors the order matching logic in OrderManager to prevent the selection of candidates with non-positive gains after rejecting empty allowances. It separates the tracking of the optimal match (best) from the frontier exploration state (advance), ensuring that only candidates with positive gains are selected while still allowing the search frontier to advance. Additionally, corresponding unit tests have been added and updated to verify this behavior. There are no review comments to address, and I have no further feedback to provide.

@phroi

phroi commented May 27, 2026

Copy link
Copy Markdown
Member Author

LGTM

Phroi %37

@phroi
phroi merged commit 33d48e2 into master May 27, 2026
1 check 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