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

Omni-Node renamings #5915

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

serban300
Copy link
Contributor

@serban300 serban300 commented Oct 3, 2024

  • moved the omni-node lib from
    cumulus/polkadot-parachain/polkadot-parachain-lib to
    cumulus/polkadot-omni-node/lib
  • renamed polkadot-parachain-lib to polkadot-omni-node-lib
  • added polkadot-omni-node binary

Related to #5566

@serban300 serban300 added the T0-node This PR/Issue is related to the topic “node”. label Oct 3, 2024
@serban300 serban300 self-assigned this Oct 3, 2024
@serban300 serban300 force-pushed the polkadot-parachain-renamings branch 5 times, most recently from a21aa14 to f977da2 Compare October 3, 2024 12:48
@skunert
Copy link
Contributor

skunert commented Oct 4, 2024

I am not entirely sure if we gain something from this naming. To us its maybe obvious. But looking from the outside, polkadot-parachain or even just parachain-node sounds more intuitive and less surprising to me. In the end this is exactly what it is, just a parachain-node.

@serban300
Copy link
Contributor Author

I am not entirely sure if we gain something from this naming. To us its maybe obvious. But looking from the outside, polkadot-parachain or even just parachain-node sounds more intuitive and less surprising to me. In the end this is exactly what it is, just a parachain-node.

@kianenigma what do you think ?

@kianenigma
Copy link
Contributor

(have not read the code yet)

I am not surprised to see pushback on the renaming 🙈

I am in favor of the renaming. Here's my reasons:

  1. It amplifies the code-word of the project "omni-something". It is a guess, but I think it is more comprehensible to repeat the omni-prefix in all such tools.
  2. Calling it parachain-node and/or polkadot-parachain prevents us from ever even imagining adding more abilities related to being a solo-chain to this node, something that I guess even parachain teams reasonably want esp. to run a testnet.
  3. It changes nothing about polkadot-parachain: It remains as is, and it is indeed "Just a polkadot parachain node" as @skunert puts it.

Perhaps the confusion is partly because my original issue was lacking on point (remove any chainspec from the generic polkadot-omni-node). The outcome of this PR should be:

  1. rename the library crate to polkadot-omni-node-lib. This is hidden from all users atm, because no one is using this crate yet (unless if you are @xlc and acala-node).
  2. polkadot-parachain remains 100% as-is, and it is an instantiation of polkadot-omni-node-lib with all polkadot-related chainspecs hardcoded. This makes perfect sense to me in terms of naming: A parachain node, built from the generic polkadot-omni-node-lib, that is specialized for running Polkadot system chains, ergo polakdot-parachain.
  3. polkadot-omni-node is a new binary, which is a raw instantiation of polkadot-omni-node-lib without any chainspecs or specialization.

So when looked at this way, we are not really touching polkadot-parachain. It remains as-is, and we are adding a new binary that is the more generic version of it.

Cargo.toml Outdated
"cumulus/polkadot-parachain",
"cumulus/polkadot-parachain/polkadot-parachain-lib",
"cumulus/polkadot-omni-node/polkadot-omni-node-lib",
"cumulus/polkadot-omni-node/polkadot-parachain",
Copy link
Contributor

Choose a reason for hiding this comment

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

So just looking at this file, i can confirm that this is not what I intended (as noted, my original issue was not super clear). High level, expect to see something like:

cumulus/polkadot-parachain should stay. This is also crucial for our internal devops to not put a bounty on my head.
cumulus/polkadot-omni-node should be a new crate.
cumulus/polkadot-parachain-lib -> cumulus/polkadot-omni-node/lib

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I misunderstood the requirement a bit. Will fix it.

@skunert
Copy link
Contributor

skunert commented Oct 4, 2024

(have not read the code yet)

I am not surprised to see pushback on the renaming 🙈

I am in favor of the renaming. Here's my reasons:

1. It amplifies the code-word of the project "omni-_something_". It is a guess, but I think it is more comprehensible to repeat the omni-prefix in all such tools.

2. Calling it `parachain-node` and/or `polkadot-parachain` prevents us from ever even _imagining_ adding more abilities related to being a solo-chain to this node, something that I [guess even parachain teams reasonably want](https://github.com/orgs/paritytech/projects/157/views/1?pane=issue&itemId=82036322) esp. to run a testnet.

3. It changes nothing about polkadot-parachain: It remains as is, and it is indeed "Just a polkadot parachain node" as @skunert puts it.

Perhaps the confusion is partly because my original issue was lacking on point (remove any chainspec from the generic polkadot-omni-node). The outcome of this PR should be:

0. rename the library crate to `polkadot-omni-node-lib`. This is hidden from all users atm, because no one is using this crate yet (unless if you are @xlc and `acala-node`).

1. `polkadot-parachain` remains 100% as-is, and it is an instantiation of `polkadot-omni-node-lib` with all polkadot-related chainspecs hardcoded. This makes perfect sense to me in terms of naming: A parachain node, built from the generic `polkadot-omni-node-lib`, that is specialized for running Polkadot system chains, ergo `polakdot-parachain`.

2. `polkadot-omni-node` is a new binary, which is a raw instantiation of `polkadot-omni-node-lib` without any chainspecs or specialization.

So when looked at this way, we are not really touching polkadot-parachain. It remains as-is, and we are adding a new binary that is the more generic version of it.

These are good points, I also forgot about frame-omni-bencher which goes nicely with this.

@kianenigma
Copy link
Contributor

kianenigma commented Oct 4, 2024

These are good points, I also forgot about frame-omni-bencher which goes nicely with this.

In my dreams, the binary list that one would see in our release pages would be:

  1. polkadot-parachain
  2. polkadot-omni-node
  3. polkadot-omni-bencher
  4. polkadot-omni-chain-spec-builder

Not to late to rename them? idk. But given https://github.com/orgs/paritytech/projects/157/views/1?pane=issue&itemId=78432998 in the roadmap, I won't push for this. Goal is to allow users to to all of this in just polkadot-omni-node.

@serban300 another small action item that comes to mind here is to ensure that as a consequence of this PR, polkadot-omni-node is also released. This requires a few GH/GL action files to be edited, #5387 is probably a good example. CC @EgorPopelyaev

@serban300 serban300 force-pushed the polkadot-parachain-renamings branch 3 times, most recently from aa30052 to b67524d Compare October 7, 2024 12:05
- moved the omni-node lib from
  `cumulus/polkadot-parachain/polkadot-parachain-lib` to
  `cumulus/polkadot-omni-node/lib`
- renamed `polkadot-parachain-lib` to `polkadot-omni-node-lib`
@serban300
Copy link
Contributor Author

serban300 commented Oct 7, 2024

So just looking at this file, i can confirm that this is not what I intended (as noted, my original issue was not super clear). High level, expect to see something like:

cumulus/polkadot-parachain should stay. This is also crucial for our internal devops to not put a bounty on my head. cumulus/polkadot-omni-node should be a new crate. cumulus/polkadot-parachain-lib -> cumulus/polkadot-omni-node/lib

@kianenigma @skunert I addressed this comment. I had to force push because there were a lot of conflicts. Can you PTAL ? This PR only contains the renamings and also added the polkadot-omni-node binary. I will implement the part about publishing the polkadot-omni-node binary in a different PR. Would like to merge this one as soon as possible to avoid merge conflicts since it moves a lot of files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants