Open AndrewSisley opened 1 year ago
The reason it's duplicated is two fold:
I'd rather not return a client type from db.GetAllP2PCollections and I'd rather not expose the db field publicly. This leaves us with the current implementation where we ask the Peer of the P2P collections. It then has the chance to return a structured representation of the P2P collections.
Discussed on Discord with Andy, we could prefix the topics in order to make them filterable. That way it would be possible to return the list of P2P collection topics from the server.
d/{some-dockey}
c/{some-collectionID}
Peer.GetAllP2PCollections returns database state, not server state.
I'm totally happy if this is the desired behaviour we want long term, but the functions around it
AddP2PCollections
andRemoveP2PCollections
deal with server state as well as the persisted db state, and there is always going to be some risk that the two may differ (we will never be perfect coders).The function documentation also states that it
gets all the collectionIDs from the pubsup topics
, which to me (an internal dev) reads like it gets the state of the server.If we want to keep this function's behaviour as-is, I'd suggest a tweaking of the func documentation, and perhaps a new function that returns the actual state of the server.
Given that
db.GetAllP2PCollections
also exists (and is pretty much all the Peer func does), I would suggest that we changePeer.GetAllP2PCollections
as the client/db function covers the behaviour that this function currently has and there seems little reason to duplicate it.Note: Unsure whether to flag this as a bug, documentation or code quality, so I put it as bug for now. Probably quite low priority right now.