-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
test: TransactionsProvider
implementation of BlockchainProvider2<DB>
#10594
Conversation
TransactionsProvider
implementation of BlockchainProvider2<DB>
TransactionsProvider
implementation of BlockchainProvider2<DB>
tx_count: if tx_count.end == tx_count.start { | ||
tx_count.start..tx_count.start + 1 | ||
} else { | ||
tx_count | ||
}, |
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: you can accept impl RangeBounds<u8>
, and convert into Range
here, so you can explicitly pass TEST_TRANSACTIONS_COUNT..=TEST_TRANSACTIONS_COUNT
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'm not sure if this solves the problem... Indeed this if/else is there for the case where BlockRangeParams
is default (for about half of the tests) and so the range is 0..0 which is not accepted by the random_block_range
method which requires at least 0..1
(which was hardcoded before). That's why I did this trick, I think it's enough right?
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.
if you pass 0..=0
to this method, it should get converted to 0..1
which is basically the same as it was before, but more explicit
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.
Just fixed, let me know if this is what you had in mind here
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.
One last nit, otherwise LGTM! Thank you for going through all these comments 😅
No problem, always a pleasure to learn and discuss some tricks :) |
Should close #10339