fix(trading): take quote network costs into account in postSellNativeCurrencyOrder - #967
Conversation
…CurrencyOrder `TradingSdk.postSellNativeCurrencyOrder()` fetched a quote and then forwarded only `advancedSettings.additionalParams` to `postSellNativeCurrencyOrder()`, dropping the costs of the quote it had just fetched. For SELL orders the `/quote` API returns `sellAmount` AFTER network costs, so the amount the user asked to sell is `quote.sellAmount + quote.feeAmount`. With `networkCostsAmount` missing it defaults to `'0'`, so `getOrderToSign()` builds the EthFlow order with `beforeAllFees.sellAmount === quote.sellAmount`. The on-chain `createOrder` call is then sent with that reduced amount as `msg.value`: selling 0.1 ETH creates an order that sells 0.098646335338956442 ETH, priced as if there were no network costs at all. `postSwapOrder()` also routes native sell tokens through the EthFlow flow (via `postCoWProtocolTrade()`), and it does forward `networkCostsAmount`, so the two documented entry points produced different orders for the same parameters. Forward `networkCostsAmount` and `protocolFeeBps` from the quote response, mirroring `postSwapOrderFromQuote()`. `advancedSettings.additionalParams` is still spread last, so explicit overrides keep working.
📝 WalkthroughWalkthrough
ChangesNative SELL order cost forwarding
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/trading/src/tradingSdk.postSellNativeCurrencyOrder.test.tsParsing error: Unable to parse the specified 'tsconfig' file. Ensure it's correct and has valid syntax. packages/contracts-ts/tsconfig.json(2,14): error TS6053: File ' packages/trading/src/tradingSdk.tsParsing error: Unable to parse the specified 'tsconfig' file. Ensure it's correct and has valid syntax. packages/contracts-ts/tsconfig.json(2,14): error TS6053: File ' 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/trading/src/tradingSdk.postSellNativeCurrencyOrder.test.ts (1)
39-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for nonzero
protocolFeeBps.The fixture omits
protocolFeeBps. Therefore, the conversion and forwarding branch inpackages/trading/src/tradingSdk.tsLine 277 does not run. Add a quote with a nonzero protocol fee and assert the derived EthFlow order applies that fee.🤖 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 `@packages/trading/src/tradingSdk.postSellNativeCurrencyOrder.test.ts` around lines 39 - 61, Extend the SELL_QUOTE_MOCK test coverage with a nonzero protocolFeeBps value, then assert the EthFlow order produced by the trading SDK conversion and forwarding path applies the corresponding protocol fee. Ensure the assertion targets the derived order’s fee-related field and preserves existing zero-fee behavior.
🤖 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.
Nitpick comments:
In `@packages/trading/src/tradingSdk.postSellNativeCurrencyOrder.test.ts`:
- Around line 39-61: Extend the SELL_QUOTE_MOCK test coverage with a nonzero
protocolFeeBps value, then assert the EthFlow order produced by the trading SDK
conversion and forwarding path applies the corresponding protocol fee. Ensure
the assertion targets the derived order’s fee-related field and preserves
existing zero-fee behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bb9391ba-b883-401d-af68-4c7a600a2ce2
📒 Files selected for processing (2)
packages/trading/src/tradingSdk.postSellNativeCurrencyOrder.test.tspackages/trading/src/tradingSdk.ts
Selling 0.1 ETH through
TradingSdk.postSellNativeCurrencyOrder()creates an EthFlow order that sells0.098646335338956442 ETH, and sends that same reduced amount as the transactionvalue. The user under-sells, and the order is signed as if network costs were zero, so the solver is left no room for gas while still owing the quotedbuyAmount— it typically never fills and has to be refunded aftervalidTo.The reproduction is the README example for this method, unchanged:
Cause
TradingSdk.postSellNativeCurrencyOrder()fetches a quote and then forwards onlyadvancedSettings?.additionalParams, dropping the costs of the quote it just fetched:For SELL orders the
/quoteAPI returnssellAmountafter network costs, so the amount the user asked to sell isquote.sellAmount + quote.feeAmount. WithnetworkCostsAmountmissing it falls back to'0'ingetEthFlowTransaction(),getQuoteAmountsAndCosts()then computesbeforeAllFees.sellAmount === quote.sellAmount,getOrderToSign()signs that, andgetEthFlowTransaction()sends it astransaction.value.postSwapOrder()routes native sell tokens through the same EthFlow path (viapostCoWProtocolTrade()) and does forwardnetworkCostsAmountfrom the quote, so the two documented entry points produced different orders for identical parameters.Fix
Forward
networkCostsAmountandprotocolFeeBpsfrom the quote response, mirroring whatpostSwapOrderFromQuote()already does....advancedSettings?.additionalParamsis spread last, so anyone already passing either value by hand keeps their behaviour — same precedence aspostSwapOrderFromQuote().Tests
New:
packages/trading/src/tradingSdk.postSellNativeCurrencyOrder.test.ts— 3 tests, run against all three adapters (ethers v5 / ethers v6 / viem). The quote mock is the one already used bypostSwapOrder.test.ts:sellAmount = 98646335338956442,feeAmount = 1353664661043558.valuesellquote.sellAmount + quote.feeAmount. Onmainthis fails withExpected: "100000000000000000"/Received: "98646335338956442".postSellNativeCurrencyOrder()andpostSwapOrder()build the same order for a native sell token. Also fails onmain.advancedSettings.additionalParams.networkCostsAmountstill overrides the quote value, so a future refactor can't silently reverse the spread order.@cowprotocol/sdk-tradinggoes from 22 to 23 suites and 254 to 257 passing (2 skipped); no pre-existing test changed status. Fullturbo run testis 36/36 green, andtsc --noEmitandeslintare clean onpackages/trading.Summary by CodeRabbit
Bug Fixes
Tests