Skip to content

Conversation

@AlexandruCihodaru
Copy link

No description provided.

Signed-off-by: Alexandru Cihodaru <[email protected]>
Copy link

@alindima alindima left a comment

Choose a reason for hiding this comment

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

Great work! Very thorough tests

}
Poll::Ready(Some(Ok(new_blocks).to_rpc_result()))
}
// handled directly in get_filter_changes

Choose a reason for hiding this comment

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

IMO it'd be cleaner if we used the same pattern for all filter types. either implementing poll manually or using async functions for all

Copy link
Author

Choose a reason for hiding this comment

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

Will do this in the last PR of the filters series. I already have the change locally.

Signed-off-by: Alexandru Cihodaru <[email protected]>
Copy link

@iulianbarbu iulianbarbu left a comment

Choose a reason for hiding this comment

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

Very good work! Thanks 🚀

Comment on lines +197 to +198
// This can be optimized if we also submit the block number
// from subscribe_and_cache_new_blocks

Choose a reason for hiding this comment

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

Probably a similar block by hash will be done there as well to return the block number. I think the subscriptions return only hashes.

Copy link

@re-gius re-gius left a comment

Choose a reason for hiding this comment

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

LGTM - Great job with testing

@AlexandruCihodaru AlexandruCihodaru merged commit 20d4033 into master Nov 25, 2025
78 of 135 checks passed
@AlexandruCihodaru AlexandruCihodaru deleted the acihodaru/log_filters branch November 25, 2025 08:18
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.

5 participants