feat: add protocol fee calculation for orders #675
Conversation
WalkthroughThis pull request introduces protocol fee support throughout the quote calculation pipeline. A new Changes
Sequence DiagramsequenceDiagram
participant Quote as Quote Calc
participant GetOrder as getOrderToSign
participant GetQuote as getQuote
participant GetQAC as getQuoteAmountsAndCosts
participant ProtocolFee as getQuoteAmountsWithProtocolFee
rect rgb(200, 220, 255)
Note over Quote: Old Flow
Quote->>GetQAC: afterPartnerFees → slippage
end
rect rgb(220, 255, 220)
Note over Quote: New Flow
Quote->>GetOrder: protocolFeeBps
GetOrder->>GetQuote: protocolFeeBps
GetQuote->>GetQAC: protocolFeeBps
GetQAC->>ProtocolFee: networkCosts + protocolFeeBps
ProtocolFee->>ProtocolFee: Calculate protocolFeeAmount
ProtocolFee->>GetQAC: afterProtocolFees
GetQAC->>GetQAC: afterProtocolFees → slippage
GetQAC->>GetOrder: costs.protocolFee
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
|
All contributors have signed the CLA ✍️ ✅ |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📦 GitHub Packages PublishedLast updated: Nov 14, 2025, 03:25:20 PM UTC The following packages have been published to GitHub Packages with pre-release version
InstallationThese packages require authentication to install from GitHub Packages. First, create a # Create .npmrc file in your project root
echo "@cowprotocol:registry=https://npm.pkg.github.com" > .npmrc
echo "//npm.pkg.github.com/:_authToken=YOUR_GITHUB_TOKEN" >> .npmrcTo get your GitHub token:
Then install any of the packages above, either by exact version (i.e. # Yarn
yarn add npm:@cowprotocol/cow-sdk@pr-675
# pnpm
pnpm install npm:@cowprotocol/cow-sdk@pr-675
# NPM
npm install npm:@cowprotocol/cow-sdk@pr-675Update to the latest version (only if you used the tag)Every commit will publish a new package. To upgrade to the latest version, run: # Yarn
yarn upgrade @cowprotocol/cow-sdk
# pnpm
pnpm update @cowprotocol/cow-sdk
# NPM
npm update @cowprotocol/cow-sdkView PackagesYou can view the published packages at: https://github.com/cowprotocol/cow-sdk/packages |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/order-book/package.json(1 hunks)packages/order-book/src/quoteAmountsAndCostsUtils.ts(6 hunks)packages/order-book/src/types.ts(3 hunks)packages/trading/src/getOrderToSign.ts(2 hunks)packages/trading/src/getQuote.ts(1 hunks)packages/trading/src/resolveSlippageSuggestion.ts(1 hunks)packages/trading/src/utils/misc.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: sebastiantf
Repo: cowprotocol/cow-sdk PR: 317
File: src/bridging/providers/bungee/util.ts:0-0
Timestamp: 2025-05-30T13:00:36.623Z
Learning: The `calculateFeeBps` function in the Bungee bridge provider includes proper validation checks (division by zero, fee amount > input amount) and uses BigInt arithmetic with rounding for precision before converting to Number. This approach is safe because fee amounts are always less than input amounts, ensuring the resulting bps value stays under 10,000.
📚 Learning: 2025-05-30T13:00:36.623Z
Learnt from: sebastiantf
Repo: cowprotocol/cow-sdk PR: 317
File: src/bridging/providers/bungee/util.ts:0-0
Timestamp: 2025-05-30T13:00:36.623Z
Learning: The `calculateFeeBps` function in the Bungee bridge provider includes proper validation checks (division by zero, fee amount > input amount) and uses BigInt arithmetic with rounding for precision before converting to Number. This approach is safe because fee amounts are always less than input amounts, ensuring the resulting bps value stays under 10,000.
Applied to files:
packages/order-book/src/types.tspackages/trading/src/getQuote.tspackages/trading/src/getOrderToSign.tspackages/trading/src/resolveSlippageSuggestion.tspackages/order-book/src/quoteAmountsAndCostsUtils.ts
📚 Learning: 2025-05-13T12:22:48.422Z
Learnt from: anxolin
Repo: cowprotocol/cow-sdk PR: 306
File: src/trading/getQuote.ts:96-105
Timestamp: 2025-05-13T12:22:48.422Z
Learning: In the CoW SDK's trading functionality, app data must be built before getting a quote, even when using AUTO slippage. If AUTO slippage is used and the suggested slippage is higher than the default, the app data is recreated with the new slippage value, but the quote itself doesn't need to be refetched because slippage has no fundamental impact on the quote.
Applied to files:
packages/trading/src/resolveSlippageSuggestion.ts
🔇 Additional comments (11)
packages/trading/src/resolveSlippageSuggestion.ts (1)
39-39: LGTM - Protocol fee parameter correctly propagated.The conversion of
quote.protocolFeeBpsto Number is appropriate for BPS values, which are always small integers (typically 0-10000). The conditional ensuresundefinedis passed when the protocol fee is not present.packages/trading/src/getQuote.ts (2)
211-211: LGTM - Consistent protocol fee parameter handling.The protocol fee is correctly converted and passed to
getQuoteAmountsAndCosts, maintaining consistency with the pattern used throughout the codebase.
217-217: LGTM - Protocol fee propagated to order signing.The protocol fee is correctly included in the order-to-sign flow, ensuring it influences the final signed order data.
packages/trading/src/utils/misc.ts (1)
36-36: LGTM - Protocol fee correctly integrated into mapping utility.The
mapQuoteAmountsAndCostsfunction properly handles the newprotocolFeefield, maintaining consistency with the existing patterns fornetworkFeeandpartnerFee. TheafterProtocolFeesserialization is correctly added.Also applies to: 59-62, 67-67
packages/order-book/src/types.ts (2)
24-27: LGTM - Protocol fee type definition matches existing patterns.The
protocolFeefield structure is consistent with the existingpartnerFeeandnetworkFeepatterns.
39-39: LGTM - Documentation and type definition correctly updated.The documentation accurately describes the protocol fee application order, and the
afterProtocolFeesfield provides visibility into amounts after protocol fees but before slippage.Also applies to: 79-84
packages/trading/src/getOrderToSign.ts (1)
21-21: LGTM - Protocol fee parameter correctly threaded through order signing.The
protocolFeeBpsparameter is properly typed, destructured, and passed togetQuoteAmountsAndCosts, ensuring protocol fees are considered when calculating the order to sign.Also applies to: 25-25, 67-67
packages/order-book/src/quoteAmountsAndCostsUtils.ts (4)
130-147: LGTM - Early return for zero/negative protocol fees.The edge case handling for non-positive protocol fees is correct, returning zero fee amount and unchanged amounts.
151-167: LGTM - SELL order protocol fee calculation appears correct.The formula correctly reconstructs the protocol fee from the buy amount. The comment accurately explains that the API has already deducted the protocol fee from the buy amount, and the reverse calculation extracts it:
protocolFeeInBuy = buyAmount * feeBps / (1 - feeBps).
247-252: Correct: Slippage now calculated on afterProtocolFees.This change correctly implements the intended fee application order: network fees → partner fees → protocol fees → slippage. Previously, slippage was applied to
afterPartnerFees, but now it correctly usesafterProtocolFees.
215-215: LGTM - Protocol fee properly integrated into the calculation pipeline.The protocol fee parameter is correctly read with a default value, computed via the helper function, included in the costs structure, and exposed in the return value. The integration follows the established patterns in the codebase.
Also applies to: 238-245, 265-268, 279-279
| "test:coverage": "jest --coverage --json --outputFile=jest.results.json && npx coveralls < ./coverage/lcov.info", | ||
| "test:coverage:html": "jest --silent=false --coverage --coverageReporters html", | ||
| "swagger:codegen": "openapi --input https://raw.githubusercontent.com/cowprotocol/services/v2.291.0/crates/orderbook/openapi.yml --output src/generated --exportServices false --exportCore false", | ||
| "swagger:codegen": "openapi --input https://raw.githubusercontent.com/cowprotocol/services/dfb50cb4a103e8f949f5a7145beb6be63ef41c85/crates/orderbook/openapi.yml --output src/generated --exportServices false --exportCore false", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify if this commit hash corresponds to a released version.
Using a commit hash instead of a version tag makes it harder to track which API version is in use. Before merging, ensure this commit is part of a stable release and consider using a version tag instead.
🏁 Script executed:
#!/bin/bash
# Check if the commit is tagged with a version
cd /tmp
git clone --filter=blob:none --no-checkout https://github.com/cowprotocol/services.git 2>/dev/null || true
cd services
git fetch origin dfb50cb4a103e8f949f5a7145beb6be63ef41c85 2>/dev/null
git tag --contains dfb50cb4a103e8f949f5a7145beb6be63ef41c85 2>/dev/null | head -5Length of output: 0
Let me try alternative verification approaches to check if this commit is tagged or part of a release:
---
🏁 Script executed:
```shell
#!/bin/bash
# Try using GitHub API to check the commit and its tags
COMMIT="dfb50cb4a103e8f949f5a7145beb6be63ef41c85"
REPO="cowprotocol/services"
# Check if commit exists
echo "Checking if commit exists..."
curl -s "https://api.github.com/repos/${REPO}/commits/${COMMIT}" | jq -r '.sha // empty' | head -1
# Try to find tags associated with this commit
echo "Searching for associated tags..."
curl -s "https://api.github.com/repos/${REPO}/git/refs/tags" | jq -r ".[] | select(.object.sha == \"${COMMIT}\") | .ref" 2>/dev/null | head -5
Length of output: 102
---
🏁 Script executed:
```shell
#!/bin/bash
COMMIT="dfb50cb4a103e8f949f5a7145beb6be63ef41c85"
REPO="cowprotocol/services"
# Get commit details
echo "=== Commit Details ==="
curl -s "https://api.github.com/repos/${REPO}/commits/${COMMIT}" | jq -r '.commit.message, .commit.author.date, .commit.committer.date' 2>/dev/null
# Get v2.291.0 tag info
echo ""
echo "=== v2.291.0 Tag Details ==="
curl -s "https://api.github.com/repos/${REPO}/git/refs/tags/v2.291.0" | jq -r '.object' 2>/dev/null
# Get the commit date for v2.291.0
echo ""
echo "=== v2.291.0 Commit Date ==="
curl -s "https://api.github.com/repos/${REPO}/tags" | jq -r '.[] | select(.name == "v2.291.0") | .commit.url' 2>/dev/null | head -1 | xargs -I {} curl -s {} | jq -r '.commit.author.date' 2>/dev/null
Length of output: 362
Do not merge with this unreleased commit hash. Use a stable version tag instead.
The commit dfb50cb4a103e8f949f5a7145beb6be63ef41c85 is a recent feature branch merge from 2025-11-12, not a released version. It's not tagged and differs from the previous stable version v2.291.0. Using an unreleased commit introduces maintenance risk and makes dependency tracking difficult. Before merging, switch to a stable version tag (e.g., v2.291.0 or a newer release if available).
🤖 Prompt for AI Agents
In packages/order-book/package.json around line 23, the swagger:codegen script
references an unreleased commit hash (dfb50cb4a103e8f949f5a7145beb6be63ef41c85)
in the OpenAPI input URL; replace that commit hash with a stable release tag
(for example v2.291.0 or a newer official release) so the codegen points to a
released version, then update the repository (or CI) to regenerate the
src/generated output with the chosen tag and run tests/linters to ensure no
regressions.
| } else { | ||
| /** | ||
| * todo This is not implemented properly yet for buyAmount/sellAmount calculations | ||
| */ | ||
| const ONE_PLUS_PROTOCOL_FEE_BPS = ONE_HUNDRED_BPS + protocolFeeBpsBig | ||
| const sellAmountWithNetworkFee = sellAmountAfterNetworkCosts.big + networkCostAmount.big | ||
| const protocolFeeAmountInSell = (sellAmountWithNetworkFee * protocolFeeBpsBig) / ONE_PLUS_PROTOCOL_FEE_BPS | ||
|
|
||
| return { | ||
| protocolFeeAmount: protocolFeeAmountInSell, | ||
| afterProtocolFees: { | ||
| sellAmount: sellAmountAfterNetworkCosts.big, // todo | ||
| buyAmount: buyAmountAfterNetworkCosts.big, // todo | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
BUY order protocol fee calculation is incomplete.
The TODO comments on lines 170-171, 179, and 180 indicate that the BUY order protocol fee calculation is not properly implemented yet. Additionally, the formula and amount calculations need verification:
-
Line 173-174: The calculation uses
sellAmountAfterNetworkCosts + networkCostAmount, which appears to reconstruct the original sell amount before network costs. This needs verification against the intended protocol fee application logic for BUY orders. -
Lines 179-180: The
afterProtocolFeesamounts are marked astodoand simply return the input amounts unchanged, meaning protocol fees are not actually being applied to BUY orders.
Since this is a DRAFT PR, this is expected, but these issues must be resolved before merging.
Would you like me to help design the correct formula for BUY order protocol fee calculation? The approach should mirror the SELL order logic but account for the different fee application direction.
|
I have read the CLA Document and I hereby sign the CLA |
limitofzero
changed the title
Summary by CodeRabbit
Release Notes