Page MenuHomePhabricator
Differential D12998

[avalanche] Leverage AnyVoteItem to have a single VoteItemUpdate object
ClosedPublic
Actions

Authored by Fabien on Jan 18 2023, 15:43.

Details

Reviewers
sdulfari
Group Reviewers
Restricted Project
Commits
rABC4d77536c6183: [avalanche] Leverage AnyVoteItem to have a single VoteItemUpdate object
Summary

Remove the type specific vote update, so a single VoteItemUpdate can be used and remove some boilerplate.

There is no change in behavior.

Depends on D12995, D12996, D12997.

Test Plan
ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien created this revision.Jan 18 2023, 15:43
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 18 2023, 15:43
Fabien requested review of this revision.Jan 18 2023, 15:43
Harbormaster completed remote builds in B21781: Diff 37581.Jan 18 2023, 15:55
sdulfari requested changes to this revision.Jan 18 2023, 17:49
sdulfari added a subscriber: sdulfari.
sdulfari added inline comments.
src/avalanche/test/processor_tests.cpp
283 ↗(On Diff #37581)

Why not getItem?

370–375 ↗(On Diff #37581)

This test case is now item type agnostic.

This revision now requires changes to proceed.Jan 18 2023, 17:49
Fabien added inline comments.Jan 19 2023, 08:42
src/avalanche/test/processor_tests.cpp
283 ↗(On Diff #37581)

To make it more explicit, especially when you read the test case you might not understand why you need an additional call to getItem (you already have an item, right ?), despite fromAnyVoteItem make it clear that you have the wrong type. Let me know if you disagree.

370–375 ↗(On Diff #37581)

ah good catch, I initially unrolled but forgot to rename after I switched to a templated test case

Fabien updated this revision to Diff 37589.Jan 19 2023, 08:45

Rename the test variable

Harbormaster completed remote builds in B21789: Diff 37589.Jan 19 2023, 08:57
sdulfari accepted this revision.Jan 20 2023, 16:39
sdulfari added inline comments.
src/avalanche/test/processor_tests.cpp
283 ↗(On Diff #37581)

I think there's something missing because the "from" implies a "to" which doesn't read well in the tests or anywhere else. But I don't want to block on this so this patch is green. We can discuss offline and rename if something else is better.

This revision is now accepted and ready to land.Jan 20 2023, 16:39
Closed by commit rABC4d77536c6183: [avalanche] Leverage AnyVoteItem to have a single VoteItemUpdate object (authored by Fabien). · Explain WhyJan 20 2023, 20:07
This revision was automatically updated to reflect the committed changes.
Fabien added a commit: rABC4d77536c6183: [avalanche] Leverage AnyVoteItem to have a single VoteItemUpdate object.

Revision Contents
Changeset List

PathSize
src/
avalanche/
20 lines
144 lines
test/
64 lines
141 lines

Diff 37623

View Options

src/avalanche/processor.h

Loading...
View Options

src/avalanche/processor.cpp

Loading...
View Options

src/avalanche/test/processor_tests.cpp

Loading...
View Options

src/net_processing.cpp

Loading...