-
Notifications
You must be signed in to change notification settings - Fork 262
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
Return events from blocks skipped over during Finalization, too #473
Merged
+206
−59
Merged
Changes from 7 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
b525401
make subscription stream generic in EventSubscription
jsdw 11368cf
rename to EventSub/FinalizedEventSub
jsdw 40ad3fc
wip fix some lifetimes so that event sub can depend on client in stream
jsdw 3fb6493
Cargo fmt + comment tweaks
jsdw 2380c77
Add another comment
jsdw 09c69e4
factor out prev block header fetching into a separate function to tidy
jsdw 4ded755
add a comment
jsdw e4c18ce
remove ListOrValue as it's unused
jsdw 3f728cd
Into<u128> on BlockNumber to simplify things
jsdw 3ee62ec
clippy
jsdw 4d8cde5
Fix an example and clippy
jsdw 8504b10
simplify iterator now we are Into<u128>
jsdw 5fbc66d
Into<u64> instead because it needs serializing, and test core logic
jsdw a6acd6b
Tweak missing block test to fill in >=2 holes
jsdw 1242471
tweak a comment
jsdw File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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'm not a fan of duplicating this
block_hash
function. The problem is thatConfig::BlockNumber
isn't something that we can easily work with.I could also require that
Config::BlockNumber
implements something likeInto<u128>
, or I could remove the "other" copy of this function, but it sounds like some consideration went into it, so I was hesitant. What do you think @ascjones?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.
The nice thing abour requiring
Into<u128>
or similar would be that I can get rid ofnum_traits
again, and the other required traits I added toBlockNumber
, which I was just using to add a minimal amount of information about the BlockNumber so that I could iterate over them.Is it reasonable to assume that nobody would use something more esoteric than a rust unsigned int to represent block numbers, and so
Into<u128>
shouldn't cause any issues?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 think it is reasonable to use
Into<u128>
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 went for
Into<u64>
in the end because u128's don't serialize to JSON so easily; I'm hopingu64
is also a safe target here (I assume it's basically a choice betweenu32
andu64
that people would tend to make?)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.
Into<u64>
should also be fine.