Support batching multiple RPC requests #187

franciszekjob · 2024-04-25T08:43:28Z

Describe your changes

Suport batching multiple RPC requests

add StarknetRequest and StarknetBatchRequest classes
add in StarknetProvider.batchRequests()

Linked issues

Closes #186

Breaking changes

This issue contains breaking changes

StarknetAccount
- executeV1(...) async throws -> StarknetInvokeTransactionResponse -> executeV1(...) async throws -> StarknetRequest<StarknetInvokeTransactionResponse, AddInvokeTransactionParams>
- executeV3(...) async throws -> StarknetInvokeTransactionResponse -> executeV3(...) async throws -> StarknetRequest<StarknetInvokeTransactionResponse, AddInvokeTransactionParams>
- executeV1(...) async throws -> StarknetInvokeTransactionResponse -> executeV1(...) async throws -> StarknetRequest<StarknetInvokeTransactionResponse, AddInvokeTransactionParams>
- executeV3(...) async throws -> StarknetInvokeTransactionResponse -> executeV3(...) async throws -> StarknetRequest<StarknetInvokeTransactionResponse, AddInvokeTransactionParams>
- estimateFeeV1(...) async throws -> StarknetFeeEstimate -> estimateFeeV1(...) async throws -> StarknetRequest<[StarknetFeeEstimate], EstimateFeeParams>
- estimateFeeV3(...) async throws -> StarknetFeeEstimate -> estimateFeeV3(...) async throws -> StarknetRequest<[StarknetFeeEstimate], EstimateFeeParams>
- estimateDeployAccountFeeV1(...) async throws -> StarknetFeeEstimate -> estimateDeployAccountFeeV1(...) async throws -> StarknetRequest<[StarknetFeeEstimate], EstimateFeeParams>
- estimateDeployAccountFeeV3(...) async throws -> StarknetFeeEstimate -> estimateDeployAccountFeeV3(...) async throws -> StarknetRequest<[StarknetFeeEstimate], EstimateFeeParams>
- getNonce() async throws -> Felt -> getNonce() async throws -> StarknetRequest<Felt, GetNonceParams>

…wift into feat/186-batch-multiple-rpc-requests

franciszekjob · 2024-05-10T13:31:15Z

Tests/StarknetTests/Providers/ProviderTests.swift


        XCTAssertEqual(simulations2.count, 2)
        XCTAssertTrue(simulations2[0].transactionTrace is StarknetInvokeTransactionTrace)
        XCTAssertTrue(simulations2[1].transactionTrace is StarknetDeployAccountTransactionTrace)
    }
+
+    func testBatchGetTransactionByHash() async throws {


Do we need to include more calls in this test?

There should be a test for incorrect responses order. Imho it should be placed in a separate class eg. JsonRpcResponseTests as it focuses on the responses parsing mechanism.

Wdyt? @DelevoXDG

I think that can be enough

Yes, should add such test to JsonRpcResponseTests

I've added test for incorrect responses order
Also getOrderedRpcResults() is now a separate internal function. It doesn't affect StarknetBatchRequest, but simplifies creating test for incorrect order.

…wift into feat/186-batch-multiple-rpc-requests

THenry14 · 2024-05-14T12:46:53Z

Sources/Starknet/Accounts/StarknetAccount.swift

    }

    public func estimateFeeV1(calls: [StarknetCall], nonce: Felt, skipValidate: Bool) async throws -> StarknetFeeEstimate {
        let params = StarknetInvokeParamsV1(nonce: nonce, maxFee: .zero)
        let signedTransaction = try signV1(calls: calls, params: params, forFeeEstimation: true)

-        return try await provider.estimateFee(for: signedTransaction, simulationFlags: skipValidate ? [.skipValidate] : [])
+        return try await provider.estimateFee(for: signedTransaction, simulationFlags: skipValidate ? [.skipValidate] : []).send()[0]


should be return [0] here?

Yes, please look at the old implementation of calling estimateFee. We could directly access value at index 0, because the returned type here was [StarknetFeeEstimate]. Now it's Request<[StarknetFeeEstimate], EstimateFeeParams>, therefore first we need to send and then access [0] element.

THenry14 · 2024-05-14T12:51:08Z

Sources/Starknet/Network/BatchRequest.swift

+        var orderedRpcResults: [Result<U, StarknetProviderError>?] = Array(repeating: nil, count: rpcPayloads.count)
+        for rpcResponse in rpcResponses {
+            if let error = rpcResponse.error {
+                orderedRpcResults[rpcResponse.id] = .failure(StarknetProviderError.jsonRpcError(error.code, error.message, error.data))
+            } else if let result = rpcResponse.result {
+                orderedRpcResults[rpcResponse.id] = .success(result)
+            } else {
+                orderedRpcResults[rpcResponse.id] = .failure(StarknetProviderError.unknownError)
+            }
+        }


this seems like it could be really slow for longer response lists. I suppose we need ordering to properly deserialize? is there any other way?

We need to reorder the responses to match the order in which they were sent. This can be achieved with sorting O(nlogn) or direct placement O(n). Second solution was accepted and implemented in JVM SDK, where there is same situation, here.

THenry14 · 2024-05-14T12:53:01Z

Sources/Starknet/Network/HttpNetworkProvider.swift

+        let result = try await sendBatch(payload: [payload], config: config, receive: [U.self])
+        guard let firstResult = result.first else {
+            throw HttpNetworkProviderError.unknownError
+        }
+        return firstResult


why cannot we just send single request, why does it have to be sent as a batch?

Actually I wanted to reuse sendBatch to reduce duplicated code. However, sending single request is more valid in the conceptual aspect. I've refactored it - now there are two send methods with different signatures. They send request with single json payload and array of jsons as payload respectively.

I've also added private methods for error handling to reduce duplicated code.

…rovider`

…wift into feat/186-batch-multiple-rpc-requests

.gitignore

DelevoXDG · 2024-05-15T16:29:27Z

Sources/Starknet/Accounts/StarknetAccount.swift

@@ -106,7 +106,7 @@ public class StarknetAccount: StarknetAccountProtocol {
        let params = StarknetInvokeParamsV1(nonce: nonce, maxFee: maxFee)
        let signedTransaction = try signV1(calls: calls, params: params, forFeeEstimation: false)

-        return try await provider.addInvokeTransaction(signedTransaction)
+        return try await provider.addInvokeTransaction(signedTransaction).send()


Why not return Request in StarknetAccount methods as well?

Seems to me we might wanna batch multiple requests created by an account.

Correct, they also use provider methods. I've updated StarknetAccount methods to return Request.

DelevoXDG · 2024-05-15T16:33:11Z

Sources/Starknet/Data/Transaction/TransactionReceiptWrapper.swift

@@ -1,6 +1,6 @@
 import Foundation

-enum TransactionReceiptWrapper: Decodable {
+public enum TransactionReceiptWrapper: Decodable {


Is this change necessary?

Yes, TransactionReceiptWrapper is public because it's used in getTransactionReceiptBy()

Sources/Starknet/Network/BatchRequest.swift

DelevoXDG · 2024-05-15T17:24:22Z

Sources/Starknet/Providers/StarknetProvider/StarknetProvider.swift

+        let rpcPayloads = getBatchRequestRpcPayloads(requests: requests)
+        let config = getHttpConfiguration()


If we refactor batchRequests<U: Decodable, P: Encodable>(requests: Request<U, P>...) to just call batchRequests<U: Decodable, P: Encodable>(requests: [Request<U, P>]), we can get rid of both getBatchRequestRpcPayloads and getHttpConfiguration and inline their contents there.

I inlined content of getBatchRequestRpcPayloads there, but had to keep getHttpConfiguration, which is also used in buildRequest.

Sources/Starknet/Providers/StarknetProviderProtocol.swift

DelevoXDG · 2024-05-15T17:40:18Z

Tests/StarknetTests/Providers/ProviderTests.swift

@@ -5,7 +5,7 @@ import XCTest
 final class ProviderTests: XCTestCase {
    static var devnetClient: DevnetClientProtocol!

-    var provider: StarknetProviderProtocol!
+    var provider: StarknetProvider!


Nit:

Maybe we should just add separate provider instance / cast for batch tests?

I don't have a strong opinion on this though. But the idea is to verify provider as a protocol can be used with no issues

Sure, I restored var provider: StarknetProviderProtocol!, added separate batchProvider and used it in test for batchRequests.

DelevoXDG · 2024-05-15T18:04:13Z

Tests/StarknetTests/Providers/ProviderTests.swift

+        do {
+            let _ = try transactionsResponse[1].get().transaction.hash
+            XCTFail("Fetching transaction with nonexistent hash should fail")
+        } catch {}


We can catch an error and verify if it's correct one

Sure, I've added check for error type and error message 👍

DelevoXDG · 2024-05-15T18:04:49Z

Tests/StarknetTests/Providers/ProviderTests.swift


        XCTAssertEqual(simulations2.count, 2)
        XCTAssertTrue(simulations2[0].transactionTrace is StarknetInvokeTransactionTrace)
        XCTAssertTrue(simulations2[1].transactionTrace is StarknetDeployAccountTransactionTrace)
    }
+
+    func testBatchGetTransactionByHash() async throws {


I think that can be enough

Yes, should add such test to JsonRpcResponseTests

Co-authored-by: Maksim Zdobnikau <43750648+DelevoXDG@users.noreply.github.com>

….com/software-mansion/starknet.swift into feat/186-batch-multiple-rpc-requests

…tion`

franciszekjob · 2024-05-16T14:20:30Z

Tests/StarknetTests/Accounts/AccountTest.swift

-        let nonce = try await account.getNonce()
-        let feeEstimate = try await account.estimateFeeV1(call: call, nonce: nonce)
+        let nonce = try await account.getNonce().send()
+        let feeEstimate = try await account.estimateFeeV1(call: call, nonce: nonce).send()[0]


The change of estimate fee return type resulted in having to access element [0] every time we use this metod. Unfortunately there isn't any other way, because first we need to send the request and the access its result.

franciszekjob · 2024-05-16T14:21:41Z

Tests/StarknetTests/Providers/ProviderTests.swift

@@ -30,16 +31,17 @@ final class ProviderTests: XCTestCase {
        }

        provider = makeStarknetProvider(url: Self.devnetClient.rpcUrl)
+        batchProvider = makeStarknetProvider(url: Self.devnetClient.rpcUrl)


Nit: could the name variable name be improved?

franciszekjob · 2024-05-16T14:24:42Z

Tests/StarknetTests/Providers/ProviderTests.swift

+        do {
+            let _ = try transactionsResponse[1].get().transaction.hash
+            XCTFail("Fetching transaction with nonexistent hash should fail")
+        } catch let error as StarknetProviderError {
+            switch error {
+            case let .jsonRpcError(_, message, _):
+                XCTAssertEqual(message, "Transaction hash not found", "Unexpected error message received")
+            default:
+                XCTFail("Expected JsonRpcError but received \(error)")
+            }
+        } catch {
+            XCTFail("Error was not a StarknetProviderError. Received error type: \(type(of: error))")
+        }


I don't really like the fact that there is double nesting here, but I don't any other way to access jsonRpcError message.

franciszekjob · 2024-05-16T14:26:11Z

All requested changes have been addressed, I've also added few remarks about newly applied changes.

franciszekjob added 20 commits April 25, 2024 04:06

Add possibility ofid modification in JsonRpcPayload

d020773

Merge branch 'main' of https://github.com/software-mansion/starknet.s…

9f82f76

…wift into feat/186-batch-multiple-rpc-requests

Refactor sending rpc request for specVersion

b598af7

Format

b6b9a40

Add generic type of params in HttpRequest and buildRequest

3801260

Refactor method signatures to return HttpRequest

a1d84d2

Remove makeRequest and update methods to use buildRequest

b53eef0

Change access levels of request param types

5be5ad4

Update usage of StarknetProvider endpoints in tests

2af15e7

Format

53879fc

MoveHttpRequest. Add HttpBatchRequest and Request

0adb281

Add batchRequests method

f9df81b

Add testBatchGetTransactionByHash test

b00296b

Update testBatchGetTransactionByHash

a2739d2

Rename request files

f681f58

Remove batchRequests from StarknetProviderProtocol

00a123f

Order responses ordering

d0b66f0

Add batchRequests with variadic param requests

bfb3b25

Fix wrong variable name

871aefc

Update testBatchGetTransactionByHash test

1a74d7b

franciszekjob commented May 10, 2024

View reviewed changes

franciszekjob requested a review from DelevoXDG May 10, 2024 13:53

franciszekjob added 8 commits May 10, 2024 16:26

Merge branch 'main' of https://github.com/software-mansion/starknet.s…

33071cf

…wift into feat/186-batch-multiple-rpc-requests

Wrap values returned from batchRequests in Result

2fd5b3b

Add different transactions to testBatchGetTransactionByHash

1beafcb

Format

697d340

Add incrementing id in json rpc requests

af2b884

Refactor batchRequests

56f9916

Use variadic params in testBatchGetTransactionByHash

c6554a4

Refactor send in BatchRequest

917e8c9

Remove mistakenly copied BatchRequest class

ebb791b

franciszekjob requested a review from THenry14 May 13, 2024 09:58

THenry14 reviewed May 14, 2024

View reviewed changes

franciszekjob added 3 commits May 15, 2024 11:14

Refactor send and sendbatch methods in HttpNetworkProvider

05f7cdd

Add methods for encoding and response error handling to `HttpNetworkP…

53dd8d2

…rovider`

Change handleEncodeError to handleEncodingError

a0de208

franciszekjob requested a review from THenry14 May 15, 2024 10:15

franciszekjob marked this pull request as ready for review May 15, 2024 11:13

Merge branch 'main' of https://github.com/software-mansion/starknet.s…

199657b

…wift into feat/186-batch-multiple-rpc-requests

DelevoXDG reviewed May 15, 2024

View reviewed changes

franciszekjob and others added 10 commits May 15, 2024 23:10

Update .gitignore

fdee240

Co-authored-by: Maksim Zdobnikau <43750648+DelevoXDG@users.noreply.github.com>

Change return type of StarknetAccount methods to Request

38c1773

Merge branch 'feat/186-batch-multiple-rpc-requests' of https://github…

4d77998

….com/software-mansion/starknet.swift into feat/186-batch-multiple-rpc-requests

Add Starknet prefix to Request and BatchRequest

6811c24

Restore let id in JsonRpcPayload

55cc253

Verify error in testBatchGetTransactionByHash

a9c3113

Add test for json rpc responses with incorrect order

85d70fc

Remove redundant constructors from request structs

21bdc4b

Add StarknetProvider extension; Move there `getHttpRequestConfigura…

0ac08b1

…tion`

Add separate provider for batching request tests

d96c19f

franciszekjob commented May 16, 2024

View reviewed changes

franciszekjob requested a review from DelevoXDG May 16, 2024 14:26

Update doc comments

3e261bc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support batching multiple RPC requests #187

Support batching multiple RPC requests #187

franciszekjob commented Apr 25, 2024 •

edited

franciszekjob May 10, 2024 •

edited

DelevoXDG May 15, 2024

franciszekjob May 16, 2024 •

edited

THenry14 May 14, 2024

franciszekjob May 14, 2024 •

edited

THenry14 May 14, 2024

franciszekjob May 14, 2024

THenry14 May 14, 2024

franciszekjob May 15, 2024 •

edited

DelevoXDG May 15, 2024

franciszekjob May 16, 2024

DelevoXDG May 15, 2024

franciszekjob May 15, 2024 •

edited

DelevoXDG May 15, 2024

franciszekjob May 16, 2024

DelevoXDG May 15, 2024

franciszekjob May 16, 2024

DelevoXDG May 15, 2024

franciszekjob May 16, 2024 •

edited

DelevoXDG May 15, 2024

franciszekjob May 16, 2024

franciszekjob May 16, 2024

franciszekjob May 16, 2024 •

edited

franciszekjob commented May 16, 2024 •

edited

		let rpcPayloads = getBatchRequestRpcPayloads(requests: requests)
		let config = getHttpConfiguration()

Support batching multiple RPC requests #187

Are you sure you want to change the base?

Support batching multiple RPC requests #187

Conversation

franciszekjob commented Apr 25, 2024 • edited

Describe your changes

Linked issues

Breaking changes

franciszekjob May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franciszekjob May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franciszekjob May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franciszekjob May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franciszekjob May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franciszekjob May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franciszekjob May 16, 2024 • edited

Choose a reason for hiding this comment

franciszekjob commented May 16, 2024 • edited

franciszekjob commented Apr 25, 2024 •

edited

franciszekjob May 10, 2024 •

edited

franciszekjob May 16, 2024 •

edited

franciszekjob May 14, 2024 •

edited

franciszekjob May 15, 2024 •

edited

franciszekjob May 15, 2024 •

edited

franciszekjob May 16, 2024 •

edited

franciszekjob May 16, 2024 •

edited

franciszekjob commented May 16, 2024 •

edited