feat(trading): use suggested slippage from BFF #544
Conversation
|
Warning Rate limit exceeded@shoom3301 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds BFF endpoint mappings and a CoWBFFClient; implements an API-backed slippage suggestion (suggestSlippageBpsWithApi) that prefers BFF-provided slippage and falls back to local calculation; updates getQuote to use the API-backed path and adds unit tests and a publish workflow step. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Trader
participant Quote as getQuote/getQuoteRaw
participant Slippage as suggestSlippageBpsWithApi
participant BFF as CoWBFFClient
participant Local as suggestSlippageBps
Trader->>Quote: requestQuote(params, maybe bffEnv)
Quote->>Slippage: suggestSlippageBpsWithApi(params, bffEnv?, timeout?)
Slippage->>BFF: getSlippageTolerance(chainId, sell-buy, timeout)
alt API returns valid numeric slippageBps within bounds
BFF-->>Slippage: { slippageBps }
Slippage-->>Quote: slippageBps (API)
else API null/error/invalid/out-of-bounds
BFF-->>Slippage: null / error / invalid
Slippage->>Local: suggestSlippageBps(params)
Local-->>Slippage: slippageBps (fallback)
Slippage-->>Quote: slippageBps (fallback)
end
Quote-->>Trader: quote including suggestedSlippageBps
sequenceDiagram
participant Env as CowEnv
participant Client as CoWBFFClient
note over Env,Client: baseUrl = BFF_ENDPOINTS[env] (prod/staging)
Env-->>Client: env ('prod' default or 'staging')
Client->>Client: set baseUrl
Client->>BFF: GET {baseUrl}/{chainId}/markets/{sell}-{buy}/slippageTolerance (with signal & headers)
BFF-->>Client: HTTP response -> parse JSON -> validate slippageBps
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (14)
packages/trading/src/getQuote.ts (2)
127-136: Avoid awaiting slippage API when slippageBps is explicitly provided to reduce latency.Currently we wait for the API even if caller supplied slippage, increasing p95 unnecessarily. Call API only when slippageBps is undefined.
- const suggestedSlippageBps = await suggestSlippageBpsWithApi({ - isEthFlow, - quote, - tradeParameters, - trader, - advancedSettings, - bffEnv: env, - }) + const suggestedSlippageBps = + slippageBps === undefined + ? await suggestSlippageBpsWithApi({ + isEthFlow, + quote, + tradeParameters, + trader, + advancedSettings, + bffEnv: env, + }) + : slippageBps
67-76: Optional: expose API timeout in advanced settings.Consider threading a slippageApiTimeoutMs from SwapAdvancedSettings into suggestSlippageBpsWithApi to tune UX per integration.
packages/trading/src/cowBffClient.test.ts (2)
3-6: Restore global.fetch after tests to avoid cross-suite pollution.Direct assignment persists beyond this file. Capture original and restore in afterAll.
-// Mock fetch globally -const mockFetch = jest.fn() -global.fetch = mockFetch +// Mock fetch globally with proper restore +const originalFetch = global.fetch +const mockFetch = jest.fn() as any +beforeAll(() => { + global.fetch = mockFetch as any +}) +afterAll(() => { + global.fetch = originalFetch as any +})
34-37: Gitleaks false positive: ERC20 addresses flagged as “Generic API Key”.These are public token addresses, not secrets. Add a repo-level allowlist for this test file or the exact addresses to silence noise.
packages/trading/src/getQuote.test.ts (2)
399-432: Rename test to reflect actual source of bffEnv.The env is passed via tradeParameters, not advancedSettings. Rename for clarity.
- it('should pass bffEnv from advancedSettings to suggestSlippageBpsWithApi', async () => { + it('should pass bffEnv from tradeParameters to suggestSlippageBpsWithApi', async () => {
241-263: Optional: add a test to ensure we don’t call the slippage API when slippageBps is provided.If you adopt the getQuote.ts optimization, assert we skip the API call when slippage is user-specified.
+ it('getQuoteRaw should not call slippage API when slippageBps is provided', async () => { + mockSuggestSlippageBpsWithApi.mockReset() + setGlobalAdapter(adapters[Object.keys(adapters)[0] as keyof typeof adapters]) + await getQuoteRaw( + { ...defaultOrderParams, slippageBps: 75 }, // explicit slippage + { chainId: SupportedChainId.GNOSIS_CHAIN, appCode: '0x007', account: '0xabc' as const }, + undefined, + orderBookApiMock + ) + expect(mockSuggestSlippageBpsWithApi).not.toHaveBeenCalled() + })packages/trading/src/suggestSlippageBpsWithApi.ts (1)
39-47: Harden validation: check for finite numbers.Guard against NaN/Infinity before range checks.
- if (apiResponse && typeof apiResponse.slippageBps === 'number') { + if ( + apiResponse && + typeof apiResponse.slippageBps === 'number' && + Number.isFinite(apiResponse.slippageBps) + ) { const apiSlippageBps = apiResponse.slippageBpspackages/trading/src/suggestSlippageBpsWithApi.test.ts (3)
217-223: Prefer objectContaining for call arg assertionsFuture additions to the arg shape will break exact-match assertions. Tolerate extra fields:
-expect(mockApiClient.getSlippageTolerance).toHaveBeenCalledWith({ +expect(mockApiClient.getSlippageTolerance).toHaveBeenCalledWith(expect.objectContaining({ sellToken: '0x6b175474e89094c44da98b954eedeac495271d0f', buyToken: '0x2260fac5e5542a773aa44fbcfedf7c193bc2c599', chainId: SupportedChainId.MAINNET, timeoutMs: 5000, -}) +}))
121-131: Make “too low” case future‑proofThe comment hardcodes “default minimum of 50”. To avoid coupling tests to a moving target, use an obviously invalid value (e.g., negative) so the test intent remains stable:
- getSlippageTolerance: jest.fn().mockResolvedValue({ slippageBps: 10 }), // Below default minimum of 50 + getSlippageTolerance: jest.fn().mockResolvedValue({ slippageBps: -1 }), // Always below any sensible minimum
167-177: Add coverage for non‑finite/decimal slippageConsider adding cases for
NaN,Infinity, and decimal values to ensure wrapper validation rejects them:
{ slippageBps: NaN }→ fallback{ slippageBps: Infinity }→ fallback{ slippageBps: 12.5 }→ fallback (if we require integer BPS)I’d pair this with a stricter check in the client (see separate comment).
packages/trading/src/cowBffClient.ts (4)
5-6: Export and reuse a single timeout constantExport the default so callers/tests don’t duplicate magic numbers and the wrapper can forward an explicit default.
-const DEFAULT_TIMEOUT = 2000 // 2 sec +export const DEFAULT_SLIPPAGE_TIMEOUT_MS = 2000 // 2 sec ... - timeoutMs = DEFAULT_TIMEOUT, + timeoutMs = DEFAULT_SLIPPAGE_TIMEOUT_MS,Also applies to: 21-23, 37-38
47-55: Drop Content‑Type on GET and keep headers minimal
Content-Typeon a GET without a body is unnecessary. Keep justAccept.const response = await fetch(url, { method: 'GET', headers: { - Accept: 'application/json', - 'Content-Type': 'application/json', + Accept: 'application/json', }, signal: controller.signal, })
40-43: Harden URL construction and token formattingSafer path building and token normalization reduce 404 risk on case‑sensitive routes:
- const url = `${this.baseUrl}/${chainId}/markets/${sellToken}-${buyToken}/slippageTolerance` + const sell = encodeURIComponent(sellToken.toLowerCase()) + const buy = encodeURIComponent(buyToken.toLowerCase()) + const url = `${this.baseUrl}/${chainId}/markets/${sell}-${buy}/slippageTolerance`
65-73: Tighten response validation (finite integer BPS)Avoid accepting
NaN,Infinity, or decimals:- if (typeof data !== 'object' || data === null || typeof data.slippageBps !== 'number') { + const okShape = typeof data === 'object' && data !== null + const bps = okShape ? (data as any).slippageBps : undefined + if (!Number.isFinite(bps) || !Number.isInteger(bps)) { log('Invalid slippage tolerance API response structure') return null } - log(`Retrieved slippage tolerance from API: ${data.slippageBps} BPS`) - return { slippageBps: data.slippageBps } + log(`Retrieved slippage tolerance from API: ${bps} BPS`) + return { slippageBps: bps }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/trading/src/consts.ts(1 hunks)packages/trading/src/cowBffClient.test.ts(1 hunks)packages/trading/src/cowBffClient.ts(1 hunks)packages/trading/src/getQuote.test.ts(3 hunks)packages/trading/src/getQuote.ts(2 hunks)packages/trading/src/suggestSlippageBpsWithApi.test.ts(1 hunks)packages/trading/src/suggestSlippageBpsWithApi.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-10T02:54:14.585Z
Learnt from: jeffersonBastos
PR: cowprotocol/cow-sdk#327
File: packages/app-data/src/api/fetchDocFromCid.spec.ts:1-3
Timestamp: 2025-06-10T02:54:14.585Z
Learning: In the app-data package, fetchMock is configured globally in setupTests.cjs (`global.fetchMock = fetchMock`), so test files don't need to import fetchMock explicitly - it's available globally in all tests.
Applied to files:
packages/trading/src/cowBffClient.test.ts
🧬 Code graph analysis (7)
packages/trading/src/getQuote.ts (1)
packages/trading/src/suggestSlippageBpsWithApi.ts (1)
suggestSlippageBpsWithApi(25-63)
packages/trading/src/cowBffClient.test.ts (1)
packages/trading/src/cowBffClient.ts (1)
CoWBFFClient(18-78)
packages/trading/src/suggestSlippageBpsWithApi.test.ts (2)
packages/trading/src/cowBffClient.ts (1)
CoWBFFClient(18-78)packages/trading/src/suggestSlippageBpsWithApi.ts (1)
suggestSlippageBpsWithApi(25-63)
packages/trading/src/consts.ts (1)
packages/config/src/types/configs.ts (1)
CowEnv(35-35)
packages/trading/src/suggestSlippageBpsWithApi.ts (5)
packages/trading/src/suggestSlippageBps.ts (1)
SuggestSlippageBps(13-19)packages/config/src/types/configs.ts (1)
CowEnv(35-35)packages/trading/src/cowBffClient.ts (1)
CoWBFFClient(18-78)packages/trading/src/utils/slippage.ts (1)
getDefaultSlippageBps(57-63)packages/common/src/utils/log.ts (1)
log(3-6)
packages/trading/src/cowBffClient.ts (4)
packages/config/src/types/configs.ts (1)
CowEnv(35-35)packages/trading/src/consts.ts (1)
BFF_ENDPOINTS(4-7)packages/common/src/utils/log.ts (1)
log(3-6)packages/order-book/src/api.ts (1)
fetch(435-444)
packages/trading/src/getQuote.test.ts (4)
packages/trading/src/suggestSlippageBpsWithApi.ts (1)
suggestSlippageBpsWithApi(25-63)packages/common/src/adapters/context.ts (1)
setGlobalAdapter(37-40)packages/trading/src/getQuote.ts (1)
getQuoteRaw(56-184)packages/config/src/chains/const/contracts.ts (1)
ETH_ADDRESS(4-4)
🪛 Gitleaks (8.28.0)
packages/trading/src/cowBffClient.test.ts
[high] 34-34: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 35-35: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Publish to GitHub Packages
- GitHub Check: Build Package
- GitHub Check: test
- GitHub Check: eslint
🔇 Additional comments (1)
packages/trading/src/consts.ts (1)
4-7: No action required—env is correctly typed
TradeParametersinheritsenv?: CowEnvviaTradeOptionalParameters, so the endpoints mapping covers all valid cases.
📦 GitHub Packages PublishedLast updated: Sep 29, 2025, 12:45:54 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, configure your token: # Set your GitHub token
# 1. create one at https://github.com/settings/tokens. Make sure you select the option "Generate new token (classic)"
# 2. Check only the "read:packages" scope
# 3. Add the token to the npm config
npm config set //npm.pkg.github.com/:_authToken YOUR_GITHUB_TOKENThen install any of the packages above: # Yarn
yarn add @cowprotocol/[email protected] --registry https://npm.pkg.github.com
# pnpm
pnpm install @cowprotocol/[email protected] --registry https://npm.pkg.github.com
# NPM
npm install @cowprotocol/[email protected] --registry https://npm.pkg.github.comView PackagesYou can view the published packages at: https://github.com/cowprotocol/cow-sdk/packages |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/trading/src/getQuote.ts (1)
137-141: FAST gating looks good; consider skipping network when user sets slippage.The branch correctly uses local calc for FAST and API otherwise. Optional: if
slippageBpsis provided, skip the API call to save latency and avoid unnecessary external dependency.Example tweak:
- const suggestedSlippageBps = - quoteRequest.priceQuality === PriceQuality.FAST - ? suggestSlippageBps(suggestSlippageParams) - : await suggestSlippageBpsWithApi(suggestSlippageParams) + const suggestedSlippageBps = + slippageBps !== undefined + ? suggestSlippageBps(suggestSlippageParams) + : (quoteRequest.priceQuality === PriceQuality.FAST + ? suggestSlippageBps(suggestSlippageParams) + : await suggestSlippageBpsWithApi(suggestSlippageParams))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/trading/src/getQuote.test.ts(3 hunks)packages/trading/src/getQuote.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: anxolin
PR: cowprotocol/cow-sdk#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.
📚 Learning: 2025-05-13T12:22:48.422Z
Learnt from: anxolin
PR: cowprotocol/cow-sdk#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/getQuote.ts
🧬 Code graph analysis (2)
packages/trading/src/getQuote.ts (2)
packages/trading/src/suggestSlippageBpsWithApi.ts (2)
SuggestSlippageBpsWithApi(9-12)suggestSlippageBpsWithApi(25-63)packages/trading/src/suggestSlippageBps.ts (1)
suggestSlippageBps(24-67)
packages/trading/src/getQuote.test.ts (4)
packages/trading/src/suggestSlippageBpsWithApi.ts (1)
suggestSlippageBpsWithApi(25-63)packages/common/src/adapters/context.ts (1)
setGlobalAdapter(37-40)packages/trading/src/getQuote.ts (1)
getQuoteRaw(57-190)packages/config/src/chains/const/contracts.ts (1)
ETH_ADDRESS(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: Publish to GitHub Packages
- GitHub Check: eslint
🔇 Additional comments (3)
packages/trading/src/getQuote.ts (2)
26-31: Imports wired correctly for API-backed and fallback slippage paths.Good separation of concerns: API path via
suggestSlippageBpsWithApiand local fallback viasuggestSlippageBps. No issues here.
128-135: Pass 2 000 ms timeout into suggestSlippageBpsWithApi
The 2 s timeout isn’t forwarded here or set by default inCoWBFFClient/suggestSlippageBpsWithApi; explicitly pass the 2000 ms timeout parameter.packages/trading/src/getQuote.test.ts (1)
473-512: Confirm intended scope of FAST override in tests.This validates the FAST bypass using
advancedSettings.quoteRequest.priceQuality = FAST. Elsewhere, tests assert priceQuality must be OPTIMAL. Please confirm that public entry points won’t set FAST inadvertently and that this override is limited to unit tests or explicit power‑user flows.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.test.ts (1)
292-368: Always restore fake timers, even on failures
jest.useRealTimers()sits at the bottom of the test. If anything throws before that line (assertion, rejection, etc.), Jest stays in fake-timer mode and the pending timers from the slow providers leak into later specs, causing flaky follow-up tests. Wrap the fake-timer window intry/finallyso cleanup always runs (and flush/clear pending timers before switching back). (testing-library.com)- jest.useFakeTimers() - - const resultPromise = strategy.execute(request, config) - - // Advance timers past the fast provider delay but before slow providers - jest.advanceTimersByTime(50) - - const result = await resultPromise - - // Should still return the quote from the fast provider - expect(result).toBeTruthy() - expect(result?.providerDappId).toBe('mockProvider') - - jest.useRealTimers() + jest.useFakeTimers() + try { + const resultPromise = strategy.execute(request, config) + + // Advance timers past the fast provider delay but before slow providers + jest.advanceTimersByTime(50) + + const result = await resultPromise + + // Should still return the quote from the fast provider + expect(result).toBeTruthy() + expect(result?.providerDappId).toBe('mockProvider') + } finally { + jest.runOnlyPendingTimers() + jest.useRealTimers() + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/publish-github-packages.yml(1 hunks)packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.test.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: anxolin
PR: cowprotocol/cow-sdk#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.
🧬 Code graph analysis (1)
packages/bridging/src/BridgingSdk/strategies/BestQuoteStrategy.test.ts (1)
packages/bridging/src/BridgingSdk/mock/bridgeRequestMocks.ts (1)
bridgeQuoteResult(127-161)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: eslint
- GitHub Check: Publish to GitHub Packages
- GitHub Check: Build Package
Uses API from: cowprotocol/bff#97
API example: https://bff.barn.cow.fi/1/markets/0x6b175474e89094c44da98b954eedeac495271d0f-0x2260fac5e5542a773aa44fbcfedf7c193bc2c599/slippageTolerance
Before this PR, we were using simplified, hardcoded function to suggest slippage:
https://github.com/cowprotocol/cow-sdk/blob/main/packages/trading/src/suggestSlippageBps.ts
In this PR I added
/slippageToleranceAPI intoTrading SDK.The API specification: https://www.notion.so/cownation/Speed-Finish-slippage-based-on-market-volatility-2168da5f04ca80e3987bf42b9d6c8a22?source=copy_link
The API is enabled by default and will be used as slippage value when
slippageBpsis not specified in parameters (same logic as before).The
/slippageTolerancecall has 2 sec timeout and will fallback to existingsuggestSlippageBps.tslogic if any network or other errors.Summary by CodeRabbit
New Features
Tests
Chores