Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Default rather than option#9290

Closed
gilescope wants to merge 1 commit intoparitytech:masterfrom
gilescope:giles-simpler-finality
Closed

Default rather than option#9290
gilescope wants to merge 1 commit intoparitytech:masterfrom
gilescope:giles-simpler-finality

Conversation

@gilescope
Copy link
Contributor

Implementes simplification suggested in #9177

I'm in two minds as to whether I like this change or not, so throwing it open to the reviewers.

@gilescope gilescope requested a review from andresilva as a code owner July 6, 2021 15:34
@@ -1173,7 +1173,7 @@ where
debug!(target: "afg", "Finding best chain containing block {:?} with number limit {:?}", block, limit);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just above it's looking for the block so we know it exists.

block_on(longest_chain_select.finality_target(uninserted_block.hash().clone(), None))
.unwrap(),
);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test didn't really make sense any more.

@andresilva
Copy link
Contributor

There is already #9192 open for this, since it's coming from a community member (and it also was opened first) I'd prefer merging that one instead. I haven't reviewed it yet since I was out, will do.

@gilescope gilescope closed this Jul 6, 2021
@gilescope
Copy link
Contributor Author

Sorry missed that. Looks like we agree on the changes - their PR LGTM.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants