feat(trading): add externally signed order submission - #925
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe trading package adds helpers for constructing and submitting externally signed orders. It also exports the new APIs, documents the signer-less workflow, adds tests, and provides ethers v6 and viem Node.js examples. ChangesSigner-less trading flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant TradingSdk
participant getOrderToSubmit
participant ExternalSigner
participant postSignedOrder
participant OrderBookApi
TradingSdk->>getOrderToSubmit: build order body from quote
ExternalSigner->>ExternalSigner: sign order typed data
TradingSdk->>postSignedOrder: submit order body and signature
postSignedOrder->>OrderBookApi: upload app data when required
postSignedOrder->>OrderBookApi: send order with signature
OrderBookApi-->>TradingSdk: return order id and signing result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 ✍️ ✅ |
📦 GitHub Packages PublishedLast updated: Jul 23, 2026, 01:58:25 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-925
# pnpm
pnpm install npm:@cowprotocol/cow-sdk@pr-925
# NPM
npm install npm:@cowprotocol/cow-sdk@pr-925Update 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 |
|
I have read the CLA Document and I hereby sign the CLA |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
examples/nodejs/ethers6/README.md (1)
39-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code block.
Specifying a language for fenced code blocks improves syntax highlighting and satisfies markdown linters.
♻️ Proposed refactor
-``` +```sh pnpm --filter example-nodejs-ethers6 dev:signerless</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@examples/nodejs/ethers6/README.mdaround lines 39 - 41, Update the fenced
code block containing the pnpm command in the README to specify the shell
language identifier, using the existing command unchanged.</details> <!-- cr-comment:v1:ab04d333676c6735d1f4441e --> </blockquote></details> <details> <summary>examples/nodejs/viem/src/signerless.ts (2)</summary><blockquote> `53-53`: _🎯 Functional Correctness_ | _🔵 Trivial_ | _💤 Low value_ **Use `parseUnits` for robust token amount calculation.** Replace the floating-point calculation with `parseUnits` for accurate conversion. <details> <summary>♻️ Proposed refactor</summary> ```diff - const amount = BigInt(Math.floor(DEFAULT_SELL_AMOUNT * 10 ** WETH.decimals)).toString() + const amount = parseUnits(DEFAULT_SELL_AMOUNT, WETH.decimals).toString()🤖 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 `@examples/nodejs/viem/src/signerless.ts` at line 53, Update the amount calculation in signerless.ts to use viem’s parseUnits with the sell amount and WETH.decimals, replacing the floating-point Math.floor and BigInt conversion while preserving the resulting token-unit string.
29-30: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse a string for the token amount and
parseUnitsto avoid precision loss.Using a JavaScript
numberand floating-point arithmetic for token amounts can lead to precision loss. Consider definingDEFAULT_SELL_AMOUNTas a string and using viem'sparseUnitshelper.♻️ Proposed refactor
First, add
parseUnitsto theviemimport at the top of the file:import { createPublicClient, http, parseUnits, type TypedDataDefinition } from 'viem'Then update the amount constant:
-const DEFAULT_SELL_AMOUNT = 0.1 // WETH amount +const DEFAULT_SELL_AMOUNT = '0.1' // WETH amount // ===============================================================🤖 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 `@examples/nodejs/viem/src/signerless.ts` around lines 29 - 30, Update DEFAULT_SELL_AMOUNT to a string value and use viem’s parseUnits when converting it to the token amount, adding parseUnits to the existing viem imports. Ensure the resulting amount uses the token’s appropriate decimals and avoid JavaScript floating-point arithmetic throughout the sell flow.examples/nodejs/viem/README.md (1)
40-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code block.
Specifying a language for fenced code blocks improves syntax highlighting and satisfies markdown linters.
♻️ Proposed refactor
-``` +```sh pnpm --filter example-nodejs-viem dev:signerless</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@examples/nodejs/viem/README.mdaround lines 40 - 42, Update the fenced code
block containing the pnpm command to specify the shell language identifier,
preserving the command unchanged.</details> <!-- cr-comment:v1:25c51cfbb1c6761f54a4c130 --> </blockquote></details> <details> <summary>examples/nodejs/ethers6/src/signerless.ts (1)</summary><blockquote> `50-50`: _🎯 Functional Correctness_ | _🔵 Trivial_ | _💤 Low value_ **Use `parseUnits` to avoid floating-point precision loss.** Using `Number` and `Math.round` for token amounts can lead to precision loss due to JavaScript's floating-point arithmetic. Consider using `ethers.parseUnits` to reliably handle token decimals. <details> <summary>♻️ Proposed refactor</summary> First, add `parseUnits` to the `ethers` import at the top of the file: ```ts import { JsonRpcProvider, Wallet, parseUnits } from 'ethers'Then update the amount calculation:
- const amount = Math.round(Number(DEFAULT_SELL_AMOUNT) * 10 ** WETH.decimals).toString() + const amount = parseUnits(DEFAULT_SELL_AMOUNT, WETH.decimals).toString()🤖 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 `@examples/nodejs/ethers6/src/signerless.ts` at line 50, Update the amount calculation in signerless.ts to use ethers’ parseUnits instead of Number and Math.round, preserving DEFAULT_SELL_AMOUNT and WETH.decimals as the value and unit inputs so token amounts are represented without floating-point precision loss. Add parseUnits to the existing ethers import.
🤖 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 `@packages/trading/src/getOrderToSubmit.ts`:
- Around line 56-60: Update the native-token guard in getOrderToSubmit to
inspect the authoritative orderToSign.sellToken rather than
getIsEthFlowOrder(tradeParameters) or other quote metadata. Ensure only orders
whose signed sell token is native are rejected, while valid ERC-20 orders
proceed.
---
Nitpick comments:
In `@examples/nodejs/ethers6/README.md`:
- Around line 39-41: Update the fenced code block containing the pnpm command in
the README to specify the shell language identifier, using the existing command
unchanged.
In `@examples/nodejs/ethers6/src/signerless.ts`:
- Line 50: Update the amount calculation in signerless.ts to use ethers’
parseUnits instead of Number and Math.round, preserving DEFAULT_SELL_AMOUNT and
WETH.decimals as the value and unit inputs so token amounts are represented
without floating-point precision loss. Add parseUnits to the existing ethers
import.
In `@examples/nodejs/viem/README.md`:
- Around line 40-42: Update the fenced code block containing the pnpm command to
specify the shell language identifier, preserving the command unchanged.
In `@examples/nodejs/viem/src/signerless.ts`:
- Line 53: Update the amount calculation in signerless.ts to use viem’s
parseUnits with the sell amount and WETH.decimals, replacing the floating-point
Math.floor and BigInt conversion while preserving the resulting token-unit
string.
- Around line 29-30: Update DEFAULT_SELL_AMOUNT to a string value and use viem’s
parseUnits when converting it to the token amount, adding parseUnits to the
existing viem imports. Ensure the resulting amount uses the token’s appropriate
decimals and avoid JavaScript floating-point arithmetic throughout the sell
flow.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba9c542e-febe-4e5a-8fce-788b13f5e751
📒 Files selected for processing (13)
examples/nodejs/ethers6/README.mdexamples/nodejs/ethers6/package.jsonexamples/nodejs/ethers6/src/signerless.tsexamples/nodejs/viem/README.mdexamples/nodejs/viem/package.jsonexamples/nodejs/viem/src/signerless.tspackages/trading/README.mdpackages/trading/src/getOrderToSubmit.test.tspackages/trading/src/getOrderToSubmit.tspackages/trading/src/index.tspackages/trading/src/postSignedOrder.test.tspackages/trading/src/postSignedOrder.tspackages/trading/src/tradingSdk.ts
| if (getIsEthFlowOrder(tradeParameters)) { | ||
| throw new Error( | ||
| 'Orders selling the native token cannot be submitted to the order book. Use getEthFlowTransaction instead.', | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check the signed order’s sell token instead of quote metadata.
Line 56 can disagree with the payload being submitted when deserialized inputs contain stale tradeParameters. Use the authoritative orderToSign.sellToken; otherwise a native-token order may pass this guard, or a valid ERC-20 order may be rejected.
Proposed fix
- if (getIsEthFlowOrder(tradeParameters)) {
+ if (getIsEthFlowOrder(orderToSign)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (getIsEthFlowOrder(tradeParameters)) { | |
| throw new Error( | |
| 'Orders selling the native token cannot be submitted to the order book. Use getEthFlowTransaction instead.', | |
| ) | |
| } | |
| if (getIsEthFlowOrder(orderToSign)) { | |
| throw new Error( | |
| 'Orders selling the native token cannot be submitted to the order book. Use getEthFlowTransaction instead.', | |
| ) | |
| } |
🤖 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/getOrderToSubmit.ts` around lines 56 - 60, Update the
native-token guard in getOrderToSubmit to inspect the authoritative
orderToSign.sellToken rather than getIsEthFlowOrder(tradeParameters) or other
quote metadata. Ensure only orders whose signed sell token is native are
rejected, while valid ERC-20 orders proceed.
| ) | ||
| } | ||
|
|
||
| if (getIsEthFlowOrder(tradeParameters)) { |
There was a problem hiding this comment.
In fact, we use only two fields from tradeParameters: owner and sellToken.
sellToken is already present in orderToSign.
My concern is that we need to minimize the parameters of getOrderToSubmit function as much as possible.
It looks like we can get rid of tradeParameters by replacing it with just owner.
There was a problem hiding this comment.
Same thing with quoteResponse, we only use id from it here
There was a problem hiding this comment.
Good point, thanks, you're right.
One thing I want to ask before doing it, the ticket (https://linear.app/cowswap/project/sdk-external-signer-less-order-signing-b006c53af4e0/overview#scope-proposed-work-3d2fdecd) proposes the happy path as sdk.getOrderToSubmit(quoteResults) - i.e. hand it the whole quote result you already got from getQuoteOnly. Dropping to { orderToSign, appDataInfo, owner, quoteId } requires less arguments, but pushes destructuring onto every caller, which moves away from that recipe.
Do you think we should do it?
| return { | ||
| ...orderToSign, | ||
| from: owner, | ||
| signingScheme: SigningScheme.EIP712, |
There was a problem hiding this comment.
I believe it can be any of SigningScheme
There was a problem hiding this comment.
Good point, I should just have it as optional argument (default to EIP712). P3 will cover EIP1271 in the future
2 participants
Summary
Adds a first-class, documented signer-less path to the Trading SDK: complete a swap with only an
owneraddress and an externally-produced EIP-712 signature - no signer or private key is ever passed to the SDK. This is for integrators using cold wallets (Tangem), MPC/custody services (Fireblocks), or smart accounts, who sign the payload in their own environment and hand the signature back.The flow already worked via low-level calls but was undocumented and required hand-assembling the
sendOrderbody. This PR makes it a supported, documented API (the P0 milestone).Related: https://linear.app/cowswap/project/sdk-external-signer-less-order-signing-b006c53af4e0
New API surface (
@cowprotocol/sdk-trading):getOrderToSubmit(quoteResults)- builds thesendOrderbody from a quote, everything except the signature.sdk.postSignedOrder(orderToSubmit, signature)- uploads app-data and submits the externally signed order (also available as the standalonepostSignedOrder(orderBookApi, orderToSubmit, signature)).Testing
sendOrderparity testexamples/nodejs/{ethers6,viem}/src/signerless.tsSummary by CodeRabbit
Summary by CodeRabbit
New Features
getOrderToSubmit,postSignedOrder) and aTradingSdk.postSignedOrdermethod.Documentation
Tests