-
Notifications
You must be signed in to change notification settings - Fork 96
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
base: main
Are you sure you want to change the base?
Rpc multiquery #93
Conversation
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; |
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 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
lib/src/core/amount.dart
Outdated
@@ -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) { |
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.
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) { |
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.
this sorting implementation could be improved but didnt want to make the code less understandable
lib/web3dart.dart
Outdated
@@ -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'; |
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 added a direct export from the main library file, correct me if you think any other way to export this custom implementation is better
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.
Thank you @juampiq6. I appreciate your efforts. Please let's review and improve the code together.
lib/json_rpc_multiquery.dart
Outdated
@@ -0,0 +1,80 @@ | |||
// ignore_for_file: sort_constructors_first |
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.
Please remove sort_constructors_first
and sort the constructors first.
lib/json_rpc_multiquery.dart
Outdated
@@ -0,0 +1,80 @@ | |||
// ignore_for_file: sort_constructors_first | |||
|
|||
library json_rpc_multiquery; |
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.
please use web3dart
for the library name.
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.
Forget my previous comment, I believe this should a part
.
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.
so.. i should put part web3dart
lib/src/core/amount.dart
Outdated
@@ -45,6 +47,12 @@ class EtherAmount { | |||
return EtherAmount.inWei(parsedAmount * _factors[unit]!); | |||
} | |||
|
|||
/// Constructs an amount of Ether in wei from an hex String |
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.
A unit is essential for this method.
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.
sure, will add it
@@ -0,0 +1,267 @@ | |||
library eth_rpc_query; |
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.
Another library name?
I believe this should a a part
.
lib/src/core/multiquery_client.dart
Outdated
/// 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( |
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.
method name should be camelCase.
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.
Why not adding this to the original Web3Client class?
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.
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
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 agree with docs :)
@xclud all fixes done, i dont know what could be missing |
Thank You @juampiq6, There are a couple of merge conflicts. |
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.