feat(explorer): show network costs and protocol fee breakdown on order details#7588
Open
jmg-duarte wants to merge 17 commits into
Open
feat(explorer): show network costs and protocol fee breakdown on order details#7588jmg-duarte wants to merge 17 commits into
jmg-duarte wants to merge 17 commits into
Conversation
…r details Derive the breakdown client-side from `trades[].executedProtocolFees`: sum protocol fees per token, then split `order.totalFee` into network costs vs. protocol fees — subtracting only when both are denominated in the same token, since the protocol fee comes from the surplus token while `totalFee` lives in `executedFeeToken`.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduce protocol-fee types and extraction (getProtocolFees), add unit tests, render per-fee protocol-fee rows in GasFeeDisplay (with token resolution), add NumbersBreakdown UI, enrich orders in OrderDetails, adjust a ShowMoreButton usage, and bump many ChangesProtocol fee extraction & display
Sequence DiagramsequenceDiagram
participant OrderDetails
participant getProtocolFees
participant GasFeeDisplay
participant useMultipleErc20
OrderDetails->>getProtocolFees: enrichOrderFromTrades(trades)
getProtocolFees->>OrderDetails: protocolFees[]
OrderDetails->>GasFeeDisplay: render(order with protocolFees)
GasFeeDisplay->>useMultipleErc20: request token metadata for fee tokenAddresses
useMultipleErc20->>GasFeeDisplay: token metadata map
GasFeeDisplay->>GasFeeDisplay: resolveToken & formatFee -> render rows
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Deploying explorer-dev with
|
| Latest commit: |
2e8419a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9c0c9bc8.explorer-dev-dxz.pages.dev |
| Branch Preview URL: | https://jmgd-ucp.explorer-dev-dxz.pages.dev |
Deploying swap-dev with
|
| Latest commit: |
2e8419a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://dfc74df0.swap-dev-5u6.pages.dev |
| Branch Preview URL: | https://jmgd-ucp.swap-dev-5u6.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/explorer/src/test/utils/operator/orderFees.test.ts (1)
81-89: ⚡ Quick winAdd a mixed-checksum aggregation regression test.
Current coverage checks case-insensitive comparison, but not aggregation across multiple trades where the same token appears in different casing. Add one test with two trades (
FEE_TOKENandFEE_TOKEN.toLowerCase()) and assert single-token aggregation without warning.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/explorer/src/test/utils/operator/orderFees.test.ts` around lines 81 - 89, Add a regression test in orderFees.test.ts that verifies aggregation across mixed-checksum token strings: create two trades using makeTrade — one with token FEE_TOKEN and one with FEE_TOKEN.toLowerCase() — pass them to getFees alongside an order from makeOrder with executedFeeToken set to a different-cased FEE_TOKEN and totalFee large enough to split protocol/network fees, then assert the returned protocolFees equals the sum of both trade amounts (as a single token aggregation) and networkCosts equals the expected remainder; also assert no warning was emitted (e.g., result.warnings is empty or console.warn was not called) to ensure the same-token branch aggregates case-insensitively across multiple trades.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/explorer/src/api/operator/types.ts`:
- Around line 104-108: Change the protocolFeeTokenAddress property in the
OperatorTrade type from string to AddressKey (replace protocolFeeTokenAddress?:
string with protocolFeeTokenAddress?: AddressKey) and update all callers; then
replace any address comparisons that use toLowerCase() in the sameDenomination
logic with the address helpers — in apps/explorer/src/utils/operator.ts and
apps/explorer/src/components/orders/GasFeeDisplay/index.tsx use
areAddressesEqual(a,b) or normalize using getAddressKey(addr) before comparing
instead of addr.toLowerCase(), ensuring you import AddressKey, areAddressesEqual
and getAddressKey from the existing address utility module.
In `@apps/explorer/src/components/orders/GasFeeDisplay/index.tsx`:
- Around line 67-69: Replace direct .toLowerCase() comparisons with the cow-sdk
address helper: update the sameDenomination computation in GasFeeDisplay to call
areAddressesEqual(executedFeeToken, protocolFeeTokenAddress) instead of
comparing lowered strings; change formatFee and resolveToken signatures to
accept an AddressKey parameter type and inside each use
areAddressesEqual(tokenAddress, addressKey) (or similar) to determine equality
rather than calling toLowerCase() on addresses, and update any internal logic
that relied on string lowercasing to use the areAddressesEqual helper for all
address comparisons.
In `@apps/explorer/src/utils/operator.ts`:
- Around line 461-468: The fees grouping currently keys feesByToken by raw token
strings causing checksum-casing splits; change feesByToken to Map<AddressKey,
BigNumber> and when iterating executedProtocolFees use getAddressKey(token) as
the key (while still validating token exists) so amounts aggregate under a
normalized AddressKey (refer to feesByToken and the trades.forEach block). Also
replace the string lower-case comparison in sameDenomination with a call to
areAddressesEqual(tokenA, tokenB) per the address-comparison guideline to ensure
correct equality checks across checksum variants.
---
Nitpick comments:
In `@apps/explorer/src/test/utils/operator/orderFees.test.ts`:
- Around line 81-89: Add a regression test in orderFees.test.ts that verifies
aggregation across mixed-checksum token strings: create two trades using
makeTrade — one with token FEE_TOKEN and one with FEE_TOKEN.toLowerCase() — pass
them to getFees alongside an order from makeOrder with executedFeeToken set to a
different-cased FEE_TOKEN and totalFee large enough to split protocol/network
fees, then assert the returned protocolFees equals the sum of both trade amounts
(as a single token aggregation) and networkCosts equals the expected remainder;
also assert no warning was emitted (e.g., result.warnings is empty or
console.warn was not called) to ensure the same-token branch aggregates
case-insensitively across multiple trades.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 834ec6c8-a987-4d6c-a53b-f60aacd95af2
📒 Files selected for processing (9)
apps/explorer/src/api/operator/types.tsapps/explorer/src/components/AppDataRowContent/AppDataRowContent.tsxapps/explorer/src/components/common/ShowMoreButton.tsxapps/explorer/src/components/orders/GasFeeDisplay/GasFeeDisplay.stories.tsxapps/explorer/src/components/orders/GasFeeDisplay/index.tsxapps/explorer/src/components/orders/NumbersBreakdown/index.tsxapps/explorer/src/components/orders/OrderDetails/index.tsxapps/explorer/src/test/utils/operator/orderFees.test.tsapps/explorer/src/utils/operator.ts
Collaborator
|
@jmg-duarte can you take a look at the failing unittests? https://github.com/cowprotocol/cowswap/actions/runs/26631936508/job/78482889272?pr=7588 |
Collaborator
|
Additionally, something is weird. Checking a random order, I see this: 
The network costs token is not being decoded for some reason. In this case, the token is WETH. However, it's also an edge case as it's an ethflow order, so the sell token should actually be displayed as ETH. |
|
I was expecting a change towards only displaying protocol fees. We will not be able to reconstruct network fees. (I am assuming that |
fairlighteth
reviewed
Jun 2, 2026
fairlighteth
left a comment
fairlighteth
left a comment
Contributor
There was a problem hiding this comment.
⚠️ AI Review (Codex GPT-5, worked 4m): fee breakdown uses only the current fills page
Finding: Aggregate fee breakdown from all order trades, not the paginated fills table page
- Location:
apps/explorer/src/components/orders/OrderDetails/index.tsx:160 enrichOrderFromTradescallsgetFees(order, trades), buttradescomes fromuseOrderTrades(order, tableState.pageOffset, tableState.pageSize), which fetcheslimit + 1records and then slices back to the current page.- For a partially fillable order with more than 10 fills, the overview breakdown only sums the visible page. Page 1 omits later fills; after paging in the Fills tab, returning to Overview can show different fee/network-cost values for the same order.
- Impact: order-level costs become undercounted and page-dependent.
Suggested fix
- Use a complete trade set or dedicated aggregate for the order-level fee breakdown, or only render the breakdown when all trades needed for the aggregate have been loaded.
- Add coverage for an order with more fills than
RESULTS_PER_PAGEsoprotocolFeesincludes all fills and does not change withpageOffset.
Review scope and related context
This is separate from the existing review comments, which already cover:
- Address typing/comparison: resolved with
AddressKey,getAddressKey, andareAddressesEqual. - EthFlow WETH/ETH fee-token formatting: already reported by @alfetopito.
- Whether network fees should be displayed at all: already raised by @fhenneke.
- Mixed-checksum aggregation coverage: already suggested by CodeRabbit.
🤖 Prompt for AI agents
Verify this finding against current code. Fix only if still valid, keep the change minimal, and validate with targeted tests.
Context:
- apps/explorer/src/components/orders/OrderDetails/index.tsx:160 calls getFees(order, trades).
- apps/explorer/src/explorer/components/OrderWidget/index.tsx:36 passes useOrderTrades(order, tableState.pageOffset, tableState.pageSize).
- apps/explorer/src/hooks/useOperatorTrades.ts:69 fetches limit + 1 and lines 119-122 slice back to the current page.
- Fee breakdown should be order-level, not dependent on the currently selected fills page.
Surface each volume fee's policy factor as basis points (e.g. "Volume fee (2 bps)") so an order's protocol vs partner volume fees—identically labeled by the API, which doesn't expose fee owner—can be told apart. The factor is already in executedProtocolFees, just unused until now. Also skip non-positive fees so a policy that charges nothing doesn't add a "0" row, and stack the breakdown table under the show-more toggle (via DetailRow stack) instead of beside it, trimming the gap above it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/explorer/src/test/hooks/useOrderProtocolFees.test.tsx`:
- Around line 55-77: The test currently generates trades with fees "0".."24" but
getProtocolFees filters non-positive amounts, so update the trade creation in
the test (the Array.from call that calls createRawTradeWithFee) to start amounts
at 1 (e.g. use String(i + 1) instead of String(i)), and adjust the expected
mapped values in the assertion that checks protocolFees.map(...) to match the
new sequence (String(i + 1)); keep the rest of the paging/assertion logic
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 99909cd1-bd36-4558-be09-79e316969175
📒 Files selected for processing (10)
apps/explorer/src/api/operator/types.tsapps/explorer/src/components/orders/DetailsTable/items/CostAndFeesItem.tsxapps/explorer/src/components/orders/GasFeeDisplay/index.tsxapps/explorer/src/components/orders/NumbersBreakdown/index.tsxapps/explorer/src/components/orders/OrderDetails/index.tsxapps/explorer/src/explorer/components/OrderWidget/index.tsxapps/explorer/src/hooks/useOperatorTrades.tsapps/explorer/src/test/hooks/useOrderProtocolFees.test.tsxapps/explorer/src/test/utils/operator/orderFees.test.tsapps/explorer/src/utils/operator.ts
✅ Files skipped from review due to trivial changes (1)
- apps/explorer/src/components/orders/DetailsTable/items/CostAndFeesItem.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/explorer/src/utils/operator.ts
- apps/explorer/src/api/operator/types.ts
- apps/explorer/src/test/utils/operator/orderFees.test.ts
- apps/explorer/src/components/orders/NumbersBreakdown/index.tsx
elena-zh
requested changes
Jun 15, 2026
elena-zh
left a comment
elena-zh
left a comment
Contributor
There was a problem hiding this comment.
Hey @jmg-duarte , thank you, however, changes does not lgtm.
- Fees and costs total value does not match to the one displayed on CoW swap:
Please, compare it with the develop:
- It would be great to show a total amount at the top, then show a collapsed value with fees details. IMO, just 'show more' without a value looks weird.
- For an order with multiple fills it would be great to show a total of fees per category for all fills instead of showing an endless table like this one:
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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
Derive the breakdown client-side from
trades[].executedProtocolFees: sum protocol fees per token, then splitorder.totalFeeinto network costs vs. protocol fees — subtracting only when both are denominated in the same token, since the protocol fee comes from the surplus token whiletotalFeelives inexecutedFeeToken.Based on #5024
To Test
Summary by CodeRabbit
New Features
Improvements
Tests
Chores