-
Notifications
You must be signed in to change notification settings - Fork 169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WebSockets #227
WebSockets #227
Conversation
Hi @dnKaratzas! I've tested the new WebSocket client. It seems to work quite well, and the API is intuitive. The only thing missing, in my opinion, is a logs subscription, which can be super helpful when you need to subscribe to a particular smart contract event. |
Hey @arguiot . Glad to hear it! Regarding logs im working on it. Its the only missing subscription |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still review in progress, but left a few comments.
@@ -141,7 +141,7 @@ class EthereumClientTests: XCTestCase { | |||
func testEthCall() async { | |||
do { | |||
let tx = EthereumTransaction(from: nil, to: EthereumAddress("0x3c1bd6b420448cf16a389c8b0115ccb3660bb854"), value: BigUInt(1800000), data: nil, nonce: 2, gasPrice: BigUInt(400000), gasLimit: BigUInt(50000), chainId: EthereumNetwork.Ropsten.intValue) | |||
let txHash = try await client?.eth_call(tx, block: .Latest) | |||
let txHash = try await client?.eth_call(tx, resolution: .noOffchain(failOnExecutionError: true), block: .Latest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep a version of eth_call
without 'resolution' parameter? Most usecases don't want to offchain (passing noOffchain
as here), so would be less verbose if caller genearally doesn't have to pass it in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DarthMike done. Please check it b5bfebb
@@ -14,7 +14,9 @@ let package = Package( | |||
.package(name: "BigInt", url: "https://github.com/attaswift/BigInt", from: "5.0.0"), | |||
.package(name: "GenericJSON", url: "https://github.com/zoul/generic-json-swift", from: "2.0.0"), | |||
.package(url: "https://github.com/GigaBitcoin/secp256k1.swift.git", .upToNextMajor(from: "0.6.0")), | |||
.package(url: "https://github.com/OpenCombine/OpenCombine.git", from: "0.13.0") | |||
.package(url: "https://github.com/OpenCombine/OpenCombine.git", from: "0.13.0"), | |||
.package(url: "https://github.com/vapor/websocket-kit.git", from: "2.0.0"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we should create different products in package definition to allow importing WSS only if required. What do you think @dnKaratzas ?
|
||
} | ||
#if os(Linux) | ||
// On Linux some tests are fail. Need investigation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to add a TODO/issue/checklist in PR before merging
@@ -160,7 +160,7 @@ class OffchainLookupTests: XCTestCase { | |||
let tx = try! function.transaction() | |||
|
|||
do { | |||
let _ = try await client.eth_call(tx) | |||
let _ = try await client.eth_call(tx, resolution: .noOffchain(failOnExecutionError: true), block: .Latest) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think no need to change the test as it's the same as default
func addRequest(_ key: Int, request: WebSocketRequest) { | ||
semaphore.wait() | ||
requestQueue[key] = request | ||
semaphore.signal() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less repetition using a private function like:
func exclusiveAccess(_ access: @autoclosure () -> Void) {
semaphore.wait()
access()
semaphore.signal()
}
func addRequest(_ key: Int, request: WebSocketRequest) {
exclusiveAccess(requestQueue[key] = request)
}
func net_version(completion: @escaping((EthereumClientError?, EthereumNetwork?) -> Void)) | ||
func eth_gasPrice(completion: @escaping((EthereumClientError?, BigUInt?) -> Void)) | ||
func eth_blockNumber(completion: @escaping((EthereumClientError?, Int?) -> Void)) | ||
func eth_getBalance(address: EthereumAddress, block: EthereumBlock, completion: @escaping((EthereumClientError?, BigUInt?) -> Void)) | ||
func eth_getCode(address: EthereumAddress, block: EthereumBlock, completion: @escaping((EthereumClientError?, String?) -> Void)) | ||
func eth_estimateGas(_ transaction: EthereumTransaction, completion: @escaping((EthereumClientError?, BigUInt?) -> Void)) | ||
func eth_sendRawTransaction(_ transaction: EthereumTransaction, withAccount account: EthereumAccountProtocol, completion: @escaping((EthereumClientError?, String?) -> Void)) | ||
func eth_getTransactionCount(address: EthereumAddress, block: EthereumBlock, completion: @escaping((EthereumClientError?, Int?) -> Void)) | ||
func eth_getTransaction(byHash txHash: String, completion: @escaping((EthereumClientError?, EthereumTransaction?) -> Void)) | ||
func eth_getTransactionReceipt(txHash: String, completion: @escaping((EthereumClientError?, EthereumTransactionReceipt?) -> Void)) | ||
func eth_call( | ||
_ transaction: EthereumTransaction, | ||
resolution: CallResolution, | ||
block: EthereumBlock, | ||
completion: @escaping((EthereumClientError?, String?) -> Void) | ||
) | ||
func eth_getLogs(addresses: [EthereumAddress]?, topics: [String?]?, fromBlock: EthereumBlock, toBlock: EthereumBlock, completion: @escaping((EthereumClientError?, [EthereumLog]?) -> Void)) | ||
func eth_getLogs(addresses: [EthereumAddress]?, orTopics: [[String]?]?, fromBlock: EthereumBlock, toBlock: EthereumBlock, completion: @escaping((EthereumClientError?, [EthereumLog]?) -> Void)) | ||
func eth_getBlockByNumber(_ block: EthereumBlock, completion: @escaping((EthereumClientError?, EthereumBlockInfo?) -> Void)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for this change it's ok to just remove these APIs as we have the result-based ones; to avoid duplicating code in WSSClient
// MARK: - Async/Await | ||
extension EthereumWebSocketClient { | ||
public func net_version() async throws -> EthereumNetwork { | ||
return try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<EthereumNetwork, Error>) in | ||
net_version(completionHandler: continuation.resume) | ||
} | ||
} | ||
|
||
public func eth_gasPrice() async throws -> BigUInt { | ||
return try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<BigUInt, Error>) in | ||
eth_gasPrice(completionHandler: continuation.resume) | ||
} | ||
} | ||
|
||
public func eth_blockNumber() async throws -> Int { | ||
return try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<Int, Error>) in | ||
eth_blockNumber(completionHandler: continuation.resume) | ||
} | ||
} | ||
|
||
public func eth_getBalance(address: EthereumAddress, block: EthereumBlock) async throws -> BigUInt { | ||
return try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<BigUInt, Error>) in | ||
eth_getBalance(address: address, block: block, completionHandler: continuation.resume) | ||
} | ||
} | ||
|
||
public func eth_getCode(address: EthereumAddress, block: EthereumBlock = .Latest) async throws -> String { | ||
return try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<String, Error>) in | ||
eth_getCode(address: address, block: block, completionHandler: continuation.resume) | ||
} | ||
} | ||
|
||
public func eth_estimateGas(_ transaction: EthereumTransaction) async throws -> BigUInt { | ||
return try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<BigUInt, Error>) in | ||
eth_estimateGas(transaction, completionHandler: continuation.resume) | ||
} | ||
} | ||
|
||
public func eth_sendRawTransaction(_ transaction: EthereumTransaction, withAccount account: EthereumAccountProtocol) async throws -> String { | ||
return try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<String, Error>) in | ||
eth_sendRawTransaction(transaction, withAccount: account, completionHandler: continuation.resume) | ||
} | ||
} | ||
|
||
public func eth_getTransactionCount(address: EthereumAddress, block: EthereumBlock) async throws -> Int { | ||
return try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<Int, Error>) in | ||
eth_getTransactionCount(address: address, block: block, completionHandler: continuation.resume) | ||
} | ||
} | ||
|
||
public func eth_getTransaction(byHash txHash: String) async throws -> EthereumTransaction { | ||
return try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<EthereumTransaction, Error>) in | ||
eth_getTransaction(byHash: txHash, completionHandler: continuation.resume) | ||
} | ||
} | ||
|
||
public func eth_getTransactionReceipt(txHash: String) async throws -> EthereumTransactionReceipt { | ||
return try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<EthereumTransactionReceipt, Error>) in | ||
eth_getTransactionReceipt(txHash: txHash, completionHandler: continuation.resume) | ||
} | ||
} | ||
|
||
public func eth_getLogs(addresses: [EthereumAddress]?, topics: [String?]?, fromBlock from: EthereumBlock = .Earliest, toBlock to: EthereumBlock = .Latest) async throws -> [EthereumLog] { | ||
return try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<[EthereumLog], Error>) in | ||
eth_getLogs(addresses: addresses, topics: topics, fromBlock: from, toBlock: to, completionHandler: continuation.resume) | ||
} | ||
} | ||
|
||
public func eth_getLogs(addresses: [EthereumAddress]?, orTopics topics: [[String]?]?, fromBlock from: EthereumBlock = .Earliest, toBlock to: EthereumBlock = .Latest) async throws -> [EthereumLog] { | ||
return try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<[EthereumLog], Error>) in | ||
eth_getLogs(addresses: addresses, orTopics: topics, fromBlock: from, toBlock: to, completionHandler: continuation.resume) | ||
} | ||
} | ||
|
||
public func eth_getBlockByNumber(_ block: EthereumBlock) async throws -> EthereumBlockInfo { | ||
return try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<EthereumBlockInfo, Error>) in | ||
eth_getBlockByNumber(block, completionHandler: continuation.resume) | ||
} | ||
} | ||
|
||
public func eth_call(_ transaction: EthereumTransaction, block: EthereumBlock = .Latest) async throws -> String { | ||
return try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<String, Error>) in | ||
eth_call(transaction, block: block, completionHandler: continuation.resume) | ||
} | ||
} | ||
|
||
public func eth_call(_ transaction: EthereumTransaction, resolution: CallResolution = .noOffchain(failOnExecutionError: true), block: EthereumBlock = .Latest) async throws -> String { | ||
return try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<String, Error>) in | ||
eth_call(transaction, resolution: resolution, block: block, completionHandler: continuation.resume) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of duplicated code given we reimplement with only the request transport side being different. Maybe reworking the WSS to be the transport layer instead of a full client implementation could work better? It would require lot more changes though
Add to ability to use an elg or create a new Remove request after calling its callback Finalize WebSocket EthereumClientProtocol methods - Add WebSocket tests Optimizations / additional features: - WebSocket reconnect functionallity - Remove pending requests on socket close - Disable testSimpleEthGetLogs till fixed - Remove pending request after failure - Rename request to requestQueue - Add SwiftLogger Add Queue’s Fix promise failure handling Add subscriptions logic, optimizations Add newPendingTransaction, newBlockHeader, syncing subscriptions Clear subscriptions on reconnection, disable failing ws tests Add input property on EthereumTransaction Formatting, optimizatios Add eth_getLogs errorHandling Fix EthereumSync decoding Re-enable testSimpleEthGetLogs test, add webSocketConfig for tests with higher maxFrameSize to resolve `message too large ws error` Log close code for debug Add WebSocketClient tests Disable some tests on linux to find the reason that are failing Make use of Logger Rebase, fix conflicts Reciew fixes, rebase Add ThreadSafety to shared resources Remove deprecated methods
Refactored the whole thing. Closing that PR and will continue to #239 |
Started the PR to take feedback from you about the current implementation.
Solves #182 - Related #221
I make use https://github.com/vapor/websocket-kit which is built on top of
swift-nio
and is compatible with Linux tooThe
EthereumWebSocketClient
comforts to theEthereumClientProtocol
likeEthereumClient
!Most of the
EthereumClientProtocol
methods are ready...TODO:
WebSocketClient TestsSubscriptionsTimeout logicRequests/Responses QueueWebSocket reconnect functionality