feat(bridging): add Relay bridge provider - #846
Open
cozy-hn wants to merge 4 commits into
Open
Conversation
Add ReceiverAccountBridgeProvider implementation for Relay Protocol, enabling cross-chain bridging via deposit addresses. - RelayApi: fetch wrapper for /currencies/v2, /quote/v2, /intents/status/v3, /requests/v2 - RelayBridgeProvider: full BridgeProvider interface implementation - Support for all CoW SDK chains (11 networks including Optimism) - Optional API key support via x-api-key header - Native address mapping (Relay 0x000...000 <-> CoW ETH_ADDRESS) - Fee/slippage computation utilities - getBridgingParams uses requestId from stored quoteBody for status tracking - Unit tests (54 passing) + integration tests against live Relay API - refundTo field support for strict deposit address mode Constraint: Relay API /requests/v2 depositAddress query returns empty — using requestId from quoteBody as primary lookup Rejected: depositAddress-based lookup | API returns empty results for deposit addresses Confidence: high Scope-risk: narrow Not-tested: API key rate limit behavior (no key provisioned yet)
Relay API may return native addresses with mixed case. toRelayAddress already uses toLowerCase() — align fromRelayAddress. Confidence: high Scope-risk: narrow
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Relay bridge integration: new RelayApi HTTP client with caching and error mapping, RelayBridgeProvider implementation and types/constants, utility functions for fee/slippage/address handling, tests, and a public re-export of the provider and its option/result types. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Provider as RelayBridgeProvider
participant API as RelayApi
participant Relay as Relay HTTP
Client->>Provider: getQuote(request)
Provider->>API: POST /quote/v2 (build request, force strict/useDepositAddress)
API->>Relay: POST /quote/v2
Relay-->>API: RelayQuoteResponse
API-->>Provider: RelayQuoteResponse
Provider->>Provider: map fees/slippage/limits, extract depositAddress
Provider-->>Client: RelayQuoteResult
Client->>Provider: getStatus(bridgingId)
Provider->>API: GET /intents/status/v3?requestId=...
API->>Relay: GET /intents/status/v3
Relay-->>API: RelayStatusResponse
API-->>Provider: RelayStatusResponse
Provider->>Provider: translate status -> BridgeStatus
Provider-->>Client: BridgeStatusResult
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
packages/bridging/src/providers/relay/RelayApi.test.ts (1)
8-14: Restoreglobal.fetchafter the suite to avoid cross-test leakage.Line 9 mutates a global and never restores it. Add teardown to keep test isolation stable.
♻️ Proposed fix
describe('RelayApi', () => { const mockFetch = jest.fn() + const originalFetch = global.fetch beforeAll(() => { - global.fetch = mockFetch + global.fetch = mockFetch as typeof fetch }) + + afterAll(() => { + global.fetch = originalFetch + }) beforeEach(() => { mockFetch.mockReset() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bridging/src/providers/relay/RelayApi.test.ts` around lines 8 - 14, The test suite replaces global.fetch with mockFetch in the beforeAll and resets mockFetch in beforeEach but never restores the original fetch, risking cross-test leakage; capture the original global.fetch (e.g., const originalFetch) before assigning mockFetch in beforeAll and add an afterAll teardown that restores global.fetch = originalFetch so other suites are unaffected (refer to global.fetch, mockFetch, beforeAll, beforeEach, and add afterAll to the test file).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bridging/src/providers/relay/RelayApi.spec.ts`:
- Around line 3-75: The test suite described in "describe('RelayApi: Shape of
API response')" runs real network calls and should be gated; update
RelayApi.spec.ts to skip by default and only run when an env var (e.g.
RUN_RELAY_INTEGRATION=true or RUN_INTEGRATION_TESTS) is set: wrap the suite in a
conditional that calls describe.skip when the env var is not present, or
programmatically call describe(...) only when present, so RelayApi tests (and
calls to RelayApi.getCurrencies, getQuote, getRequests, getStatus) do not run in
CI by default.
In `@packages/bridging/src/providers/relay/RelayApi.ts`:
- Around line 65-73: The fetch call in RelayApi (the block assigning response =
await fetch(url, options)) can hang; wrap the request with an
AbortController-based timeout: create an AbortController, add its signal to the
fetch options passed to the existing fetch call, start a setTimeout that calls
controller.abort() after a configured timeout (e.g., configurable constant) and
clear the timeout when fetch resolves; in the catch, detect an abort (error.name
=== 'AbortError' or similar) and map it to a
BridgeProviderQuoteError(BridgeQuoteErrors.API_ERROR) with a timeout-specific
message, otherwise preserve the existing network error handling; update the
function in RelayApi.ts that performs the fetch to use this pattern.
- Line 95: The code currently returns (await response.json()) as T and lets JSON
parse errors (SyntaxError) bubble up; wrap the response.json() call in a
try/catch inside the RelayApi method that performs the fetch, catch
JSON/SyntaxError and throw the bridge-level normalized error type
INVALID_API_JSON_RESPONSE (preserving original error details in the new error),
so callers always receive the standardized error instead of raw SyntaxError when
Relay responds with invalid JSON.
In `@packages/bridging/src/providers/relay/RelayBridgeProvider.test.ts`:
- Around line 86-101: The test mutates global.fetch with mockFetch but only
restores it at the end, which can leak if an assertion throws; update the test
so global.fetch is always restored by wrapping the mutation in a try/finally
(set originalFetch before assignment, run providerWithKey.testApi.getCurrencies
and assertions inside try, and restore global.fetch = originalFetch in finally)
or move the restore into an afterEach that resets global.fetch; target the
mockFetch/global.fetch usage in RelayBridgeProvider.test (references: mockFetch,
originalFetch, TestRelayBridgeProvider, providerWithKey.testApi.getCurrencies).
In `@packages/bridging/src/providers/relay/RelayBridgeProvider.ts`:
- Around line 104-105: The quote and reconstructed deposit params diverge
because getQuote() sets recipient to receiver ?? account while
getBridgingParams() later uses order.owner; make the recipient selection
consistent end-to-end by using the same precedence in both places (e.g.,
recipient = receiver ?? owner ?? account) or by carrying the original recipient
through the quote/result and using that value in getBridgingParams(); update the
recipient assignment in getQuote() (and the similar block at lines 185-203) to
match the chosen precedence and ensure getBridgingParams() reads that same
symbol rather than assuming order.owner.
- Around line 99-116: The getQuote implementation always calls Relay with
tradeType: 'EXACT_INPUT' and treats amount as input, so you must reject BUY-side
requests up front to avoid incorrect quotes: inside getQuote (in
RelayBridgeProvider) add a guard that detects a BUY request on the incoming
QuoteBridgeRequest (e.g., request.side/request.type/request.kind === 'BUY'
depending on the request shape) and throw/return an appropriate error before
invoking this.api.getQuote; ensure the error clearly states BUY quotes are not
supported so callers fail fast instead of receiving a wrong 'EXACT_INPUT' quote.
In `@packages/bridging/src/providers/relay/types.ts`:
- Around line 132-135: Update the RelayRequestsResponse interface so the
continuation field can be null to match tests: change the continuation type in
RelayRequestsResponse (in packages/bridging/src/providers/relay/types.ts) from
"continuation?: string" to "continuation?: string | null" (or otherwise allow
string|null) so consumers and tests that set continuation: null type-check
correctly. Ensure references to RelayRequestsResponse or continuation elsewhere
still compile after the change.
In `@packages/bridging/src/providers/relay/util.test.ts`:
- Around line 139-148: Add a regression test in the existing
describe('fromRelayAddress') block to assert that mixed-case zero/native Relay
addresses also map to ETH_ADDRESS; update util.test.ts to call fromRelayAddress
with a mixed-case variant of the zero address (e.g.
'0x0000000000000000000000000000000000000000' with some hex letters capitalized)
and expect the result toBe(ETH_ADDRESS) so fromRelayAddress and the ETH_ADDRESS
constant behavior is locked in.
In `@packages/bridging/src/providers/relay/util.ts`:
- Around line 8-32: computeSlippageBps and computeFeeBps currently trust
string-to-number conversions and can return NaN or out-of-range bps; fix both by
validating parsed values before math: in computeSlippageBps, parse destPercent
with parseFloat and ensure it is finite and between 0 and 100 before multiplying
by 100, else fall back to USD computation; when using currencyIn.amountUsd and
currencyOut.amountUsd parse to Number, ensure both are finite, inUsd > 0, and
result slippage is finite, then clamp final bps to the 0..10000 range (use
Math.round or Math.trunc as appropriate). In computeFeeBps, parse
fees.relayer.amountUsd and details.currencyIn.amountUsd, ensure both are finite
and inUsd > 0, compute fee ratio, and clamp the returned bps to 0..10000,
returning 0 on invalid inputs to avoid propagating NaN or negatives (referencing
computeSlippageBps and computeFeeBps to locate changes).
---
Nitpick comments:
In `@packages/bridging/src/providers/relay/RelayApi.test.ts`:
- Around line 8-14: The test suite replaces global.fetch with mockFetch in the
beforeAll and resets mockFetch in beforeEach but never restores the original
fetch, risking cross-test leakage; capture the original global.fetch (e.g.,
const originalFetch) before assigning mockFetch in beforeAll and add an afterAll
teardown that restores global.fetch = originalFetch so other suites are
unaffected (refer to global.fetch, mockFetch, beforeAll, beforeEach, and add
afterAll to the test file).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5947b710-1662-4bce-b78b-100acf1b6a8b
⛔ Files ignored due to path filters (2)
packages/bridging/src/providers/relay/relay-logo.pngis excluded by!**/*.pngsrc/bridging/providers/relay/relay-logo.pngis excluded by!**/*.png
📒 Files selected for processing (10)
packages/bridging/src/index.tspackages/bridging/src/providers/relay/RelayApi.spec.tspackages/bridging/src/providers/relay/RelayApi.test.tspackages/bridging/src/providers/relay/RelayApi.tspackages/bridging/src/providers/relay/RelayBridgeProvider.test.tspackages/bridging/src/providers/relay/RelayBridgeProvider.tspackages/bridging/src/providers/relay/const/index.tspackages/bridging/src/providers/relay/types.tspackages/bridging/src/providers/relay/util.test.tspackages/bridging/src/providers/relay/util.ts
Comment on lines
+65
to
+73
| let response: Response | ||
| try { | ||
| response = await fetch(url, options) | ||
| } catch (error) { | ||
| throw new BridgeProviderQuoteError(BridgeQuoteErrors.API_ERROR, { | ||
| message: error instanceof Error ? error.message : 'Network error', | ||
| }) | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Add a request timeout for Relay HTTP calls.
Line 67 can hang indefinitely on slow/stalled upstream responses. For quote paths, this is a production reliability risk and should be bounded.
⏱️ Proposed fix (AbortController timeout)
export class RelayApi {
private baseUrl: string
private apiKey?: string
private currencyCache = new Map<string, RelayCurrency[]>()
+ private readonly timeoutMs: number
- constructor(baseUrl?: string, apiKey?: string) {
+ constructor(baseUrl?: string, apiKey?: string, timeoutMs = 15_000) {
this.baseUrl = baseUrl ?? RELAY_API_BASE_URL
this.apiKey = apiKey
+ this.timeoutMs = timeoutMs
}
@@
private async fetchJson<T>(url: string, options?: RequestInit): Promise<T> {
@@
let response: Response
+ const controller = new AbortController()
+ const timeoutId = setTimeout(() => controller.abort(), this.timeoutMs)
try {
- response = await fetch(url, options)
+ response = await fetch(url, { ...options, signal: controller.signal })
} catch (error) {
throw new BridgeProviderQuoteError(BridgeQuoteErrors.API_ERROR, {
message: error instanceof Error ? error.message : 'Network error',
})
+ } finally {
+ clearTimeout(timeoutId)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bridging/src/providers/relay/RelayApi.ts` around lines 65 - 73, The
fetch call in RelayApi (the block assigning response = await fetch(url,
options)) can hang; wrap the request with an AbortController-based timeout:
create an AbortController, add its signal to the fetch options passed to the
existing fetch call, start a setTimeout that calls controller.abort() after a
configured timeout (e.g., configurable constant) and clear the timeout when
fetch resolves; in the catch, detect an abort (error.name === 'AbortError' or
similar) and map it to a BridgeProviderQuoteError(BridgeQuoteErrors.API_ERROR)
with a timeout-specific message, otherwise preserve the existing network error
handling; update the function in RelayApi.ts that performs the fetch to use this
pattern.
| }) | ||
| } | ||
|
|
||
| return (await response.json()) as T |
Contributor
There was a problem hiding this comment.
Normalize JSON parse failures to INVALID_API_JSON_RESPONSE.
Line 95 currently lets SyntaxError escape directly if Relay responds with invalid JSON, bypassing your bridge-level error model.
🧩 Proposed fix
- return (await response.json()) as T
+ try {
+ return (await response.json()) as T
+ } catch (error) {
+ throw new BridgeProviderQuoteError(BridgeQuoteErrors.INVALID_API_JSON_RESPONSE, {
+ url,
+ message: error instanceof Error ? error.message : 'Invalid JSON response',
+ })
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bridging/src/providers/relay/RelayApi.ts` at line 95, The code
currently returns (await response.json()) as T and lets JSON parse errors
(SyntaxError) bubble up; wrap the response.json() call in a try/catch inside the
RelayApi method that performs the fetch, catch JSON/SyntaxError and throw the
bridge-level normalized error type INVALID_API_JSON_RESPONSE (preserving
original error details in the new error), so callers always receive the
standardized error instead of raw SyntaxError when Relay responds with invalid
JSON.
Comment on lines
+104
to
+105
| user: owner ?? account, | ||
| recipient: receiver ?? account, |
Contributor
There was a problem hiding this comment.
Keep the bridge recipient consistent end-to-end.
getQuote() defaults Relay's recipient to account, but getBridgingParams() later reconstructs the beneficiary as order.owner. When owner !== account or the caller set a custom receiver, the quoted destination and the recovered deposit params diverge.
🐛 Proposed fix
- recipient: receiver ?? account,
+ recipient: receiver ?? owner ?? account,
…
- recipient: order.owner as `0x${string}`,
+ recipient: (details.recipient ?? order.owner) as `0x${string}`,Also applies to: 185-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bridging/src/providers/relay/RelayBridgeProvider.ts` around lines
104 - 105, The quote and reconstructed deposit params diverge because getQuote()
sets recipient to receiver ?? account while getBridgingParams() later uses
order.owner; make the recipient selection consistent end-to-end by using the
same precedence in both places (e.g., recipient = receiver ?? owner ?? account)
or by carrying the original recipient through the quote/result and using that
value in getBridgingParams(); update the recipient assignment in getQuote() (and
the similar block at lines 185-203) to match the chosen precedence and ensure
getBridgingParams() reads that same symbol rather than assuming order.owner.
Comment on lines
+8
to
+32
| export function computeSlippageBps(details: RelayQuoteDetails): number { | ||
| // Primary path: use pre-computed slippage from Relay response | ||
| const destPercent = details.slippageTolerance?.destination?.percent | ||
| if (destPercent != null) { | ||
| return Math.round(parseFloat(destPercent) * 100) | ||
| } | ||
|
|
||
| // Fallback: compute from USD values | ||
| const inUsd = Number(details.currencyIn.amountUsd) | ||
| const outUsd = Number(details.currencyOut.amountUsd) | ||
|
|
||
| if (inUsd <= 0) return 0 | ||
|
|
||
| const slippage = 1 - outUsd / inUsd | ||
| return Math.max(0, Math.trunc(slippage * 10_000)) | ||
| } | ||
|
|
||
| export function computeFeeBps(details: RelayQuoteDetails, fees: RelayFees): number { | ||
| const inUsd = Number(details.currencyIn.amountUsd) | ||
| const feeUsd = Number(fees.relayer.amountUsd) | ||
|
|
||
| if (inUsd <= 0) return 0 | ||
|
|
||
| return Math.trunc((feeUsd / inUsd) * 10_000) | ||
| } |
Contributor
There was a problem hiding this comment.
Guard fee/slippage math against invalid or out-of-range numeric payloads.
Line 12, Line 16-17, and Line 27 rely on unchecked string-to-number conversion. If Relay returns malformed numeric fields, these methods can emit NaN (or invalid bps), which can break quote normalization downstream.
🛠️ Proposed hardening
const RELAY_NATIVE_ADDRESS = '0x0000000000000000000000000000000000000000'
+const MAX_BPS = 10_000
+
+function toFiniteNumber(value: string | undefined): number | null {
+ if (value == null) return null
+ const num = Number(value)
+ return Number.isFinite(num) ? num : null
+}
export function computeSlippageBps(details: RelayQuoteDetails): number {
// Primary path: use pre-computed slippage from Relay response
const destPercent = details.slippageTolerance?.destination?.percent
if (destPercent != null) {
- return Math.round(parseFloat(destPercent) * 100)
+ const percent = toFiniteNumber(destPercent)
+ if (percent == null) return 0
+ return Math.min(MAX_BPS, Math.max(0, Math.round(percent * 100)))
}
// Fallback: compute from USD values
- const inUsd = Number(details.currencyIn.amountUsd)
- const outUsd = Number(details.currencyOut.amountUsd)
+ const inUsd = toFiniteNumber(details.currencyIn.amountUsd)
+ const outUsd = toFiniteNumber(details.currencyOut.amountUsd)
- if (inUsd <= 0) return 0
+ if (inUsd == null || outUsd == null || inUsd <= 0) return 0
const slippage = 1 - outUsd / inUsd
- return Math.max(0, Math.trunc(slippage * 10_000))
+ return Math.min(MAX_BPS, Math.max(0, Math.trunc(slippage * MAX_BPS)))
}
export function computeFeeBps(details: RelayQuoteDetails, fees: RelayFees): number {
- const inUsd = Number(details.currencyIn.amountUsd)
- const feeUsd = Number(fees.relayer.amountUsd)
+ const inUsd = toFiniteNumber(details.currencyIn.amountUsd)
+ const feeUsd = toFiniteNumber(fees.relayer.amountUsd)
- if (inUsd <= 0) return 0
+ if (inUsd == null || feeUsd == null || inUsd <= 0 || feeUsd < 0) return 0
- return Math.trunc((feeUsd / inUsd) * 10_000)
+ return Math.min(MAX_BPS, Math.max(0, Math.trunc((feeUsd / inUsd) * MAX_BPS)))
}Based on learnings: The calculateFeeBps function in the Bungee bridge provider includes explicit validation (including division-by-zero and bounds-related checks) before returning bps.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bridging/src/providers/relay/util.ts` around lines 8 - 32,
computeSlippageBps and computeFeeBps currently trust string-to-number
conversions and can return NaN or out-of-range bps; fix both by validating
parsed values before math: in computeSlippageBps, parse destPercent with
parseFloat and ensure it is finite and between 0 and 100 before multiplying by
100, else fall back to USD computation; when using currencyIn.amountUsd and
currencyOut.amountUsd parse to Number, ensure both are finite, inUsd > 0, and
result slippage is finite, then clamp final bps to the 0..10000 range (use
Math.round or Math.trunc as appropriate). In computeFeeBps, parse
fees.relayer.amountUsd and details.currencyIn.amountUsd, ensure both are finite
and inUsd > 0, compute fee ratio, and clamp the returned bps to 0..10000,
returning 0 on invalid inputs to avoid propagating NaN or negatives (referencing
computeSlippageBps and computeFeeBps to locate changes).
- Gate integration tests behind RELAY_INTEGRATION_TESTS env var - Wrap global.fetch mock in try/finally for test isolation - Reject BUY orders in getQuote (only SELL supported) - Allow nullable continuation in RelayRequestsResponse - Add mixed-case native address test for fromRelayAddress Confidence: high Scope-risk: narrow
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/bridging/src/providers/relay/RelayBridgeProvider.ts (1)
207-207:⚠️ Potential issue | 🟡 MinorRecipient in
getBridgingParamsignores the original quote's recipient.The
getQuotemethod setsrecipient: receiver ?? account(line 109), butgetBridgingParamsreconstructs withrecipient: order.owner(line 207). If the original request had a customreceiverdifferent fromowner, this discrepancy could cause issues for downstream systems expecting consistency.Consider extracting the recipient from
quoteBody.details.recipientwhich should contain the value used in the original quote.🐛 Proposed fix
- recipient: order.owner as `0x${string}`, + recipient: (details.recipient ?? order.owner) as `0x${string}`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bridging/src/providers/relay/RelayBridgeProvider.ts` at line 207, getBridgingParams currently sets recipient to order.owner which overwrites a custom receiver from the original quote; change it to use the recipient stored on the quote (quoteBody.details.recipient) when present, falling back to order.owner only if that field is undefined. Locate getBridgingParams in RelayBridgeProvider and replace the hard-coded recipient: order.owner usage with logic that reads quoteBody.details.recipient (or equivalent property used by getQuote) and uses order.owner as a fallback so downstream consumers see the same recipient the quote used.
🧹 Nitpick comments (3)
packages/bridging/src/providers/relay/RelayBridgeProvider.ts (1)
243-249:getCancelBridgingTxandgetRefundBridgingTxthrow synchronously despitePromise<EvmCall>return type.The interface declares these as returning
Promise<EvmCall>, but the implementation throws synchronously. While this works for immediate callers, it's inconsistent with the async contract. If callers expectawait getCancelBridgingTx()to handle errors via.catch(), synchronous throws will bypass that.Given the interface TODO comments indicate these are under review, this may be acceptable for now, but consider either:
- Making the methods async and using
throwinside- Returning
Promise.reject(new Error('Not implemented'))♻️ Consistent async behavior
- getCancelBridgingTx(_bridgingId: string): Promise<EvmCall> { - throw new Error('Not implemented') + async getCancelBridgingTx(_bridgingId: string): Promise<EvmCall> { + throw new Error('Not implemented') } - getRefundBridgingTx(_bridgingId: string): Promise<EvmCall> { - throw new Error('Not implemented') + async getRefundBridgingTx(_bridgingId: string): Promise<EvmCall> { + throw new Error('Not implemented') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bridging/src/providers/relay/RelayBridgeProvider.ts` around lines 243 - 249, The synchronous throws in getCancelBridgingTx and getRefundBridgingTx violate their Promise<EvmCall> signature; change each to return a rejected Promise or be declared async so the error is delivered asynchronously—e.g., update getCancelBridgingTx and getRefundBridgingTx to either be async functions that throw or return Promise.reject(new Error('Not implemented'))—so callers using await/.catch receive the error consistently.packages/bridging/src/providers/relay/util.test.ts (1)
144-147: Mixed-case test only validates prefix, not hex digits.The test at line 145 is identical to line 141 (both lowercase). Line 146 tests uppercase
0Xprefix, but since the zero address has no hex letters (a-f), this doesn't fully validate case-insensitive hex digit handling.Consider testing with an address that has hex letters, or accept that the zero address inherently can't have mixed-case hex digits.
🔧 Alternative: test a non-zero address if fromRelayAddress supports it
If the implementation uses
.toLowerCase()on the full address, you could add a test with a checksummed ERC-20 address to confirm it handles mixed-case properly. However, since the current logic only special-cases the zero address, the existing test may be sufficient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bridging/src/providers/relay/util.test.ts` around lines 144 - 147, The mixed-case test for fromRelayAddress only verifies the 0x/0X prefix and not hex-letter case handling; add (or replace one of) the expectations to use a non-zero Relay address containing hex letters (e.g., with a-f/A-F characters) and assert it normalizes to ETH_ADDRESS (or the expected normalized value) so fromRelayAddress is validated for mixed-case hex digits as well; reference the fromRelayAddress function and ETH_ADDRESS constant when updating the test.packages/bridging/src/providers/relay/RelayBridgeProvider.test.ts (1)
171-213: Missing test for BUY order rejection ingetQuote.The
getQuoteimplementation now rejectsOrderKind.BUYrequests (lines 100-102 in provider), but there's no test case validating this behavior. WhilegetIntermediateTokenshas a non-SELL rejection test (lines 143-147),getQuoteshould have the same coverage.🧪 Add test for BUY order rejection
describe('getQuote', () => { + it('throws on non-sell orders', async () => { + await expect( + provider.getQuote({ kind: OrderKind.BUY } as any), + ).rejects.toThrow('ONLY_SELL_ORDER_SUPPORTED') + }) + it('returns correct RelayQuoteResult', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bridging/src/providers/relay/RelayBridgeProvider.test.ts` around lines 171 - 213, Add a unit test in RelayBridgeProvider.test.ts that verifies provider.getQuote rejects when called with OrderKind.BUY: mock provider.testApi.getQuote (or leave it unused), call provider.getQuote with kind: OrderKind.BUY and the same payload shape used in existing tests, and assert the call rejects (e.g., await expect(provider.getQuote({... kind: OrderKind.BUY ...})).rejects.toThrow()) to match the implementation in provider.getQuote which explicitly rejects BUY requests; reference the getQuote method and OrderKind.BUY in the new test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/bridging/src/providers/relay/RelayBridgeProvider.ts`:
- Line 207: getBridgingParams currently sets recipient to order.owner which
overwrites a custom receiver from the original quote; change it to use the
recipient stored on the quote (quoteBody.details.recipient) when present,
falling back to order.owner only if that field is undefined. Locate
getBridgingParams in RelayBridgeProvider and replace the hard-coded recipient:
order.owner usage with logic that reads quoteBody.details.recipient (or
equivalent property used by getQuote) and uses order.owner as a fallback so
downstream consumers see the same recipient the quote used.
---
Nitpick comments:
In `@packages/bridging/src/providers/relay/RelayBridgeProvider.test.ts`:
- Around line 171-213: Add a unit test in RelayBridgeProvider.test.ts that
verifies provider.getQuote rejects when called with OrderKind.BUY: mock
provider.testApi.getQuote (or leave it unused), call provider.getQuote with
kind: OrderKind.BUY and the same payload shape used in existing tests, and
assert the call rejects (e.g., await expect(provider.getQuote({... kind:
OrderKind.BUY ...})).rejects.toThrow()) to match the implementation in
provider.getQuote which explicitly rejects BUY requests; reference the getQuote
method and OrderKind.BUY in the new test.
In `@packages/bridging/src/providers/relay/RelayBridgeProvider.ts`:
- Around line 243-249: The synchronous throws in getCancelBridgingTx and
getRefundBridgingTx violate their Promise<EvmCall> signature; change each to
return a rejected Promise or be declared async so the error is delivered
asynchronously—e.g., update getCancelBridgingTx and getRefundBridgingTx to
either be async functions that throw or return Promise.reject(new Error('Not
implemented'))—so callers using await/.catch receive the error consistently.
In `@packages/bridging/src/providers/relay/util.test.ts`:
- Around line 144-147: The mixed-case test for fromRelayAddress only verifies
the 0x/0X prefix and not hex-letter case handling; add (or replace one of) the
expectations to use a non-zero Relay address containing hex letters (e.g., with
a-f/A-F characters) and assert it normalizes to ETH_ADDRESS (or the expected
normalized value) so fromRelayAddress is validated for mixed-case hex digits as
well; reference the fromRelayAddress function and ETH_ADDRESS constant when
updating the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e9ccd32-15e7-494d-88dc-2026eb069716
📒 Files selected for processing (5)
packages/bridging/src/providers/relay/RelayApi.spec.tspackages/bridging/src/providers/relay/RelayBridgeProvider.test.tspackages/bridging/src/providers/relay/RelayBridgeProvider.tspackages/bridging/src/providers/relay/types.tspackages/bridging/src/providers/relay/util.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/bridging/src/providers/relay/RelayApi.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bridging/src/providers/relay/types.ts
Replace non-null assertions with optional chaining and early returns for type narrowing in test files. Confidence: high Scope-risk: narrow
Author
|
Verified end-to-end locally by linking the built SDK to the CoW Swap frontend.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RelayBridgeProviderimplementingReceiverAccountBridgeProviderfor Relay Protocolx-api-keyheader) for production rate limits0x000...000) and CoW (ETH_ADDRESS)getBridgingParamsusesrequestIdfrom storedquoteBodyfor status trackingrefundTofield support required for strict deposit address modeFiles
packages/bridging/src/providers/relay/RelayApi.tspackages/bridging/src/providers/relay/RelayBridgeProvider.tspackages/bridging/src/providers/relay/types.tspackages/bridging/src/providers/relay/util.tspackages/bridging/src/providers/relay/const/index.tspackages/bridging/src/providers/relay/relay-logo.pngpackages/bridging/src/index.tsTest coverage
RelayApi.test.ts— API client unit tests (13 tests incl. API key header)RelayApi.spec.ts— Integration tests against live Relay API (5 tests)RelayBridgeProvider.test.ts— Provider logic tests (21 tests)util.test.ts— Utility function tests (15 tests)Known issues
/requests/v2?depositAddress=returns empty — workaround usesrequestIdfromquoteBodyTest plan
Summary by CodeRabbit