Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[skip changelog] feat(f3): move F3 pubsub topics to F3 module #12383

Closed
wants to merge 1 commit into from

Conversation

Stebalien
Copy link
Member

Related Issues

Pre-requisite for #12204

Proposed Changes

Move F3 pubsub topics to F3 module. This way, we don't have to rely on a global variable to determine if F3 is enabled, which will make testing easier. Instead, we can provide the pubsub topics when we provide the F3 manifest provider and service.

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

@Stebalien Stebalien force-pushed the steb/pluggable-pubsub-topics branch 3 times, most recently from cc1906a to 42b6bd9 Compare August 14, 2024 01:41
allowTopics = append(allowTopics, f3TopicName+"/"+fmt.Sprintf("%d", i))
}
}
build.F3TopicName(in.Nn),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always enabled. The filter exists to protect against a DoS attack with too many topic names, adding a single one won't make a difference.

@@ -163,7 +163,8 @@ var ChainNode = Options(
),

If(build.IsF3Enabled(),
Override(new(manifest.ManifestProvider), lf3.NewManifestProvider),
Override(new(*manifest.Manifest), lf3.NewManifest),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will make it easier to override the manifest while testing without changing everything.

node/builder.go Outdated
BandwidthReporterKey = special{11} // Libp2p option
ConnGaterKey = special{12} // Libp2p option
ResourceManagerKey = special{14} // Libp2p option
F3ManifestProviderKey = special{15} // F3 Manifest Provider
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this here because the new manifest provider optionally returns a list of pubsub topics.

@Stebalien Stebalien force-pushed the steb/pluggable-pubsub-topics branch from 42b6bd9 to 2039cd6 Compare August 14, 2024 01:42
This way, we don't have to rely on a global variable to determine if F3
is enabled, which will make testing easier.

Pre-requisite for #12204
@Stebalien
Copy link
Member Author

Stebalien commented Aug 14, 2024

Ah, lovely. It's a DI cycle.

@Stebalien Stebalien marked this pull request as draft August 14, 2024 03:18
@Stebalien Stebalien closed this Aug 28, 2024
@Stebalien
Copy link
Member Author

Closed because this has to be re-done from scratch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

1 participant