Migrate go-kademlia state machines #893
Conversation
v2/coord/conversion.go
Outdated
| // This function will panic if id's underlying type is not kadt.PeerID | ||
| func NodeIDToAddrInfo(id kad.NodeID[KadKey]) peer.AddrInfo { | ||
| func KadPeerIDToAddrInfo(id kad.NodeID[KadKey]) peer.AddrInfo { |
There was a problem hiding this comment.
I think so, but I can double check. I iiterated on checking whether I could remove these helpers after each round of refactoring.
There was a problem hiding this comment.
In use in a few places. Hopefully we can straighten out the types and have fewer conversion functions in the future
v2/coord/coordinator.go
Outdated
| @@ -324,7 +324,7 @@ func (c *Coordinator) GetClosestNodes(ctx context.Context, k KadKey, n int) ([]N | |||
| closest := c.rt.NearestNodes(k, n) | |||
| nodes := make([]Node, 0, len(closest)) | |||
| for _, id := range closest { | |||
| nh, err := c.networkBehaviour.getNodeHandler(ctx, NodeIDToPeerID(id)) | |||
| nh, err := c.networkBehaviour.getNodeHandler(ctx, peer.ID(id)) | |||
There was a problem hiding this comment.
from this call, it looks liek getNodeHandler is operating on a peer.ID.
As a general separation, I think it would be great if most of the things in coord only operated on kadt.PeerID. Not sure if there is anything prohibiting that in this case here - just a general idea.
There was a problem hiding this comment.
Yes, that sounds sensible, will check usage here before merging
There was a problem hiding this comment.
I have removed this specific usage and converted the self fields for the coordiinator and behaviours to kadt.PeerID, but doing more means touching the Router interface so let's do that in a later PR.
v2/coord/internal/nettest/layouts.go
Outdated
| @@ -38,7 +37,7 @@ func LinearTopology(n int, clk clock.Clock) (*Topology, []*Node, error) { | |||
| nodes[i] = &Node{ | |||
| NodeInfo: ai, | |||
| Router: NewRouter(ai.ID, top), | |||
| RoutingTable: simplert.New[key.Key256, kad.NodeID[key.Key256]](kadt.PeerID(ai.ID), 20), | |||
| RoutingTable: simplert.New[key.Key256, kadt.PeerID](kadt.PeerID(ai.ID), 20), | |||
There was a problem hiding this comment.
Sometimes we're using key.Key256 and sometimes the alias KadKey in coord. It would be great if we could be consistent and use either everywhere.
- use key.Key256 everywhere
- move coord.KadKey to, e.g., kadt.Key and use it from there
- (make the coordinator generic on the key type and use
Keverywhere)
Intuitively I would have done 2 🤷♂️ but don't have a strong opinion.
There was a problem hiding this comment.
Agree on 2. I just missed this one. Will do another iteration of search and replace :D
There was a problem hiding this comment.
I may make the coordinator generic in the same way the state machines are. It might be cleaner, but might not be worth the effort at this stage.
|
Thanks for already doing the type changes! I only remarked nits and stuff that I can do in a separate PR for example. |
I also took the opportunity to remove usage of NodeInfo/Address and replace with a generic NodeID
I tidied up some naming and made everything more consistent. I plan to move the behaviours into the routing and query directories too and do some more cleanup next.