feat: improve smart slippage #97
Conversation
…e-smart-slippage # Conflicts: # libs/repositories/src/gen/cow/cow-api-types.ts # libs/services/src/SlippageService/SlippageService.ts # libs/services/src/SlippageService/SlippageServiceMain.spec.ts # libs/services/src/SlippageService/SlippageServiceMain.test.ts # libs/services/src/SlippageService/SlippageServiceMain.ts # libs/services/src/SlippageService/SlippageServiceMock.ts
WalkthroughRefactors slippage calculation to prefer pair-based volatility (adds PairVolatility/getVolatilityForPair), updates SlippageServiceMain internals and tests, removes SlippageServiceMock and its re-export, marks several repository integration test suites as skipped, and adjusts many SlippageService test-data slippageBps values. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant S as SlippageServiceMain
participant U as USDPriceRepo
Client->>S: getSlippageBps(chainId, base, quote, ...)
rect rgba(220,235,255,0.35)
note over S: Try pair-based volatility first
S->>U: getPricePoints(chainId, base)
U-->>S: base price points
S->>U: getPricePoints(chainId, quote)
U-->>S: quote price points
S->>S: align timestamps → calculate pair volatility
end
alt Pair volatility available
S-->>Client: slippage from pair volatility
else Fallback
rect rgba(240,240,240,0.45)
note over S: Compute max-token volatility (legacy path)
S->>U: getPricePoints(chainId, token(s))
U-->>S: price points
S->>S: calculate max volatility → derive slippage
end
S-->>Client: slippage from max volatility or 0
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)libs/services/src/SlippageService/SlippageServiceMain.spec.ts (3)
⏰ 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). (1)
🔇 Additional comments (21)
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/services/src/SlippageService/test-data/1-DAI-ETH.json (1)
8-15: Swap volatilityDetails tokens to match top-level pairing
In libs/services/src/SlippageService/test-data/1-DAI-ETH.json, set volatilityDetails.baseToken.tokenAddress to 0x6B175… (DAI) and volatilityDetails.quoteToken.tokenAddress to 0xC02a… (WETH); run the repo check script to update all test-data occurrences (e.g. lines 1456–1463).
🧹 Nitpick comments (10)
libs/repositories/src/repos/SimulationRepository/SimulationrepositoryTenderly.test.ts (1)
35-36: Don’t permanently skip the suite—gate it on the required ENV variables.With
describe.skipthese integration tests never run, even when the Tenderly env vars are present (e.g. in a developer’s shell or a dedicated CI job). Prefer selectingdescribevsdescribe.skipdynamically so the suite still executes whenever the configuration is available.Apply something along these lines:
-// The tests are integration tests and require ENV variables -describe.skip('SimulationRepositoryTenderly', () => { +const describeIfTenderlyEnv = + TENDERLY_API_KEY && TENDERLY_ORG_NAME && TENDERLY_PROJECT_NAME + ? describe + : describe.skip; + +// Integration tests – require Tenderly env vars to run +describeIfTenderlyEnv('SimulationRepositoryTenderly', () => {That keeps the suite runnable without manual edits while ensuring default CI (without creds) still skips it. If you keep the
expect(...).toBeDefined()checks, they’ll continue to guard execution when the suite is enabled.libs/repositories/src/repos/TokenHolderRepository/TokenHolderRepositoryEthplorer.test.ts (1)
7-8: Skip conditionally instead of disabling the suite outright.
describe.skipprevents this integration suite from ever running—even in environments whereETHPLORER_API_KEYis configured. Please guard the suite with a conditionaldescribe/describe.skipselection so it self-enables when the API key is present.For example:
-// The tests are integration tests and require ENV variables -describe.skip('TokenHolderRepositoryEthplorer', () => { +const describeIfEthplorerEnv = ETHPLORER_API_KEY ? describe : describe.skip; + +// Integration tests – require ETHPLORER env vars to run +describeIfEthplorerEnv('TokenHolderRepositoryEthplorer', () => {This preserves the safety in default CI while keeping the tests runnable without editing the source when creds are supplied.
libs/repositories/src/repos/TokenHolderRepository/TokenHolderRepositoryMoralis.spec.ts (1)
7-8: Use an env-gateddescribeinstead of harddescribe.skip.As written, the Moralis suite is permanently disabled; even a developer with
MORALIS_API_KEYset can’t run it without modifying the file. Mirror the conditional pattern so the suite runs automatically whenever the key is available.Proposed tweak:
-// The tests are integration tests and require ENV variables -describe.skip('TokenHolderRepositoryMoralis', () => { +const describeIfMoralisEnv = MORALIS_API_KEY ? describe : describe.skip; + +// Integration tests – require MORALIS env vars to run +describeIfMoralisEnv('TokenHolderRepositoryMoralis', () => {That keeps CI green by default but restores the ability to execute the tests when the environment is ready.
libs/services/src/SlippageService/test-data/42161-USDC-DAI.json (1)
7-15: Optional: trim/derive volatility payload to keep fixtures lightThese fixtures embed large price series. Consider:
- Storing only derived stats (mean, stddev, window) used by SlippageService and omit raw series.
- Or limit to the exact window consumed by the code.
This reduces repo bloat and speeds tests.Also applies to: 1452-1455, 2905-2908
libs/services/src/SlippageService/SlippageServiceMain.spec.ts (3)
41-41: Rename misleading describe title.This block contains tests that return non‑zero values (e.g., 87, 1_118_034). Rename to avoid confusion.
Apply:
-describe('should return the 0 slippage if', () => { +describe('should return the expected slippage if', () => {
260-272: Fix math/comments to match expectations.The comment computes slippage as 112 bps, but the test expects 2 bps (which is correct for base=100, quote=1). Update comments to reflect division by 100 leading to ~0.0001118 → 2 bps.
274-286: Test titles don’t match setups (token worth ‘more/less’).For “worth more in USD” you set base=1.1, quote=100 (base is worth less). Rename titles or swap mock prices to reflect the intended scenario.
Example rename:
-it(`if token is worth more in USD, the slippage is smaller`, async () => { +it(`if the quote is worth much more in USD, the base-denominated slippage is larger`, async () => {And
-it(`if token is worth less in USD, the slippage is bigger`, async () => { +it(`if the base is worth less in USD, the slippage is bigger`, async () => {Also applies to: 306-318
libs/services/src/SlippageService/SlippageServiceMain.ts (3)
129-136: Guard against zero/negative USD prices to avoid division by zero.When either USD price is 0 or negative,
relativePricebecomes invalid and can yield Infinity/NaN.Apply:
- if (baseUsdPrice === null || quoteUsdPrice === null) { + if ( + baseUsdPrice === null || + quoteUsdPrice === null || + baseUsdPrice <= 0 || + quoteUsdPrice <= 0 + ) { return null; }
219-261: Volatility estimator: clarify population vs. sample variance and consistent scaling.You use population variance (divide by N). If you intended sample stddev (common for finite windows), use N-1. Either is fine, but add a brief note to avoid future “fixes.”
Add a one-line comment explaining the choice (population variance) to prevent accidental changes.
264-269: Rounding strategy note.Rounding to nearest 5m can produce cross-bucket matches if sources are phase-shifted. If you observe noisy alignment, consider floor/ceiling consistently for both series.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
libs/repositories/src/repos/SimulationRepository/SimulationrepositoryTenderly.test.ts(1 hunks)libs/repositories/src/repos/TokenHolderRepository/TokenHolderRepositoryEthplorer.test.ts(1 hunks)libs/repositories/src/repos/TokenHolderRepository/TokenHolderRepositoryMoralis.spec.ts(1 hunks)libs/services/src/SlippageService/SlippageService.ts(1 hunks)libs/services/src/SlippageService/SlippageServiceMain.spec.ts(14 hunks)libs/services/src/SlippageService/SlippageServiceMain.ts(4 hunks)libs/services/src/SlippageService/SlippageServiceMock.ts(1 hunks)libs/services/src/SlippageService/test-data/1-COW-WETH.json(1 hunks)libs/services/src/SlippageService/test-data/1-DAI-ETH.json(1 hunks)libs/services/src/SlippageService/test-data/1-DAI-WETH.json(1 hunks)libs/services/src/SlippageService/test-data/1-PEPE-DAI.json(1 hunks)libs/services/src/SlippageService/test-data/1-PEPE-WETH.json(1 hunks)libs/services/src/SlippageService/test-data/1-USDC-DAI.json(1 hunks)libs/services/src/SlippageService/test-data/100-USDC-xDAI.json(1 hunks)libs/services/src/SlippageService/test-data/100-WETH-xDAI.json(1 hunks)libs/services/src/SlippageService/test-data/100-sDAI-wxDAI.json(1 hunks)libs/services/src/SlippageService/test-data/100-sDAI-xDAI.json(1 hunks)libs/services/src/SlippageService/test-data/42161-COW-WETH.json(1 hunks)libs/services/src/SlippageService/test-data/42161-DAI-WETH.json(1 hunks)libs/services/src/SlippageService/test-data/42161-USDC-DAI.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-22T11:36:04.042Z
Learnt from: anxolin
PR: cowprotocol/bff#124
File: libs/services/src/factories.ts:124-126
Timestamp: 2025-04-22T11:36:04.042Z
Learning: Simulation repositories (SimulationRepositoryTenderly) don't require a caching layer like other repositories in the system, as the performance benefit isn't worth the implementation effort.
Applied to files:
libs/repositories/src/repos/SimulationRepository/SimulationrepositoryTenderly.test.ts
🔇 Additional comments (17)
libs/services/src/SlippageService/test-data/100-sDAI-xDAI.json (1)
6-6: Confirm the new 33 bps expectation against the recalculated pair volatility.Please double-check that the updated value aligns with the latest slippage computation for this pair (and any mirrored fixtures, if present) so we don’t mask a regression in the new pair-based pipeline.
libs/services/src/SlippageService/test-data/100-WETH-xDAI.json (1)
6-6: Slippage recalibration looks goodThe 67 bps update lines up with the new pair-volatility baselines—happy to see the fixtures refreshed alongside the algorithm tweaks.
libs/services/src/SlippageService/test-data/100-sDAI-wxDAI.json (1)
6-6: Updated tolerance matches the new model33 bps here tracks with the revised volatility inputs, so the fixture stays in sync with the refactored flow.
libs/services/src/SlippageService/test-data/100-USDC-xDAI.json (1)
6-6: Fixture aligns with the refreshed calibrationDropping to 31 bps is consistent with the smart-slippage adjustments—looks solid.
libs/services/src/SlippageService/SlippageService.ts (1)
34-53: API surface expansion fits the refactorThe
PairVolatilityhelper and the newgetVolatilityForPairentry point dovetail nicely with the pair-centric pipeline introduced in this PR.libs/services/src/SlippageService/test-data/1-PEPE-DAI.json (1)
6-6: PEPE/DAI tolerance refresh acknowledgedBumping to 137 bps mirrors the volatility uplift for this pair—fixture update looks on point.
libs/services/src/SlippageService/test-data/42161-USDC-DAI.json (2)
6-6: slippageBps updated to 10 — looks fine; please confirm downstream expectationsValue aligns with “pair‑based volatility” intent per PR summary. Ensure all tests that assert this fixture’s slippage expectations were updated accordingly.
8-15: volatilityDetails roles are correct
All fixtures have volatilityDetails.baseToken/quoteToken tokenAddress matching the top-level baseToken/quoteToken—no inversion present.Likely an incorrect or invalid review comment.
libs/services/src/SlippageService/test-data/42161-DAI-WETH.json (2)
6-6: slippageBps updated to 66 — OK; verify tests and thresholdsSmall bump; confirm any thresholding/rounding logic in SlippageServiceMain still produces 66 for this pair and that tests reflect it.
8-15: Remove incorrect volatilityDetails swap warning top-level baseToken is 0x82aF… (WETH) and volatilityDetails.baseToken matches that, so there is no inversion—comment misreads the fixture.Likely an incorrect or invalid review comment.
libs/services/src/SlippageService/test-data/1-DAI-ETH.json (1)
6-6: slippageBps updated to 63 — OK; confirm ETH/WETH treatment in testsValue looks consistent with minor volatility shift. Make sure tests expecting 63 are updated and any ETH→WETH normalization in code paths still yields this figure.
libs/services/src/SlippageService/test-data/1-DAI-WETH.json (1)
6-6: Confirm slippage fixture regeneration.Please double-check that the new
slippageBps(63) comes from rerunning the updated smart-slippage computation rather than a manual tweak, so the fixture stays in sync with production logic.libs/services/src/SlippageService/test-data/1-PEPE-WETH.json (1)
6-6: Confirm slippage fixture regeneration.Can you verify that the 81 bps value was regenerated with the latest smart-slippage pipeline? I just want to avoid drifting from what the code would now output.
libs/services/src/SlippageService/test-data/42161-COW-WETH.json (1)
6-6: Confirm slippage fixture regeneration.Please confirm the 89 bps figure was produced by the refreshed smart-slippage logic instead of a manual edit to keep the Arbitrum fixture consistent with runtime calculations.
libs/services/src/SlippageService/SlippageServiceMain.ts (1)
24-43: Good failover path: relative → max → 0.The staged fallback from pair-based to per-token to 0 is clear and deterministic.
libs/services/src/SlippageService/test-data/1-USDC-DAI.json (1)
6-6: Verify slippageBps=6 for USDC–DAI against algorithm output
Ensure the new pair-volatility flow produces 6 bps for USDC–DAI and update any tests or fixtures that reference this value. Audit all slippageBps fixtures with:rg -n --type=json '"slippageBps"\s*:\s*\d+' libs/services/src/SlippageService/test-datalibs/services/src/SlippageService/test-data/1-COW-WETH.json (1)
6-6: No additional test updates required
The SlippageServiceMain tests dynamically loadslippageBpsfrom each JSON fixture (including 1-COW-WETH.json), so updating the test-data to 88 is sufficient.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
libs/services/src/SlippageService/SlippageServiceMain.spec.ts (8)
41-101: Describe block title doesn’t match contents (contains non‑zero slippage cases).Rename/split to avoid confusion; keep “0 slippage” only for missing/indeterminable data.
53-55: Fix misleading comment (“max slippage”).It asserts 0; update the comment.
- // THEN: We get the max slippage + // THEN: We get 0 slippage
68-70: Same misleading comment here.- // THEN: We get the max slippage + // THEN: We get 0 slippage
166-168: Comment contradicts assertion (not “minimum slippage”).Clarify outcome as fallback for unmatched dates.
- // THEN: We get the minimum slippage + // THEN: We get the fallback slippage (no matching dates)
187-197: Outdated math in comments (“MAX=200” etc.).The numbers shown don’t match the asserted 2237 and the new pair‑volatility model; remove stale calc or correct it to the current units.
- // Slippage BPS = ceil(0.02236067977 * 10000) = 224 - // Adjusted Slippage = MAX = 200 + // Note: Calculation uses pair-based volatility and current unit scaling; assertions reflect the implementation.
260-272: Numeric example is inconsistent (division typo).0.01118033989 / 100 = 0.000111803... (not 0.1118...). Update comment to avoid confusion.
- // Volatility Fair Settlement (Token) = 0.01118033989 / 100 = 0.1118033989 - // Slippage BPS = ceil(0.1118033989 * 10000) = 112 + // Volatility Fair Settlement (Token) = 0.01118033989 / 100 = 0.0001118033989 + // Slippage BPS = ceil(0.0001118033989 * 10000) = 2
30-39: Reset mocks between tests to avoid leakage.Add reset/clear in beforeEach.
beforeEach(() => { + jest.resetAllMocks(); usdRepositoryMock = { getUsdPrice, getUsdPrices };
486-495: Stabilize time in tests to reduce flakiness.Use a fixed default start time for getPoints; tests that need offsets can override.
-function getPoints( +const TEST_START = new Date('2024-01-01T00:00:00Z').getTime(); + +function getPoints( prices: number[], - timeBetweenPoints = FIVE_MIN, - startDate = Date.now() + timeBetweenPoints = FIVE_MIN, + startDate = TEST_START ): PricePoint[] {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/services/src/SlippageService/SlippageServiceMain.spec.ts(12 hunks)libs/services/src/SlippageService/SlippageServiceMock.ts(0 hunks)libs/services/src/index.ts(0 hunks)
💤 Files with no reviewable changes (2)
- libs/services/src/index.ts
- libs/services/src/SlippageService/SlippageServiceMock.ts
🧰 Additional context used
🧬 Code graph analysis (1)
libs/services/src/SlippageService/SlippageServiceMain.spec.ts (3)
libs/repositories/src/repos/UsdRepository/UsdRepositoryCow.ts (1)
getUsdPrice(23-87)libs/repositories/src/repos/UsdRepository/UsdRepositoryCache.ts (2)
getUsdPrice(28-59)getUsdPrices(60-98)libs/repositories/src/repos/UsdRepository/UsdRepository.ts (1)
PricePoint(5-20)
🔇 Additional comments (2)
libs/services/src/SlippageService/SlippageServiceMain.spec.ts (2)
361-369: 6‑minute spacing expectation off (sqrt time-scaling).For stddev time rescaling, use sqrt(5/6). ~112 bps at 5min → ~103 bps at 6min. Update assertion.
- // Time stretch factor: 5min / 6min = 0.833 (less volatility per settlement time) - // Expected: Lower volatility than 5min intervals due to time stretching - expect(result).toBe(97); // Lower than baseline due to 6min intervals vs 5min + // Time scaling: sqrt(5/6) ≈ 0.9129 (less volatility per settlement time) + // Expected: ~103 bps (scaled from ~112 bps baseline) + expect(result).toBe(103);
389-396: 4‑minute spacing expectation off (sqrt time-scaling).Scale by sqrt(5/4). ~112 bps at 5min → ~125 bps at 4min.
- // Time compression factor: 5min / 4min = 1.25 (more volatility per settlement time) - // Expected: Higher volatility than 5min intervals due to time compression - expect(result).toBe(160); // Consistently calculated value for 4min intervals + // Time scaling: sqrt(5/4) ≈ 1.1180 (more volatility per settlement time) + // Expected: ~125 bps + expect(result).toBe(125);
Summary
Specification: https://www.notion.so/cownation/Speed-Finish-slippage-based-on-market-volatility-2168da5f04ca80e3987bf42b9d6c8a22?source=copy_link
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Chores
Tests