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.

Sign up for GitHub

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

add range test in FilterBlockOption #939

Merged
merged 4 commits into from
Jun 25, 2024
Merged

add range test in FilterBlockOption #939

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
87f0f79
add range test in FilterBlockOption
tcoratger Jun 18, 2024
201974d
fix comments
tcoratger Jun 18, 2024
6d202c1
fix comments
tcoratger Jun 18, 2024
4f56e5f
fix comments
tcoratger Jun 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 42 additions & 19 deletions crates/rpc-types-eth/src/filter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,15 @@ impl From<U256> for Topic {
}
}

/// Represents errors that can occur when setting block filters in `FilterBlockOption`.
#[derive(Debug, PartialEq, Eq)]
pub enum FilterBlockError {
tcoratger marked this conversation as resolved.
Show resolved Hide resolved
/// Error indicating that the `from_block` is greater than the `to_block`.
FromBlockGreaterThanToBlock,
/// Error indicating that the `to_block` is less than the `from_block`.
ToBlockLessThanFromBlock,
}

/// Represents the target range of blocks for the filter
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]
pub enum FilterBlockOption {
Expand Down Expand Up @@ -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> {
Copy link
Member

@mattsse mattsse Jun 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

// 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");
return Err(FilterBlockError::FromBlockGreaterThanToBlock);
}

Self::Range { from_block: Some(block), to_block: self.get_to_block().copied() }
Ok(Self::Range { from_block: Some(block), to_block: self.get_to_block().copied() })
}

/// Sets the block number this range filter should end at.
#[must_use]
pub fn with_to_block(&self, block: BlockNumberOrTag) -> Self {
pub fn with_to_block(&self, block: BlockNumberOrTag) -> Result<Self, FilterBlockError> {
// Check if to_block is less than from_block
if self
.get_from_block()
.and_then(|from| from.as_number())
.zip(block.as_number())
.map_or(false, |(from, to)| to < from)
{
panic!("to_block cannot be less than from_block");
return Err(FilterBlockError::ToBlockLessThanFromBlock);
}

Ok(Self::Range { from_block: self.get_from_block().copied(), to_block: Some(block) })
}

/// Sets the block number this range filter should start at.
#[must_use]
pub fn with_from_block_unchecked(&self, block: BlockNumberOrTag) -> Self {
Self::Range { from_block: Some(block), to_block: self.get_to_block().copied() }
}

/// Sets the block number this range filter should end at.
#[must_use]
pub fn with_to_block_unchecked(&self, block: BlockNumberOrTag) -> Self {
Self::Range { from_block: self.get_from_block().copied(), to_block: Some(block) }
}

Expand Down Expand Up @@ -394,14 +413,14 @@ impl Filter {
/// Sets the from block number
#[must_use]
pub fn from_block<T: Into<BlockNumberOrTag>>(mut self, block: T) -> Self {
self.block_option = self.block_option.with_from_block(block.into());
self.block_option = self.block_option.with_from_block_unchecked(block.into());
self
}

/// Sets the to block number
#[must_use]
pub fn to_block<T: Into<BlockNumberOrTag>>(mut self, block: T) -> Self {
self.block_option = self.block_option.with_to_block(block.into());
self.block_option = self.block_option.with_to_block_unchecked(block.into());
self
}

Expand Down Expand Up @@ -1181,7 +1200,7 @@ mod tests {
from_block: Some(BlockNumberOrTag::Number(1)),
to_block: Some(BlockNumberOrTag::Number(10)),
};
let updated = original.with_from_block(BlockNumberOrTag::Number(5));
let updated = original.with_from_block(BlockNumberOrTag::Number(5)).unwrap();
match updated {
FilterBlockOption::Range { from_block, to_block } => {
assert_eq!(from_block, Some(BlockNumberOrTag::Number(5)));
Expand All @@ -1192,14 +1211,16 @@ mod tests {
}

#[test]
#[should_panic(expected = "from_block cannot be greater than to_block")]
fn test_with_from_block_failure() {
// Test scenario where from_block is greater than to_block
let original = FilterBlockOption::Range {
from_block: Some(BlockNumberOrTag::Number(10)),
to_block: Some(BlockNumberOrTag::Number(5)),
};
let _ = original.with_from_block(BlockNumberOrTag::Number(6));
assert_eq!(
original.with_from_block(BlockNumberOrTag::Number(6)),
Err(FilterBlockError::FromBlockGreaterThanToBlock)
);
}

#[test]
Expand All @@ -1209,7 +1230,7 @@ mod tests {
from_block: Some(BlockNumberOrTag::Number(1)),
to_block: None,
};
let updated = original.with_from_block(BlockNumberOrTag::Number(5));
let updated = original.with_from_block(BlockNumberOrTag::Number(5)).unwrap();
match updated {
FilterBlockOption::Range { from_block, to_block } => {
assert_eq!(from_block, Some(BlockNumberOrTag::Number(5)));
Expand All @@ -1226,7 +1247,7 @@ mod tests {
from_block: Some(BlockNumberOrTag::Earliest),
to_block: Some(BlockNumberOrTag::Latest),
};
let updated = original.with_from_block(BlockNumberOrTag::Pending);
let updated = original.with_from_block(BlockNumberOrTag::Pending).unwrap();
match updated {
FilterBlockOption::Range { from_block, to_block } => {
assert_eq!(from_block, Some(BlockNumberOrTag::Pending));
Expand All @@ -1243,7 +1264,7 @@ mod tests {
from_block: Some(BlockNumberOrTag::Number(1)),
to_block: Some(BlockNumberOrTag::Number(10)),
};
let updated = original.with_to_block(BlockNumberOrTag::Number(15));
let updated = original.with_to_block(BlockNumberOrTag::Number(15)).unwrap();
match updated {
FilterBlockOption::Range { from_block, to_block } => {
assert_eq!(from_block, Some(BlockNumberOrTag::Number(1)));
Expand All @@ -1254,14 +1275,16 @@ mod tests {
}

#[test]
#[should_panic(expected = "to_block cannot be less than from_block")]
fn test_with_to_block_failure() {
// Test scenario where to_block is less than from_block
let original = FilterBlockOption::Range {
from_block: Some(BlockNumberOrTag::Number(10)),
to_block: Some(BlockNumberOrTag::Number(15)),
};
let _ = original.with_to_block(BlockNumberOrTag::Number(5));
assert_eq!(
original.with_to_block(BlockNumberOrTag::Number(5)),
Err(FilterBlockError::ToBlockLessThanFromBlock)
);
}

#[test]
Expand All @@ -1271,7 +1294,7 @@ mod tests {
from_block: None,
to_block: Some(BlockNumberOrTag::Number(10)),
};
let updated = original.with_to_block(BlockNumberOrTag::Number(5));
let updated = original.with_to_block(BlockNumberOrTag::Number(5)).unwrap();
match updated {
FilterBlockOption::Range { from_block, to_block } => {
assert_eq!(from_block, None);
Expand All @@ -1288,7 +1311,7 @@ mod tests {
from_block: Some(BlockNumberOrTag::Earliest),
to_block: Some(BlockNumberOrTag::Latest),
};
let updated = original.with_to_block(BlockNumberOrTag::Pending);
let updated = original.with_to_block(BlockNumberOrTag::Pending).unwrap();
match updated {
FilterBlockOption::Range { from_block, to_block } => {
assert_eq!(from_block, Some(BlockNumberOrTag::Earliest));
Expand Down