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.

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

Rpc multiquery #93

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Rpc multiquery #93

wants to merge 24 commits into from

Conversation

juampiq6
Copy link

Multiquery RPC feature:

This PR adds functionality for making several rpc queries in one request. Handling most of the stuff internally in a separate implementation of a Web3Client for compatibility purpose.
I can add documentation for it in the main README.md file for people who need it as it can be a useful feature for a lot of people to ask once and get several responses (reducing latency times specially for dapps that make a lot of request).
I think this is a key feature.
I added some integration test but a lot more can be added (i saw the other parts of this library werent thoughly tested so i want make sense to keep a good test suite only for this feature).
Hope this helps!

Any improvement is more than welcome as any corrections or discussion about the implementation!
@xclud i hope you have time to review it, I have marked in the PR comments the part where it does actually change a little the library API, but so far the rest and the main functionality is developed in other implementation of the Web3Client.

instead of throwing an RPCError, now an error is returned in the list of responses, to be handled by the receiver appropiately
created some really handy constructors to be able to create several queries easily
… them.

switched back rpc query id to be int instead of string, for single type managment purpose:
theorically an rpc id can be string too, but for simplicity all ids assigned will be int
…ber of responses

added response sorted by request order (not id order)

final int? id;
Copy link
Author

Choose a reason for hiding this comment

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

i added rpc id to the error response since we need to identifiy which request had the error. this is a must and doesnt bother with the original API

@@ -45,6 +47,12 @@ class EtherAmount {
return EtherAmount.inWei(parsedAmount * _factors[unit]!);
}

/// Constructs an amount of Ether in wei from an hex String
factory EtherAmount.fromHex(String hex) {
Copy link
Author

Choose a reason for hiding this comment

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

handy method to instantiate EtherAmount directly from a hex string


for (var k = 0; k > preparedQueries.keys.length; k++) {
final sameIdResponse = decodedResponses.firstWhere((dynamic r) {
if (r is RPCError) {
Copy link
Author

Choose a reason for hiding this comment

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

this sorting implementation could be improved but didnt want to make the code less understandable

@@ -28,7 +30,9 @@ export 'src/core/amount.dart';
export 'src/core/block_information.dart';
export 'src/core/block_number.dart';
export 'src/core/sync_information.dart';
export 'src/core/eth_rpc_query/eth_rpc_query.dart';
Copy link
Author

Choose a reason for hiding this comment

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

i added a direct export from the main library file, correct me if you think any other way to export this custom implementation is better

Copy link
Owner

@xclud xclud left a comment

Choose a reason for hiding this comment

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

Thank you @juampiq6. I appreciate your efforts. Please let's review and improve the code together.

@@ -0,0 +1,80 @@
// ignore_for_file: sort_constructors_first
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove sort_constructors_first and sort the constructors first.

@@ -0,0 +1,80 @@
// ignore_for_file: sort_constructors_first

library json_rpc_multiquery;
Copy link
Owner

Choose a reason for hiding this comment

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

please use web3dart for the library name.

Copy link
Owner

Choose a reason for hiding this comment

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

Forget my previous comment, I believe this should a part.

Copy link
Author

Choose a reason for hiding this comment

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

so.. i should put part web3dart

@@ -45,6 +47,12 @@ class EtherAmount {
return EtherAmount.inWei(parsedAmount * _factors[unit]!);
}

/// Constructs an amount of Ether in wei from an hex String
Copy link
Owner

Choose a reason for hiding this comment

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

A unit is essential for this method.

Copy link
Author

Choose a reason for hiding this comment

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

sure, will add it

@@ -0,0 +1,267 @@
library eth_rpc_query;
Copy link
Owner

Choose a reason for hiding this comment

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

Another library name?

I believe this should a a part.

/// one request to the eth client.
/// The resulting list of responses will a mix of [RPCError] instance/s and/or
/// [EthRPCQuery] instance/s with the returned value
Future<List<dynamic>> multiqueryCall(
Copy link
Owner

Choose a reason for hiding this comment

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

method name should be camelCase.

Copy link
Owner

Choose a reason for hiding this comment

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

Why not adding this to the original Web3Client class?

Copy link
Author

Choose a reason for hiding this comment

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

it was only meant for people who actually want to make a multiqueryCall.
Plus i didnt want to mess with the Web3Client class, this class only adds this functionality.
It can be done but its more code in one file, and it doesnt suit all the use cases for people using this lib.
though i believe documentation regarding the use of this class should be added

Copy link
Owner

Choose a reason for hiding this comment

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

I agree with docs :)

@juampiq6
Copy link
Author

juampiq6 commented Mar 2, 2023

@xclud all fixes done, i dont know what could be missing

@xclud
Copy link
Owner

xclud commented Mar 2, 2023

Thank You @juampiq6,

There are a couple of merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants