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.

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

extends logs with extra metadata and query params #651

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

r0wdy1
Copy link

@r0wdy1 r0wdy1 commented Jun 24, 2022

This PR addresses the issue below
#650

@@ -279,7 +279,16 @@ impl<T: Transport> Contract<T> {
}

/// Find events matching the topics.
Copy link
Owner

Choose a reason for hiding this comment

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

The documentation should be updated.

src/types/log.rs Outdated Show resolved Hide resolved
src/types/log.rs Outdated Show resolved Hide resolved
src/types/log.rs Outdated Show resolved Hide resolved
Comment on lines +123 to +127
pub fn from_block(mut self, block: Option<BlockNumber>) -> Self {
if let Some(block) = block {
self.filter.block_hash = None;
self.filter.from_block = Some(block);
}
Copy link
Owner

Choose a reason for hiding this comment

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

This method should not be changed, the builder should only take values that are defined, not Options. None case should be handled at the call site.

Comment on lines +133 to +137
pub fn to_block(mut self, block: Option<BlockNumber>) -> Self {
if let Some(block) = block {
self.filter.block_hash = None;
self.filter.to_block = Some(block);
}
Copy link
Owner

Choose a reason for hiding this comment

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

Same here.

Comment on lines +144 to +149
pub fn block_hash(mut self, hash: Option<H256>) -> Self {
if let Some(_block_hash) = hash {
self.filter.from_block = None;
self.filter.to_block = None;
self.filter.block_hash = hash;
}
Copy link
Owner

Choose a reason for hiding this comment

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

ditto.

Comment on lines +285 to +287
from_block: Option<BlockNumber>,
to_block: Option<BlockNumber>,
block_hash: Option<H256>,
Copy link
Owner

Choose a reason for hiding this comment

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

Having 7 parameters, and 3 of them being optional is a bit too much. I'd like to propose a helper struct with a builder to amalgamate them.

pub struct BlockRange {
  from_block: Option<BlockNumber>,
  to_block: Option<BlockNumber>,
  block_hash: Option<H256>,
}

impl BlockRange {
  // set both `from_block` and `to_block`
  pub fn range(from: BlockNumber, to: BlockNumber) -> Self;
  // set just `from_block`
  pub fn from(BlockNumber) -> Self;
  // set just `to_block`
  pub fn to(BlockNumber) -> Self;
  // set just `block_hash`
  pub fn hash(H256) -> Self;
  // look for one exact block (set both `from/to` and `hash`).
  pub fn exact(BlockNumber, H256) -> Self;

  // applies the parameters to given filter
  pub (crate) fn apply_to(&self, b: FilterBuilder) -> FilterBuilder;
}

r0wdy1 and others added 3 commits September 27, 2023 13:04
Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>
Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>
Co-authored-by: Tomasz Drwięga <tomusdrw@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants