Page MenuHomePhabricator

[avalanche] Remove getPeerId
ClosedPublic

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

Event Timeline

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

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.

This revision is now accepted and ready to land.Nov 17 2021, 17:27
This revision was automatically updated to reflect the committed changes.