[avalanche] Remove getPeerId
ClosedPublic
Actions

Authored by Fabien on Nov 8 2021, 08:54.

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Maniphest Tasks

Restricted Maniphest Task

Commits

rABC800f21378651: [avalanche] Remove getPeerId

Summary

This accessor does more that its name implies, as it attempts to create a peer if it doesn't exists which makes the API confusing. This was intended to be a test only method, but it sneaked outside of its original scope.

This diff removes getPeerId() and use registerProof() instead when this is the expected behavior. The original behavior is now enforced as test-only by moving it to the friend test class.

Ref T1854.

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Branch

avalanche_getpeerid

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 17200
Build 34231: Build Diff	build-debug · build-diff · build-without-wallet · build-clang-tidy · lint-circular-dependencies · build-clang
Build 34230: arc lint + arc unit

Event Timeline

Fabien created this revision.Nov 8 2021, 08:54

Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 8 2021, 08:54

Fabien requested review of this revision.Nov 8 2021, 08:54

Harbormaster completed remote builds in B17200: Diff 30794.Nov 8 2021, 09:13

hm getPeerId isn't used as an accessor anywhere, so you're simplifying one function at the cost of more code everywhere else

maybe simplify it even more and just get rid of it? all it does in addition to fetchOrCreatePeer is pluck the PeerId associated with the iterator anyway

there's also a subtle behavior change, if PeerManager::exists(the_proof_id) we don't call fetchOrCreatePeer anymore

hm getPeerId isn't used as an accessor anywhere, so you're simplifying one function at the cost of more code everywhere else

The fact that it isn't used as an accessor demonstrates that the API is unclear. You'd expect getPeerId to be an accessor don't you ?
This is indeed requiring more code but only in tests, which is the only place where it's effectively used as an accessor.

maybe simplify it even more and just get rid of it? all it does in addition to fetchOrCreatePeer is pluck the PeerId associated with the iterator anyway

I considered that in 2 ways:

Remove it entirely,
Make it private.

But since there was a comment clearly stating Functions which are public for testing purposes. in the header I went for the minimal change.
From your comment it looks like it adds to confusion, so I'll update that.

there's also a subtle behavior change, if PeerManager::exists(the_proof_id) we don't call fetchOrCreatePeer anymore

That was already the case ?

Remove getPeerId completely

Fabien retitled this revision from [avalanche] Simplify getPeerId to [avalanche] Remove getPeerId.Nov 9 2021, 08:17

Fabien edited the summary of this revision. (Show Details)

Fabien added inline comments.

src/avalanche/test/peermanager_tests.cpp
37 ↗	(On Diff #30797)	Note to reviewers: I didn't use `fetchOrCreateProof` on purpose here as it will conflict with other changes I'm planning to do that splits it in parts.