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

Revert approval voting #5438

Merged
merged 11 commits into from
May 11, 2022
Merged

Revert approval voting #5438

merged 11 commits into from
May 11, 2022

Conversation

davxy
Copy link
Member

@davxy davxy commented May 3, 2022

Partially addresses #5396

This PR is a follow up of #5350 and addresses the revert of the ApprovalVoting subsystem.

The operation api is similar to the one exposed by the ChainSelection subsystem.

No particular blockers were found.

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label May 3, 2022
@davxy davxy requested review from a team and drahnr May 3, 2022 10:02
@davxy davxy added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels May 3, 2022
@davxy davxy mentioned this pull request May 3, 2022
4 tasks
@davxy davxy self-assigned this May 3, 2022
@davxy davxy marked this pull request as draft May 3, 2022 16:15
@davxy davxy removed the A0-please_review Pull request needs code review. label May 3, 2022
@davxy davxy marked this pull request as ready for review May 3, 2022 18:33
@github-actions github-actions bot added the A0-please_review Pull request needs code review. label May 3, 2022
Copy link
Contributor

@skunert skunert left a comment

Choose a reason for hiding this comment

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

Revert logic looks good to me.
Otherwise just some nits.

node/service/src/lib.rs Outdated Show resolved Hide resolved
node/core/approval-voting/src/backend.rs Outdated Show resolved Hide resolved
(children, children_height)
},
None => {
let children_height = stored_range.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We could use a Range<usize> type to make this a bit more natural rather than using a tuple?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure about the suggestion, but the tuple is (Vec, BlockNumber).
I can't use a Range here

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I'd use a struct with named fields :)

Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

A few small nits, generally looks good 👍

Copy link
Contributor

@drahnr drahnr left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - would appreciate fewer .0 and .1s but that's not blocking

@davxy davxy merged commit 0f89b70 into master May 11, 2022
@davxy davxy deleted the davxy-revert-approval-voting branch May 11, 2022 08:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants