-
Notifications
You must be signed in to change notification settings - Fork 22
[DRAFT] refactor: remove frequency-bridging feature and related configurations #2569
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
build-bridging-testnet: | ||
cargo build --features frequency-testnet,frequency-bridging | ||
# build-bridging-testnet: | ||
# cargo build --features frequency-testnet,frequency-bridgin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking, but it is odd that the commented-out features are all misspelled
…equency-lint-check features
…r relay and no-relay modes
…-remove-feature-frequency-bridging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we're keeping all of the commented-out stuff in the Makefile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just haven't cleaned it up yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, are we just getting rid of all Westend-related code, the idea being that we're focusing on bridging on Paseo (and then Mainnet)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Westend-local was kind of a hack for testing with Snowbridge Zombienet, so removing it to clean that up. The westend release still remains.
#[cfg(feature = "frequency-no-relay")] | ||
use crate::no_relay::prelude as runtime_mode; | ||
#[cfg(any(not(feature = "frequency-no-relay"), feature = "frequency-lint-check"))] | ||
use crate::relay::prelude as runtime_mode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... initially I was confused by:
#[cfg(any(not(feature = "frequency-no-relay"), feature = "frequency-lint-check"))]
that's used in so many places; it seems like adding feature = "frequency-lint-check"
to the any(...)
ONLY enables the use case: --features frequency-no-relay,frequency-lint-check
. I was going to let it slide, but here I think I see a problem. If we run --features frequency-no-relay,frequency-lint-check
, here we'll end up with:
use crate::no_relay::prelude as runtime_mode; // enabled by "frequency-no-relay"
use crate::relay::prelude as runtime_mode; // enabled by "frequency-lint-check"
I think this would like be problematic...so, by that logic, it seems like we should NEVER allow both frequency-no-relay
AND frequency-lint-check
at the same time. And, therefore, we can reduce all of the instances of:
#[cfg(any(not(feature = "frequency-no-relay"), feature = "frequency-lint-check"))]
to simply
#[cfg(not(feature = "frequency-no-relay"))]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
frequency-lint-check
is meant to include both the relay and no-relay configurations so that everything will be checked by the linter. This flag is only meant to be used for tests and lints, it's not meant to be a proper build.
We have an existing issue (#2367) to remove the frequency-lint-check
feature and duplicate any tests that use the flag, which would make the code maintenance easier. I think the effort to remove the lint flag needs to wait until we merge/remove the bridging feature.
Goal
The goal of this PR is to remove the
frequency-bridging
feature flag and refactor the code so that it is easier to read and maintain.Closes #2515
Discussion
frequency-no-relay
continues to work correctly. XCM features require some cumulus features.Checklist