feat(message-cache): make the topic cache generic

[LNSD]

The current topic cache implementation can be made generic to support also the filter use case. Now it allows different types of keys:

• The key type for the relay is string (the type of PubSub topics)
• The key type for the filter should be a tuple of PubsubTopicString (aka string) and ContentTopic.

This work is necessary for #1076 (filter REST API)

[rymnc]

LGTM

[kaiserd]

LGTM 👍

I have two remarks:

1. In case the cache should only be generic over "topic kinds" like pubsub topic and content topic,
   I'd add a comment in the code explaining that.
   Otherwise, imo, a signature like this is confusing:
   proc isSubscribed*[K](t: MessageCache[K], topic: K)
   It might raise the question: why is the key name "topic"?

2. Since the generic naming for a topic kind like pubsub and content topic has been introduced using the name topic,
   naming the specific pubsubTopic version of the cache TopicCache is confusing.

(Generally, in other parts like the RFCs and so on, I'd always explicitly use pubsub topic and content topic;
if we did this consistently, simply calling this generic version topic would be OK, imo.)

We could also make the message cache generic over any type of key and rename

proc isSubscribed*[K](t: MessageCache[K], topic: K): bool
proc subscribe*[K](t: MessageCache[K], topic: K)
proc unsubscribe*[K](t: MessageCache[K], topic: K)

to

proc hasKey*[K](t: MessageCache[K], key: K): bool
proc addKey*[K](t: MessageCache[K], key: K)
proc deleteKey*[K](t: MessageCache[K], key: K)

Would make the names less expressive though.... and introducing a hierarchy that overrides such methods would be overkill 😅

[kaiserd]

nit: Typically, cstring is only used when interfacing via FFI.
Sometimes, our code uses cstring sometimes string. Imo, we should be consistent here.

[jm-clius]

May be worth noting that the Nim style guide recommends using cstring with Result: https://status-im.github.io/nim-style-guide/libraries.results.html

[jm-clius]

Regarding @kaiserd suggestions above: No strong opinions, but I agree with adding a comment to indicate that the MessageCache is designed to be a cache of messages against subscribed topics (pubsub- or contentTopic) and perhaps renaming TopicCache to PubsubTopicCache. Don't think it's necessary to be even more generic now, at least until we have identified a need for a very generic MessageCache.

[kaiserd]

May be worth noting that the Nim style guide recommends using cstring with Result: https://status-im.github.io/nim-style-guide/libraries.results.html

Good to know :). Thanks. (In one of my first PRs, I was asked why I don't use string as the error type because the context was Nim-only. I just copied and did not know this is in the style guide 😅 .)

[jm-clius]

Hah! Makes sense. You're absolutely right that we haven't been consistent with this (and it's only been recently that I started using cstring with Result after a closer reading of the style guide :) )

[jm-clius]

LGTM!

[LNSD]

In case the cache should only be generic over "topic kinds" like pubsub topic and content topic,

Yes, that's the idea.

• In the relay case: the key is the pubsub topic.
• In the filter version of the "topic cache" the key will be a tuple formed by the pubsub topic and the content topic. This is coming in one of the Filter REST API PRs.

That's the reason why I made it generic like that. I am not looking to make it super generic. Just to be used in the REST API HTTP request handlers.

[LNSD]

Would make the names less expressive though.... and introducing a hierarchy that overrides such methods would be overkill 😅

Sorry, but I'd rather not change the name of those methods.

Those names are very expressive and make a lot of sense in the context this message cache is used. The suggested method names are typically associated with dictionaries (a.k.a. tables) and this is not exactly a table. We should understand this as an addressable (by pubsub/pubsub+content topic) list of message queues.

[kaiserd]

Yes. That's fine, too. Making it more generic and changing method names was just one possible solution.
Maybe my wording was confusing. I meant, I'd either rename the TopicCache or make it more generic to avoid overloading the meaning of the "topic" name.
What is confusing to me in the current version is the file name topic_cache.nim and the cache type's name, because it is specific to pubsub topic:

type TopicCache* = MessageCache[PubSubTopicString]

whereas "topic" in the generic type's procs' signatures names something generic over (pubsubTopic, contentTopic, a tuple of those, ...).

Maybe:

type PubsubTopicCache* = MessageCache[PubSubTopicString]

[LNSD]

Ok, thanks. I am taking notes about that. I am planning to do a Relay REST API code reorganization. I will rename it under that PR to PubsubTopicCache, or maybe something like RelayMessageCache since the implementation is tied (see the handler part of the topic_cache.nim file) to the protocol.