-
Notifications
You must be signed in to change notification settings - Fork 4
Block scanner refactor #21
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
Conversation
|
Ignore the failing clippy job, I will fix this when we're sure the design is ok |
|
src/block_scanner_new.rs
Outdated
| start_height: BlockNumberOrTag, | ||
| end_height: Option<BlockNumberOrTag>, | ||
| buffer_size: usize, |
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.
Maybe better to have like optional config?
struct Config {
start_height: Option<BlockNumberOrTag>,
end_height: Option<BlockNumberOrTag>
buffer_size: Option<size>
}
subscribe(
&self,
config: Config,
)Just so we have named parameters i think it makes a nicer API. We could also have sensible defaults for them.
Start Height --> Latest
End Height --> None
Buffer Size --> 1 (process one block at a time)
wdyt
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 removed buffer_size as it makes little sense (as we discussed yesterday).
This issue #26 avoids the default value logic completely and makes the Config redundant imo. The reason it avoids the issue is that it makes it clear what the client wants to do based on the command they issue, so these height parameters no longer appear for all commands and defaults are implicit.
LeoPatOZ
left a comment
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.
Had a quick look - i currently only made comments regarding the API i.e. how someone would interact with this library rather than the specifics of the internals.
Depends on how you want to handle it, either we just get this merged now with some API changes then deal with the internals later (shouldnt be any breaking changes) or we try to clean up things in this PR. Im more for the former but let me know what you think.
Closes #1
Instead of returning a stream of block ranges, it is much more straightforward to accept a channel from the caller to which to send block ranges.
This also allows the caller to control the pace at which they receive new block ranges (1 at a time, all at once etc.).