Refactor subnets subscriptions. #14711
Conversation
nalepae
force-pushed
the
refactor-subnets-subscriptions
branch
from
c453cc3 to
2d92c53
Compare
| @@ -87,18 +107,23 @@ func (s *Service) registerSubscribers(epoch primitives.Epoch, digest [4]byte) { | |||
| ) | |||
| if flags.Get().SubscribeToAllSubnets { | |||
| s.subscribeStaticWithSubnets( | |||
| "Attestation", | |||
There was a problem hiding this comment.
Can we provide the actual topic or object type here ? Its better than using hardcoded strings here
There was a problem hiding this comment.
This description string is only used in:
log.WithField("digest", digest).Infof("%s subnets with are no longer valid, unsubscribing from all of them.", description)Before, we had the log hardcoded version:
log.Warnf("Attestation subnets with digest %#x are no longer valid, unsubscribing from all of them.", digest)So, it was already hard-coded before, but not at the same location.
I can for sure use the actual topic instead, but the log will be less human readable.
There was a problem hiding this comment.
Could you possibly use a helper function with a switch of all topics to derive this (ex: If the topic is the attestation subnet, then the description will always be Attestation . And so on. This way we don't need to add in an additional argument
| if flags.Get().SubscribeToAllSubnets { | ||
| s.subscribeStaticWithSyncSubnets( | ||
| s.subscribeStaticWithSubnets( | ||
| "Sync committee", |
There was a problem hiding this comment.
We already add in the SyncCommitteeSubnetTopicFormat topic, so do we need to add this again ?
What type of PR is this?
Other
What does this PR do? Why is it needed?
This pull requests refactors subnets subscriptions.
It removes
subscribeDynamicWithSubnetsandsubscribeStaticWithSyncSubnetsto only usesubscribeDynamicWithSubnetsandsubscribeStaticWithSubnets.It also paves the way of future data columns (for peerDAS) subscriptions without the need to add specifics functions.
Acknowledgements