ZeroX DX zeroX.tradeAsset high-level swap method - #665
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new high-level 0x swap execution API (zeroX.tradeAsset) to PortalSwift that fetches a quote, submits the returned transaction via portal.request, and polls for an on-chain receipt before returning.
Changes:
- Introduces
ZeroX.tradeAsset(params:onProgress:)with progress reporting and receipt-confirmation polling. - Wires
PortalintoTrading.zeroXvia a minimalZeroXPortalDependencyprotocol. - Adds a new test suite and extends test mocks, plus an example-app UI action and storyboard button for the new swap flow.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/PortalSwiftTests/Trading/ZeroXTradeAssetTests.swift | Adds async unit tests covering success/progress and key failure paths for tradeAsset. |
| Tests/PortalSwiftTests/Trading/ZeroXMock.swift | Extends ZeroXProtocol test mock to support tradeAsset calls. |
| SPM Example/PortalSwift/MainViewController/ViewController+ZeroXTrading.swift | Adds an example UI handler that calls portal.trading.zeroX.tradeAsset and logs progress. |
| SPM Example/PortalSwift/Base.lproj/Main.storyboard | Adds a new button wired to the example handleZeroXTradeAsset action. |
| Sources/PortalSwift/Trading/ZeroX.swift | Adds ZeroXPortalDependency, new tradeAsset API, and confirmation polling implementation. |
| Sources/PortalSwift/Trading/Trading.swift | Passes the portal dependency through to ZeroX construction. |
| Sources/PortalSwift/Portal.swift | Injects Portal into Trading and declares conformance to ZeroXPortalDependency. |
| Sources/PortalSwift/Core/Api/zeroX/ZeroXTradeAsset.swift | Introduces ZeroXTradeAssetParams, progress types, result type, and error enum. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…alSwift into ragab/sdk-70-ios-sdk-implement-zeroxtradeasset-high-level-swap-method * 'release-candidate' of https://github.com/portal-hq/PortalSwift: update code owners
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-candidate #665 +/- ##
=====================================================
+ Coverage 87.02% 87.14% +0.11%
=====================================================
Files 277 279 +2
Lines 35534 36030 +496
=====================================================
+ Hits 30923 31397 +474
- Misses 4611 4633 +22 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Rshahatit
left a comment
Rshahatit
left a comment
There was a problem hiding this comment.
Summary: Adds a high-level portal.trading.zeroX.tradeAsset() that fetches a 0x quote, signs/broadcasts it via eth_sendTransaction, and polls for on-chain confirmation in one call, with onProgress callbacks through the flow (fetching_quote → signing → submitted → confirming → confirmed/failed).
How it works: ZeroX.tradeAsset fetches a quote via the existing getQuote API, validates it (non-empty error string, data.rawResponse, non-empty transaction.to), then sends the returned transaction object through the ZeroXPortalDependency.request bridge (implemented by Portal) and polls eth_getTransactionReceipt on a fixed interval (4s × up to 225 attempts ≈ 15 min) until status == "0x1", throwing ZeroXTradeAssetError.confirmationFailed on revert or timeout. Trading/Portal now thread a portal (or nil, for tests) into ZeroX via the new ZeroXPortalDependency protocol, keeping the mock-ability the test suite relies on.
Left two comments inline — one likely bug (an unused/dead parameter that silently drops caller-supplied data) and one design question about parity with the documented cross-SDK options pattern. Nice test coverage on the happy/failure/timeout paths otherwise.
Generated by Claude Code
| onProgress: ((ZeroXTradeAssetProgressStatus, ZeroXTradeAssetProgressData) -> Void)? = nil | ||
| ) async throws -> ZeroXTradeAssetResult { | ||
| func report(_ status: ZeroXTradeAssetProgressStatus, _ data: ZeroXTradeAssetProgressData = ZeroXTradeAssetProgressData()) { | ||
| onProgress?(status, data) |
There was a problem hiding this comment.
Nit/question: tradeAsset here has no options parameter — signing always goes through eth_sendTransaction and confirmation always goes through the private waitForConfirmation poller below, with no per-call or instance-level override hook. The cross-SDK high-level-methods design doc (the one this PR's type names — ZeroXTradeAssetParams/ProgressStatus/ProgressData/Result — line up with) specifies a ZeroXTradeAssetOptions with signAndSendTransaction and waitForConfirmation overrides, same priority pattern as LiFi/Yield ("no fallback poller" — should throw if neither per-call nor instance-level confirmation is configured). Was that scoped out intentionally for this first pass, or should it be added before this ships so iOS stays consistent with the other SDKs?
Generated by Claude Code
There was a problem hiding this comment.
This was not part of the requirements on the STD document
Rshahatit
left a comment
Rshahatit
left a comment
There was a problem hiding this comment.
address these comments before merging #665 (review)
…k-70-ios-sdk-implement-zeroxtradeasset-high-level-swap-method # Conflicts: # SPM Example/PortalSwift/Base.lproj/Main.storyboard # Sources/PortalSwift/Portal.swift # Sources/PortalSwift/Trading/Trading.swift
There was a problem hiding this comment.
🟡 Not ready to approve
There are confirmed issues in the new receipt polling loop (avoidable initial delay/no retry on transient RPC errors) and in the example action’s UI updates potentially running off the main thread.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
SPM Example/PortalSwift/MainViewController/ViewController+ZeroXTrading.swift:365
- Error-path UI updates (
showStatusView/stopLoading) are executed inside an unscopedTask { ... }and may run off the main thread. Dispatch these calls to the main actor to avoid UIKit threading issues.
self.logger.error("ViewController.handleZeroXTradeAsset() - ❌ Unable to trade asset with error: \(error)")
self.showStatusView(message: "\(self.failureStatus) Unable to trade asset with error: \(error)")
self.stopLoading()
SPM Example/PortalSwift/MainViewController/ViewController+ZeroXTrading.swift:361
showStatusView/stopLoadingare UIKit updates but they’re executed inside an unscopedTask { ... }, which may run off the main thread. This can lead to UIKit threading violations; wrap UI updates inMainActor.run(or otherwise ensure they run on the main actor).
This issue also appears on line 363 of the same file.
self.logger.info("ViewController.handleZeroXTradeAsset() - ✅ Swap confirmed. Hashes: \(result.hashes)")
self.showStatusView(message: "\(self.successStatus) Swap confirmed: \(result.hashes.joined(separator: ", "))")
self.stopLoading()
Sources/PortalSwift/Trading/ZeroX.swift:394
waitForConfirmationsleeps before the very first receipt poll and also fails immediately on transient RPC errors. This adds an avoidable delay (default 4s) even when the receipt is already available, and makes swaps flaky ifeth_getTransactionReceipttemporarily fails. Align this polling loop with the existing Portal/Yield confirmation loops by polling immediately on the first attempt, checking cancellation, and retrying on non-cancellation errors.
private func waitForConfirmation(txHash: String, chainId: String, portal: ZeroXPortalDependency) async throws -> Bool {
for _ in 0 ..< confirmationMaxAttempts {
try await Task.sleep(nanoseconds: confirmationPollIntervalNanoseconds)
let response = try await portal.request(
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The new receipt-confirmation polling currently adds avoidable latency and is brittle to transient RPC errors, which can degrade swap reliability in production.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
Sources/PortalSwift/Trading/ZeroX.swift:394
waitForConfirmationsleeps before the firsteth_getTransactionReceiptpoll and also fails the whole swap on any transient receipt-RPC error. This adds an unnecessary ~4s delay on already-mined txs (default interval) and makes confirmation less reliable. Align with existing polling patterns in the codebase (e.g., Portal.waitForTransactionConfirmation polls immediately and retries on transient errors) by polling immediately, sleeping only between attempts, and swallowing non-cancellation errors while continuing to poll.
private func waitForConfirmation(txHash: String, chainId: String, portal: ZeroXPortalDependency) async throws -> Bool {
for _ in 0 ..< confirmationMaxAttempts {
try await Task.sleep(nanoseconds: confirmationPollIntervalNanoseconds)
let response = try await portal.request(
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The new receipt-polling implementation in ZeroX.waitForConfirmation has reliability/UX regressions (initial sleep before first poll and no transient-error retry behavior) compared to established patterns elsewhere in the codebase.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
Sources/PortalSwift/Trading/ZeroX.swift:394
waitForConfirmationsleeps before the first receipt check and fails immediately on any transient RPC error. Elsewhere in the codebase (e.g. Portal.waitForTransactionConfirmation and YieldXyz.waitForConfirmation) the first poll happens immediately, cancellation is checked, and transient errors are retried—this avoids an unnecessary initial 4s delay and improves reliability when nodes intermittently fail.
for _ in 0 ..< confirmationMaxAttempts {
try await Task.sleep(nanoseconds: confirmationPollIntervalNanoseconds)
let response = try await portal.request(
chainId: chainId,
Sources/PortalSwift/Trading/ZeroX.swift:307
- For
eth_sendTransactionthe code builds a raw[String: Any]dictionary. In this repo, other call sites use the strongly-typedETHTransactionParam(e.g.Portal.signAndSendTransactionandYieldXyz), which reduces encoding/key mismatches and keeps the request payload consistent across features.
let txParams: [String: Any] = [
"from": transaction.from,
"to": transaction.to,
"data": transaction.data,
"value": transaction.value,
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
Confirmation polling and example-app UI updates include reliability/threading issues that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
Sources/PortalSwift/Trading/ZeroX.swift:387
waitForConfirmationsleeps before the first receipt poll and fails the entire swap on any transienteth_getTransactionReceiptRPC error. This adds unnecessary latency (even if the receipt is already available) and reduces reliability compared to existing confirmation polling helpers that poll immediately and retry on transient errors/cancellation.
private func waitForConfirmation(txHash: String, chainId: String, portal: ZeroXPortalDependency) async throws {
for _ in 0 ..< confirmationMaxAttempts {
try await Task.sleep(nanoseconds: confirmationPollIntervalNanoseconds)
let response = try await portal.request(
Sources/PortalSwift/Core/Api/zeroX/ZeroXPriceRequest.swift:16
- The doc comment for
chainIdstill says it is "used in URL path", which contradicts the updated note above (and the actual request body includeschainId). This can confuse SDK consumers about how the API is called.
/// Request model for getting a price quote from 0x (without transaction data).
/// Note: `chainId` is included in the request body, not the URL path.
public struct ZeroXPriceRequest: Codable {
/// The chain ID for the price check (used in URL path, e.g., "eip155:1")
public let chainId: String
SPM Example/PortalSwift/MainViewController/ViewController+ZeroXTrading.swift:367
showStatusViewandstopLoadingupdate UIKit state, but they’re called from inside aTaskwithout hopping back to the main thread. This can cause intermittent UI issues/crashes; wrap UI updates inawait MainActor.run { ... }.
self.showStatusView(message: "\(self.successStatus) Swap confirmed: \(result.hashes.joined(separator: ", "))")
self.stopLoading()
} catch {
self.logger.error("ViewController.handleZeroXTradeAsset() - ❌ Unable to trade asset with error: \(error)")
self.showStatusView(message: "\(self.failureStatus) Unable to trade asset with error: \(error)")
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Not ready to approve
The new confirmation polling helper has avoidable initial delay and brittle error handling on transient receipt-RPC failures, and there’s also a misleading ZeroXPriceRequest.chainId doc comment to fix.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
Sources/PortalSwift/Core/Api/zeroX/ZeroXPriceRequest.swift:15
- The doc comment for
chainIdstill says it’s used in the URL path, but this PR updates the API to sendchainIdin the request body (and the 0x endpoints use fixed URLs). This is misleading for SDK consumers.
/// Note: `chainId` is included in the request body, not the URL path.
public struct ZeroXPriceRequest: Codable {
/// The chain ID for the price check (used in URL path, e.g., "eip155:1")
public let chainId: String
Sources/PortalSwift/Trading/ZeroX.swift:387
waitForConfirmationsleeps before the first receipt check and fails immediately on any transienteth_getTransactionReceiptRPC error. This adds an unconditional initial delay (4s by default) and can make swaps flaky during intermittent RPC issues; Portal’s own confirmation helper polls immediately and retries on transient failures.
private func waitForConfirmation(txHash: String, chainId: String, portal: ZeroXPortalDependency) async throws {
for _ in 0 ..< confirmationMaxAttempts {
try await Task.sleep(nanoseconds: confirmationPollIntervalNanoseconds)
let response = try await portal.request(
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Summary
ZeroX DX zeroX.tradeAsset high-level swap method