-
Notifications
You must be signed in to change notification settings - Fork 114
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
Support "pending" option for Web3 requests #713
Conversation
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.
Hey looks pretty sane overall ... as we talked about, wasn't practical to add any meaningful testing due to the way the code is layed out. Could you set up a jira task about making these class(es) more testable?
@@ -2872,6 +2838,14 @@ private IRepository getRepoByJsonBlockId(String _bnOrId) { | |||
return ac.getRepository().getSnapshotTo(b.getStateRoot()); | |||
} | |||
|
|||
private AionBlock getBlockByBN(Long bn) { |
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 long is better than Long here (probably doesn't make sense to pass in null)
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.
Done
@@ -2872,6 +2838,14 @@ private IRepository getRepoByJsonBlockId(String _bnOrId) { | |||
return ac.getRepository().getSnapshotTo(b.getStateRoot()); | |||
} | |||
|
|||
private AionBlock getBlockByBN(Long bn) { | |||
if (bn < 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.
Should this not be bn == -1
instead?
(And if so, I think it'd be good to put -1 in a const like 'BEST_PENDING_BLOCK' or something to that effect)
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.
Done
return null; | ||
} | ||
|
||
if (bn < 0) return pendingState.getRepository(); |
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.
Should it not be bn == -1?
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.
Done
ade1447
to
651fab7
Compare
All suggested changes have been implemented :) |
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.
looks good thanks for the changes!
@@ -1100,6 +1102,11 @@ private FltrLg createFilter(ArgFltr rf) { | |||
return null; | |||
} | |||
|
|||
if (bnTo != BEST_PENDING_BLOCK && (bnFrom > bnTo)) { |
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.
nit: unnecessary parens
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.
KILLED them :)
651fab7
to
6f90eed
Compare
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.
LGTM, thanks for the PR
Notice
It is not allowed to submit your PR to the master branch directly, please submit your PR to the master-pre-merge branch.
Description
Adds the "pending" option for the missing functions, thus fixing issue #709.
Type of change
Insert x into the following checkboxes to confirm (eg. [x]):
Testing
All existing tests pass, new tests added.
Verification
Insert x into the following checkboxes to confirm (eg. [x]):