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

Add testing of protocol features to CI #1889

Open
Ifropc opened this issue Feb 22, 2025 · 2 comments
Open

Add testing of protocol features to CI #1889

Ifropc opened this issue Feb 22, 2025 · 2 comments
Assignees

Comments

@Ifropc
Copy link
Contributor

Ifropc commented Feb 22, 2025

What problem does your feature solve?

We currently don't test protocol features (version_ge_23) in CI

What would you like to see?

Adding tests to CI

What alternatives are there?

Alternatively, we could use a separate branch for future releases. In this case we wouldn't need to maintain protocol features

@github-project-automation github-project-automation bot moved this to Backlog (Not Ready) in DevX Feb 22, 2025
@leighmcculloch
Copy link
Member

💯

@Ifropc
Copy link
Contributor Author

Ifropc commented Feb 25, 2025

A bit more on this issue:
Currently, we don't have any tests in place for version_ge_23 and the way I see it we either need to add support for testing protocol features, or remove features support and use branches instead.

Both approaches have pros and cons, and the way I see it is the following:

Protocol features pros:

  • Extensible to other features we may bring to our codebase in the future
  • Single branch resulting in no unexpected merge conflicts long term
  • Every change is automatically tested against p22 and p23 (once we have necessary pipelines in place)

Protocol features cons:

  • Reduced code readability (I think code is less readable with introduction of features, see sample in Make keys generate to no longer fund #1887 )
  • Longer testing time -- on each PR we need to test both p22 and p23 changes
  • More bug prone IMO (due to protocol number branching rules)

Branch pros:

  • Easier to read the code + code separation
  • Follows git workflow

Branch cons:

  • Potential big merge conflicts
  • Need to frequently forward changes from main into develop/p23
  • Changes merged into main branch are not tested on p23 until PR from main -> develop/p23

I personally think that protocol features bring quite a bit complexity to the project (mainly, because it's harder to follow the logic in the code, but maybe we adopt some kind of simple rules, e.g. styling code as following):

// Main function doesn't contain branching logic makes it easier to follow
fn some_func() {
 ...
 do_stuff(params);
 ...
}

// Function with just branching logic
fn do_stuff(params) {
  #[cfg(feature = "version_lt_23")]
  do_stuff_p22(params);
  #[cfg(feature = "version_gte_23")]
  do_stuff_p23(params);
}

// Functions that performs the task, only for one protocol version
#[cfg(feature = "version_lt_23")]
fn do_stuff_p22(params) {
   ...
}

do_stuff_p23(params) {
   ...
}

Any suggestions are welcome

@Ifropc Ifropc self-assigned this Feb 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog (Not Ready)
Development

No branches or pull requests

2 participants