Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: implement JsonRpc transport for batch requests #600

Closed
wants to merge 1 commit into from
Closed

feat: implement JsonRpc transport for batch requests #600

wants to merge 1 commit into from

Conversation

thetheveloper
Copy link
Contributor

Introduces JsonRpc batching requests described in #593

@bartfish bartfish force-pushed the feat/extend-jsonrpc-transport-for-batch-requests branch from e3a9eec to 9e898e8 Compare June 15, 2024 12:31
@thetheveloper thetheveloper marked this pull request as ready for review June 15, 2024 12:31
tcoratger
tcoratger reviewed Jun 17, 2024
View reviewed changes
starknet-providers/src/any.rs Outdated Show resolved Hide resolved
starknet-providers/src/any.rs Outdated Show resolved Hide resolved
starknet-providers/src/any.rs Outdated Show resolved Hide resolved
starknet-providers/src/any.rs Outdated Show resolved Hide resolved
starknet-providers/src/any.rs Outdated Show resolved Hide resolved
starknet-providers/src/jsonrpc/mod.rs Outdated Show resolved Hide resolved
starknet-providers/src/jsonrpc/transports/http.rs Outdated Show resolved Hide resolved
starknet-providers/src/jsonrpc/transports/http.rs Show resolved Hide resolved
starknet-providers/src/jsonrpc/mod.rs Outdated Show resolved Hide resolved
examples/jsonrpc_batch.rs Outdated Show resolved Hide resolved
@thetheveloper
Copy link
Contributor Author

thetheveloper commented Jun 19, 2024 •
edited
Loading

Thank you @tcoratger for the review. All review comments addressed in the latest commit.

@thetheveloper thetheveloper requested a review from tcoratger June 19, 2024 22:57
xJonathanLEI
xJonathanLEI reviewed Jun 19, 2024
View reviewed changes
starknet-providers/src/provider.rs
async fn batch_requests<I, P>(
&self,
requests: I,
) -> Result<Vec<serde_json::Value>, ProviderError>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have to resort to serde_json::Value here? I generally dislike having that as part of the interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. Not necessarily, however this function was specifically introduced for this approach - to return generic JSON type based on the type returned. Otherwise, we'd have to create custom types for this and that would limit the ways we can call it

It's possible to change it if you prefer any other direction

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adrienlacombe For me this PR is ready to be merged but I think this discussion still is opened

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ty @tcoratger , @xJonathanLEI can you please let us know your thoughts? ty

@greged93 greged93 mentioned this pull request Jun 28, 2024
Closed
@tcoratger
Copy link
Collaborator

@thetheveloper can you rebase to trigger the ci again? I think this should be good now

@bartfish bartfish force-pushed the feat/extend-jsonrpc-transport-for-batch-requests branch from 1e3a98f to f670190 Compare July 16, 2024 21:37
@thetheveloper
Copy link
Contributor Author

thetheveloper commented Jul 16, 2024 •
edited
Loading

@thetheveloper can you rebase to trigger the ci again? I think this should be good now

@tcoratger rebase done. I tried to include the test fixes (hence last commit), but from what I see, when the tests are not running one after another then everything is fine. I suppose RPC node needs to process transactions from the previous iteration of testing, so the next one doesn't fail

@thetheveloper
Copy link
Contributor Author

Should be fine now. Latest base is in place @tcoratger

@adrienlacombe
Copy link

@tcoratger @xJonathanLEI anything else needed? thanks!

@tcoratger
Copy link
Collaborator

@tcoratger @xJonathanLEI anything else needed? thanks!

For me this is ok to merge but as the PR is quite big and important I would appreciate another review from @xJonathanLEI if possible?

@bartfish bartfish force-pushed the feat/extend-jsonrpc-transport-for-batch-requests branch from 5f0b3f1 to ec92d77 Compare August 3, 2024 10:42
@xJonathanLEI xJonathanLEI force-pushed the feat/extend-jsonrpc-transport-for-batch-requests branch from ec92d77 to 3e084a5 Compare August 27, 2024 08:54
@xJonathanLEI
Copy link
Owner

Squashed and rebased.

@adrienlacombe
Copy link

gm @xJonathanLEI is this good to be merged? ty

@adrienlacombe
Copy link

@tcoratger maybe you can review as well? ty ty

@xJonathanLEI
Copy link
Owner

While this implementation probably works, I'm really not a fan of adding serde_json::Value to a supposedly neutral trait of Provider. I mean it's probably acceptable to add it to JsonRpcTransport, because as its name suggests, it deals with JSON.

I know this is a bit not very pragmatic - after all, Provider is modelled after the JSON-RPC standard. However, with the current design any Provider user would now have to deal with deserialization... which is far from ideal.

Anyway, I just prefer a design where we don't have to pollute Provider with JSON stuff.

The diffs are also a bit all over the place:

  • Why do we have a get_block_with_tx_hashes_batch method? Wouldn't that be covered by send_requests?
  • Same goes for JsonRpcRequestParams - this type only covers 2 methods.

Closing this as I'll work on an alternative version. Thanks for working on this.

@xJonathanLEI xJonathanLEI mentioned this pull request Sep 10, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrienlacombe adrienlacombe adrienlacombe left review comments

@tcoratger tcoratger Awaiting requested review from tcoratger

@xJonathanLEI xJonathanLEI Awaiting requested review from xJonathanLEI

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@thetheveloper @tcoratger @adrienlacombe @xJonathanLEI