add range test in FilterBlockOption
#939
Conversation
tcoratger
requested review from
DaniPopes,
gakonst,
mattsse,
onbjerg,
prestwich and
Evalir
as code owners
crates/rpc-types-eth/src/filter.rs
Outdated
| // Check if from_block is greater than to_block | ||
| if self | ||
| .get_to_block() | ||
| .and_then(|to| to.as_number()) | ||
| .zip(block.as_number()) | ||
| .map_or(false, |(to, from)| from > to) | ||
| { | ||
| panic!("from_block cannot be greater than to_block"); | ||
| } |
There was a problem hiding this comment.
hmm, we should not panic here.
worst thing that can happen when sending an invalid range is an rpc error as response
There was a problem hiding this comment.
Yes true, didn't think about this properly
@mattsse I suggest something else, tell me what you think. I renamed the methods that existed before with an I created two new methods without the suffix for cases where the user wants there to be checking. I know that this breaks the previous API but the user will notice it because the return type is not the same. |
crates/rpc-types-eth/src/filter.rs
Outdated
| @@ -213,34 +222,44 @@ impl FilterBlockOption { | |||
| } | |||
|
|
|||
| /// Sets the block number this range filter should start at. | |||
| #[must_use] | |||
| pub fn with_from_block(&self, block: BlockNumberOrTag) -> Self { | |||
| pub fn with_from_block(&self, block: BlockNumberOrTag) -> Result<Self, FilterBlockError> { | |||
There was a problem hiding this comment.
this is very inconvenient, we should not do this.
I'm fine with an additional ensure_ function that does this check, but this should not be part of the builder functions because not useful
is there a particular use case that requires this check?
There was a problem hiding this comment.
Just fixed that, let me know :)
I tried, apparently you didn't like it haha
In fact, it was while testing our RPC that I realized that it seemed much too easy to produce erroneous block ranges and then spend several minutes before understanding what was wrong.
For example in our case we filter with the MongoDB Rust client and therefore obviously if I filter the blocks from 15 to 2 it doesn't work but I had to do a lot of prints before understanding that I was doing that :)
So I thought that an unchecked method (for the more confident) and a checked method (for the more cautious) could be a good idea. But let's do this with an ensure_ method that at least allows us to have the option of adding risk insurance.
* add range test in FilterBlockOption * fix comments * fix comments * fix comments
Summary
Currently there are no checks in the
with_from_blockandwith_to_blockmethods to ensure the validity of the given block numbers and thus avoid false ranges.This pull request adds unit tests for the
FilterBlockOptionstruct, specifically focusing on thewith_from_blockandwith_to_blockmethods. These tests ensure that the methods correctly handle various scenarios, including valid and invalid block ranges.Changes
FilterBlockOptionMethodswith_from_block: Added a check to ensure that thefrom_blockvalue is not greater than theto_blockvalue. If this condition is violated, the method will panic with an appropriate error message.with_to_block: Added a check to ensure that theto_blockvalue is not less than thefrom_blockvalue. If this condition is violated, the method will panic with an appropriate error message.Unit Tests
Unit tests are added to verify the behavior of the
with_from_blockandwith_to_blockmethods.