fix(explorer): solver winner selection fix#7234
Merged
fairlighteth merged 7 commits intodevelopfrom Apr 6, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Contributor
WalkthroughThe PR threads Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client (FillsTableRow)
participant Hook as useTradeSolver
participant Resolver as orderSolverShared.resolveSolverByTxHash
participant API as Solver Competition API
rect rgba(173,216,230,0.5)
UI->>Hook: call useTradeSolver(txHash, orderId)
Hook->>Resolver: resolveSolverByTxHash(networkId, txHash, orderId)
Resolver->>API: fetch competition by txHash
API-->>Resolver: competition data
Resolver->>Resolver: getWinnerSolverFromCompetition(competition, orderId)
alt matching winner found
Resolver-->>Hook: solverInfo
else no matching winner
Resolver-->>Hook: undefined
end
Hook-->>UI: solver state / metadata
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
alfetopito
commented
Mar 31, 2026
| if (!competition?.solutions?.length || !orderId) return undefined | ||
|
|
||
| const winner = competition.solutions.find((s) => s.isWinner) | ||
| const winner = competition.solutions.find((s) => s.isWinner && s.orders?.find((o) => o?.id === orderId)) |
Collaborator
Author
There was a problem hiding this comment.
This is the main fix. The rest is boilerplate to pass down the orderId to this point.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/explorer/src/test/hooks/useTradeSolver.test.tsx (1)
74-134:⚠️ Potential issue | 🟡 MinorAdd a mismatch
orderIdregression test.Current tests only assert matching
orderIdpaths. Please add one case where the winner solution contains a different order id and assertsolverstaysundefined.Suggested test addition
+ it('ignores winner solutions for a different orderId', async () => { + mockedGetSolverCompetitionByTxHash.mockResolvedValueOnce( + mockSolverCompetitionResponse('projectblanc', '0xother-order'), + ) + mockedFetchSolversInfo.mockResolvedValueOnce(MOCK_SOLVERS) + + const { result } = renderHook(() => useTradeSolver('0xtx123', '0xrequested-order')) + + await waitFor(() => expect(result.current.isLoading).toBe(false)) + expect(result.current.solver).toBeUndefined() + })As per coding guidelines, "When changing interface: search all usages with
rg, update every call, remove dead imports; refresh dependency arrays, test edge cases, ensure type safety."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/explorer/src/test/hooks/useTradeSolver.test.tsx` around lines 74 - 134, Add a regression test to useTradeSolver that verifies a mismatch between the winner solution's orderId and the requested orderId yields undefined: arrange mockedGetSolverCompetitionByTxHash to resolve to a SolverCompetitionResponse whose winning solution has a different orderId than the one passed to useTradeSolver (use mockSolverCompetitionResponse or construct a response with auctionId and solutions where solution.orderId !== the test orderId), mock mockedFetchSolversInfo as before, renderHook(() => useTradeSolver(<txHash>, <orderId>Mismatch)), wait for loading to finish, and assert result.current.solver is undefined; reference useTradeSolver, mockedGetSolverCompetitionByTxHash, mockSolverCompetitionResponse, and mockedFetchSolversInfo to locate where to add the test.apps/explorer/src/hooks/useTradeSolver.ts (1)
14-40:⚠️ Potential issue | 🟠 Major
currentKeymust includeorderIdto avoid stale solver state.At Line 14 and Line 42, loading/completion tracking is keyed only by
networkId:txHash, but Line 40 now re-runs onorderIdchanges. Same tx hash + different order id can display stale solver data without loading state.Suggested fix
- const currentKey = txHash && networkId ? `${networkId}:${txHash}` : null + const currentKey = txHash && networkId && orderId ? `${networkId}:${txHash}:${orderId}` : null useEffect(() => { - if (!networkId || !txHash || !currentKey) { + if (!networkId || !txHash || !orderId || !currentKey) { setSolver(undefined) setDoneFor(null) return } let cancelled = false - resolveSolverByTxHash(networkId, txHash, orderId || '') + resolveSolverByTxHash(networkId, txHash, orderId)As per coding guidelines, "When changing interface: search all usages with
rg, update every call, remove dead imports; refresh dependency arrays, test edge cases, ensure type safety."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/explorer/src/hooks/useTradeSolver.ts` around lines 14 - 40, The loading/completion key currentKey must incorporate orderId to avoid showing stale solver state when orderId changes; update the key creation (currentKey) to include orderId (e.g., `${networkId}:${txHash}:${orderId || ''}`) so resolveSolverByTxHash results and the setDoneFor marker are unique per order, keep the same nulling behavior when any required value is missing, and ensure the useEffect dependency array still includes networkId, txHash, currentKey, and orderId so the effect re-runs correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/explorer/src/hooks/useTradeSolver.ts`:
- Around line 14-40: The loading/completion key currentKey must incorporate
orderId to avoid showing stale solver state when orderId changes; update the key
creation (currentKey) to include orderId (e.g.,
`${networkId}:${txHash}:${orderId || ''}`) so resolveSolverByTxHash results and
the setDoneFor marker are unique per order, keep the same nulling behavior when
any required value is missing, and ensure the useEffect dependency array still
includes networkId, txHash, currentKey, and orderId so the effect re-runs
correctly.
In `@apps/explorer/src/test/hooks/useTradeSolver.test.tsx`:
- Around line 74-134: Add a regression test to useTradeSolver that verifies a
mismatch between the winner solution's orderId and the requested orderId yields
undefined: arrange mockedGetSolverCompetitionByTxHash to resolve to a
SolverCompetitionResponse whose winning solution has a different orderId than
the one passed to useTradeSolver (use mockSolverCompetitionResponse or construct
a response with auctionId and solutions where solution.orderId !== the test
orderId), mock mockedFetchSolversInfo as before, renderHook(() =>
useTradeSolver(<txHash>, <orderId>Mismatch)), wait for loading to finish, and
assert result.current.solver is undefined; reference useTradeSolver,
mockedGetSolverCompetitionByTxHash, mockSolverCompetitionResponse, and
mockedFetchSolversInfo to locate where to add the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b99416ef-00b2-4c79-967c-d40038cb5d60
📒 Files selected for processing (5)
apps/explorer/src/components/orders/OrderDetails/FillsTableRow.tsxapps/explorer/src/hooks/orderSolverShared.tsapps/explorer/src/hooks/useTradeSolver.tsapps/explorer/src/test/hooks/useOrderSolver.test.tsxapps/explorer/src/test/hooks/useTradeSolver.test.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/explorer/src/test/hooks/useTradeSolver.test.tsx (1)
136-160: Add a multi-order winner case to harden inclusion logic.Current mismatch coverage is good, but it won’t catch regressions where code only checks the first order. Add a case where the requested order appears later in the winner’s
ordersarray and assert solver resolution succeeds.✅ Suggested test addition
+ it('resolves solver when requested orderId is present in winner orders (non-first position)', async () => { + const requestedOrderId = '0xrequestedOrder999' + + mockedGetSolverCompetitionByTxHash.mockResolvedValueOnce({ + auctionId: 1, + solutions: [ + { + solver: 'projectblanc', + isWinner: true, + solverAddress: '0xsolver', + ranking: 1, + orders: [{ id: '0xotherOrder111' }, { id: requestedOrderId }], + } as unknown as NonNullable<SolverCompetitionResponse['solutions']>[0], + ], + } as SolverCompetitionResponse) + mockedFetchSolversInfo.mockResolvedValueOnce(MOCK_SOLVERS) + + const { result } = renderHook(() => useTradeSolver('0xtx999', requestedOrderId)) + await waitFor(() => expect(result.current.isLoading).toBe(false)) + + expect(result.current.solver).toEqual({ + solverId: 'projectblanc', + displayName: 'Project Blanc', + image: 'https://example.com/blanc.png', + }) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/explorer/src/test/hooks/useTradeSolver.test.tsx` around lines 136 - 160, Add a new test in useTradeSolver.test.tsx that covers a multi-order winner: mock mockedGetSolverCompetitionByTxHash to return a solution where the winning solution (in the solutions array) has an orders array with multiple entries and the requestedOrderId appears not in position 0 but later (e.g., orders: [{ id: 'other1' }, { id: requestedOrderId }, ...]); call renderHook(() => useTradeSolver(txHash, requestedOrderId)), wait for isLoading false, then assert result.current.solver resolves to the expected solver object (not undefined) and mockedFetchSolversInfo was called; this ensures the matching logic in useTradeSolver iterates the orders array rather than only checking the first order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/explorer/src/test/hooks/useTradeSolver.test.tsx`:
- Around line 136-160: Add a new test in useTradeSolver.test.tsx that covers a
multi-order winner: mock mockedGetSolverCompetitionByTxHash to return a solution
where the winning solution (in the solutions array) has an orders array with
multiple entries and the requestedOrderId appears not in position 0 but later
(e.g., orders: [{ id: 'other1' }, { id: requestedOrderId }, ...]); call
renderHook(() => useTradeSolver(txHash, requestedOrderId)), wait for isLoading
false, then assert result.current.solver resolves to the expected solver object
(not undefined) and mockedFetchSolversInfo was called; this ensures the matching
logic in useTradeSolver iterates the orders array rather than only checking the
first order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d94b205a-8888-48e0-8952-a659325d8641
📒 Files selected for processing (2)
apps/explorer/src/hooks/useTradeSolver.tsapps/explorer/src/test/hooks/useTradeSolver.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/explorer/src/hooks/useTradeSolver.ts
limitofzero
approved these changes
Mar 31, 2026
fairlighteth
approved these changes
Apr 1, 2026
Contributor
fairlighteth
left a comment
fairlighteth
left a comment
There was a problem hiding this comment.
Looks good. One small nit:
One non-blocking test gap remains: apps/explorer/src/test/hooks/useTradeSolver.test.tsx:136 covers the mismatch case, but not the positive case where the requested order appears later in winner.orders[]. The code uses .find, so I don’t see a correctness issue, but that would harden the regression coverage.
elena-zh
approved these changes
Apr 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix bug reported by Bram in the solver winner selection.
On production, this order incorrectly showns Rizzolver as a winning solver for one of the fills
Since the introduction of combinatorial auctions, a single settlement can have more than one winning solution.
Thus, we also need to check for the inclusion of the order in a given winning solution:
To Test
0x03060706bc82e134fc9bf8c3941711cf50aa8364dbc3e8811cf5b60bd27d7047b1acdb57a218069f6c7308e2dc90828fbf486e0269ee46d1KipseliSummary by CodeRabbit