-
Notifications
You must be signed in to change notification settings - Fork 248
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
rpc v2: chainhead support multiple finalized block hashes in FollowEvent::Initialized
#1476
Conversation
47ac2d0
to
b5b6667
Compare
b5b6667
to
7c26b02
Compare
block_hash -> finalized_block_hashes
block_hash -> finalized_block_hashes
block_hash -> finalized_block_hashes
FollowEvent::Initialized
testing/integration-tests/src/full_client/client/unstable_rpcs.rs
Outdated
Show resolved
Hide resolved
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.
LGTM, Just a couple of things!
284913e
to
fc5b897
Compare
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.
LGTM!
this.pin_unpinnable_block_at(rel_block_num, *finalized_block); | ||
|
||
finalized_block_hashes.push(block_ref); | ||
rel_block_num += 1; |
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.
nit: would this.rel_block += 1
work here?
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.
Good spot; now there are multiple we should increment this.rel_block_num
here!
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 old code didn't increment that but sure lemme fix that..
For example if finalized_block_hashes == vec![one_hash]
then it would increment this.rel_block_num which it didn't do before. Cool?
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.
LGTM! Thanks for fixing this 🙏
Companion for paritytech/polkadot-sdk#3445 where it's now possible that the initialized event
can contain multiple finalized block hashes.