Feature: Pallet Engine (Add pallet - MVP) #43

weezy20 · 2024-03-08T08:19:16Z

Enable pop-cli to add pallets to any runtime.
pop add pallet template -r <runtime_path> or pop a p template -r <runtime_path>

sub0 Todos:

add pallet subsubcommand
add pallet-template to runtime (chainspec edits not required)
unit tests to check pallet-template
implement dependency injection (runtime / node)
unit tests for dependency injection
implement minimal support for FRAME (chainspec + runtime)

M1 Todos:

implement name conflict resolution strategy (Branch add-pallet needs to check for duplicated entries. #63)
conditional features for dependency
optional runtime path, implement store to cache results from previous invocations of pop new parachain to use as a runtime path
implement diff tool to monitor edits before committing

Milestone 2 :
Enable full FRAME pallet support

todo

Nice to haves:

Automatic runtime and node manifest path resolution, based on cache populated during pop new parachain
Interactive pallet-template customization on pop add pallet template

AlexD10S · 2024-03-09T09:39:20Z

Do you want me to review and merge now or when all sub0 TODO tasks are completed?

We should create a separate issue to track M1 and M2 tasks

Also suggest a change for the name of the PR to follow this conventions https://www.conventionalcommits.org/es/v1.0.0-beta.2/ A suggestion will be feat: pop add pallet

weezy20 · 2024-03-09T09:42:56Z

@AlexD10S This should be ready for merging when a few more tasks out of sub0 is completed.

src/engines/pallet_engine/dependency.rs

src/commands/add/mod.rs

src/engines/pallet_engine/mod.rs

brunopgalvao · 2024-03-20T10:29:07Z

src/engines/pallet_engine/steps.rs

+			// NodePalletDependency(dep) => te.inject_node(dep)?,
+			// ListBenchmarks(step) => pe.insert(step),
+			// ListBenchmarks(step) => pe.insert(step),
+			// ChainspecGenesisConfig(step) => pe.insert(step),
+			// ChainspecGenesisImport(step) => pe.insert(step),


Their implementation is required later.

src/engines/pallet_engine/template.rs

Daanvdplas · 2024-03-20T12:56:59Z

First thing I notice is that the pallet generated from pop new pallet isn't used for adding it. I can run pop add pallet template without it?

weezy20 · 2024-03-20T13:47:39Z

First thing I notice is that the pallet generated from pop new pallet isn't used for adding it. I can run pop add pallet template without it?

Correct. The feature is not implemented yet but is up for discussion here : #71

Daanvdplas

We could potentially add the pallet to the runtime/src/lib.rs commented, then the user has to fill it in the Config items themselves. This would allow to take any pallet config and copy paste it in the runtime. The DefaultConfig transition is something that can be very beneficial to this feature: Adds default config for assets pallet paritytech/polkadot-sdk#3637

example: https://github.com/paritytech/polkadot-sdk/pull/3637/files

With this, the user would only have to implement the left over Config items not handled by the DefaultConfig.

let the user specify the pallet index to use in the runtime:
pop add pallet frame or pop add pallet --path <path to runtime>
You very specifically explain what is happening in the code, very nice. However, it would also be nice if you could explain it in a more human understandable way.

src/engines/pallet_engine/mod.rs

Daanvdplas · 2024-03-20T13:49:21Z

src/engines/pallet_engine/steps.rs

+			steps.push(RuntimePalletImport((
+				quote!(
+					// Imports by pop-cli
+					pub use pallet_parachain_template;


The template that is created has a name. Could we name the pallet like that?

pop new pallet test will result here into pallet_test

So, what you're looking at here is code for adding the default pallet-parachain-template that lives on cumulus (and subsequently, on the pallet template used by pop new pallet template). There is no support yet to handle custom pallets created by pop new or FRAME.

Yet but is it not meant for adding a pallet that the user created? Don't see much reason for adding the pallet template. Someone creates a new pallet pop new pallet awesome_pallet and then someone wants to add that pallet to the runtime pop add pallet --path pallets/awesome_pallet

Reading also the other comments I made regarding this topic. What is there for value in adding the pallet_parachain_template? The user will then have to change the name themselves everywhere

Daanvdplas · 2024-03-20T13:51:25Z

src/engines/pallet_engine/steps.rs

+			steps.push(SwitchTo(State::Config));
+			steps.push(RuntimePalletConfiguration((
+				quote!(
+					/// Configure the pallet template in pallets/template.


"Configure pallet test in pallets/test."

Same for all the other places where this will have an effect.

See comment above please.

Daanvdplas · 2024-03-20T13:52:06Z

src/engines/pallet_engine/steps.rs

+			steps.push(RuntimePalletConfiguration((
+				quote!(
+					/// Configure the pallet template in pallets/template.
+					impl pallet_parachain_template::Config for Runtime {


TODO: this should be read from the pallet lib.rs

See above please. Same reason as above.

Daanvdplas · 2024-03-21T06:53:44Z

src/engines/pallet_engine/mod.rs

+		// Build Pallet Details
+		let __buf = BufReader::new(File::open(&input)?);
+		let file_end = __buf.lines().count();
+		let rt = fs::read_to_string(&input)?;


Maybe this is me but I find this naming confusing, what does it stand for? Multiple cases in the code, any reason why besides shorter?

rt -> runtime.

Agree with Daan here, this part is not easy to follow. Clear naming or take some lines into specific function might help

Daanvdplas · 2024-03-21T07:13:11Z

src/engines/pallet_engine/mod.rs

+	/// Add a new pallet to RuntimeDeclaration and return it.
+	/// Used to pass a typed AddPallet to modify the RuntimeDeclaration
+	/// Overwrites existing CRT in output, this means that `self.cursor` has to backtracked to crt_start
+	/// and updated after the pallet entry has been inserted


Sorry but I simply don't understand what you are saying here :)

You explain what is happening in the code very detailed, well done! However, it would make the reviewer's work, and anyone diving into the code, much more effective and easy if you could also add a high level explanation of what the code / function does. As a result, the reviewer can get a high level understanding of what the function is doing / meant to do before going going over the code in detail.

It would be really nice if you could document your code in a way where:

First, you explain high level what this function does, or what the code from now on (when following the code) is about to do. in a way where a (parachain) developer understands what is about to happen and why.

Second, provide additional detailed code explanation (this you already have).

Yeah for this case, I should explain it better. So the commented code in add_pallet_runtime is altogether a different system that I had created during prototyping of pallet engine to add pallets to the CRT macro. What this does is effectively use the structure RuntimeDeclaration that's parsed from the source file, append to it with more checks such as indicies, instances, etc. (some of which have no implementation right now but are put off until we dive into handling duplicated entries), and update the tokens for the CRT macro in place in the output file.

weezy20 · 2024-03-21T07:41:10Z

We could potentially add the pallet to the runtime/src/lib.rs commented, then the user has to fill it in the Config items themselves. This would allow to take any pallet config and copy paste it in the runtime. The DefaultConfig transition is something that can be very beneficial to this feature: Adds default config for assets pallet paritytech/polkadot-sdk#3637

example: https://github.com/paritytech/polkadot-sdk/pull/3637/files

With this, the user would only have to implement the left over Config items not handled by the DefaultConfig.

This is a great idea. Would probably come to fruition when we start work on adding FRAME support.

let the user specify the pallet index to use in the runtime:

This can be done as the machinery exists already, just not in the CLI. One thing to note here is that although unimplemented currently, when we start working on unique pallet additions or iow, a name conflict resolution strategy, pallet additions would automatically be indexed, respecting CRT checks such as the requirement for the index to be u8 and respecting the pallet order.

pop add pallet frame or pop add pallet --path <path to runtime>

You very specifically explain what is happening in the code, very nice. However, it would also be nice if you could explain it in a more human understandable way.

I'll try my best. Thank you.

…time manifest

AlexD10S · 2024-03-21T10:19:11Z

First thing I notice is that the pallet generated from pop new pallet isn't used for adding it. I can run pop add pallet template without it?

About this discussion I don't have it very clear.
If I run pop add pallet template without having run the pop new pallet before parachain doesn't build, because it adds by default in the Cargo.toml a path that doesn't exist:

pallet-parachain-template = { path = "/pathtoparachain/pallets/pallet-parachain-template", version = "1.0.0", default-features = false }

Is that expected? For me is a bug, we should do automatically the pop new pallet or show an error.
We should discuss the issue for this PR: #71

As per https://crates.io/crates/tempdir, tempdir was deprecated ages ago with functionality moved into tempfile which already exists as a dependency. Primary motivation was a security advisory triggered by dependabot.

weezy20 · 2024-03-21T10:29:38Z

First thing I notice is that the pallet generated from pop new pallet isn't used for adding it. I can run pop add pallet template without it?

About this discussion I don't have it very clear. If I run pop add pallet template without having run the pop new pallet before parachain doesn't build, because it adds by default in the Cargo.toml a path that doesn't exist:
pallet-parachain-template = { path = "/pathtoparachain/pallets/pallet-parachain-template", version = "1.0.0", default-features = false }
Is that expected? For me is a bug, we should do automatically the pop new pallet or show an error. We should discuss the issue for this PR: #71

This is known, not expected at all and is a bug. Do you recommend we address #71 immediately @AlexD10S ?

src/commands/add/mod.rs

AlexD10S · 2024-03-21T07:56:38Z

src/commands/add/mod.rs

+	/// Insert `pallet-parachain-template` into the runtime.
+	Template,
+	/// Insert a frame-pallet into the runtime.
+	Frame(FrameArgs),


I personally don’t like to have the FRAME logic there if is not working. It might be confusing for the user.

Why would the user be confused when they're met with a not implemented message ? If I delete this scaffolding code which is minimal btw, I will have to rewrite it when we start with Frame which we are going to do anyways.

With git there is no need to rewrite things, just take it from here into another branch.

Sorry I have to disagree with that approach. Doesn't solve anything.

AlexD10S · 2024-03-21T08:04:43Z

src/commands/add/mod.rs

+				}
+				path
+			},
+			None => {


For UX, Is not better to use clap errors?. Will be add runtime_path as mandatory above

If i run cargo run add pallet template it panics

thread 'main' panicked at src/commands/add/mod.rs:62:17: not implemented: provide a runtime path until feat:cache is implemented: --runtime <path> note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

If I run cargo run up parachain without the mandatory commands it shows me how to use the tool:

error: the following required arguments were not provided: --file <FILE> Usage: pop up parachain --file <FILE> For more information, try '--help'.

I agree. This makes more sense when we have a functional cache.. Just for the record,reason to use an Option here is to rely on automatic inference via the pop new cache in the case that user omits -r. But yeah seems right to make it required

AlexD10S · 2024-03-21T08:05:31Z

src/commands/add/mod.rs

+				path
+			},
+			None => {
+				// TODO: Fetch runtime either from cache


For TODOs is better to create an Issue, than lines of code

Some of it is already in the issue and PR description. These are just placeholder comments from where future work can be easily picked up where these were left off.

AlexD10S · 2024-03-21T09:53:49Z

src/engines/pallet_engine/mod.rs

+
+/// The main entry point into the engine.
+pub fn execute(pallet: AddPallet, runtime_path: PathBuf) -> anyhow::Result<()> {
+	// Todo: Add option to source from cli


Not very clear

What do you mean?

AlexD10S · 2024-03-21T11:01:30Z

src/engines/pallet_engine/template.rs

 pub fn create_pallet_template(
 	path: Option<String>,
 	config: TemplatePalletConfig,
 ) -> anyhow::Result<()> {
 	let target = resolve_pallet_path(path);
+	// TODO : config.name might use `-` or use snake_case. We want to use pallet_template for the pallet dirs


Why adding it?

Don't think it's necessary anymore. Will remove.

AlexD10S · 2024-03-21T11:02:25Z

src/engines/pallet_engine/parser.rs

@@ -0,0 +1,913 @@
+#![allow(unused)]
+// This file is part of Substrate modified to fit pallet manipulation needs for pop-cli


Do we use everything from here?

Not yet. This is important when a future implementation will use in place editing of RuntimeDeclaration rather than inserting strings. It's basically the entire parser for construct_runtime!

AlexD10S · 2024-03-21T11:04:27Z

src/engines/pallet_engine/mod.rs

+	}
+	/// Create a new PalletEngine
+	pub fn new(input: &PathBuf) -> anyhow::Result<Self> {
+		let tmp_dir = PathBuf::from(format!("/tmp/pallet_engine_{}", uuid::Uuid::new_v4()));


Is not better to create and remove the /tmp/pallet_engine than having to add a uuid?

How would you prevent name collisions?

but the tmp folder is removed after no?

No it's not. At least not in code. We are relying on linux system to clean it up by placing it under /tmp on reboot or on non-usage.

AlexD10S · 2024-03-21T11:13:18Z

templates/pallet/Cargo.templ

-frame-benchmarking = { version = "26.0.0", default-features = false, optional = true}
-frame-support = { version = "26.0.0", default-features = false}
-frame-system = { version = "26.0.0", default-features = false}
+frame-benchmarking = { git = "https://github.com/paritytech/polkadot-sdk", branch = "release-polkadot-v1.7.1", default-features = false }


I added this line to make it work for Sub0, but is there another way?

Otherwise we will have to always update it here when updating base-parachain: https://github.com/r0gue-io/base-parachain/blob/main/Cargo.toml

This entire template needs to be updated to use release-polkadot-* dependencies. Cannot have the base-parachain using them and this one using crates-io dependencies.

On second thought, I think you've made the appropriate changes already.

tests/add.rs

AlexD10S · 2024-03-21T12:00:30Z

First thing I notice is that the pallet generated from pop new pallet isn't used for adding it. I can run pop add pallet template without it?

About this discussion I don't have it very clear. If I run pop add pallet template without having run the pop new pallet before parachain doesn't build, because it adds by default in the Cargo.toml a path that doesn't exist:
pallet-parachain-template = { path = "/pathtoparachain/pallets/pallet-parachain-template", version = "1.0.0", default-features = false }
Is that expected? For me is a bug, we should do automatically the pop new pallet or show an error. We should discuss the issue for this PR: #71
This is known, not expected at all and is a bug. Do you recommend we address #71 immediately @AlexD10S ?

Before merge to main yes

weezy20 added 8 commits March 4, 2024 11:41

init: Pallet-engine

5f60e9a

Merge branch 'main' into add-pallet

3539460

tidy

ee058b3

step_builder for pallet-engine

e7b791a

insert_import_stmts

c3dcfa0

Merge branch 'main' into add-pallet

60cee28

fix build errors

0cfec78

prepare_config

685f3c7

weezy20 force-pushed the add-pallet branch from dcf82e4 to bbbd65f Compare March 8, 2024 10:26

fix more runtime errors

6dd21d1

weezy20 force-pushed the add-pallet branch from bbbd65f to 6dd21d1 Compare March 8, 2024 10:29

weezy20 added 3 commits March 8, 2024 18:12

Merge branch 'main' into add-pallet

4214d2c

update lockfile

7e81cc2

minor adjustments

69d9491

weezy20 force-pushed the add-pallet branch from 72e4bfc to 69d9491 Compare March 8, 2024 13:10

weezy20 added 7 commits March 8, 2024 19:01

prepare test harness for add-pallet

2e6aef6

Merge branch 'main' into add-pallet

bc788de

test : add_parachain_pallet_template

83d2d2c

improve test

cc3ca54

style command

0d40634

tweaks for correct cursor tracking

736deb4

default to use insert_str_runtime for implicit runtime decl entries.

85a7e47

weezy20 force-pushed the add-pallet branch from 3ed2a26 to 85a7e47 Compare March 9, 2024 08:44

Merge branch 'main' into add-pallet

e4eb10f

weezy20 marked this pull request as ready for review March 9, 2024 09:08

weezy20 requested review from AlexD10S and brunopgalvao March 9, 2024 09:08

typo

3e2c150

brunopgalvao reviewed Mar 20, 2024

View reviewed changes

weezy20 added 2 commits March 20, 2024 16:07

review fixes

215b028

review fixes 2

354116e

weezy20 force-pushed the add-pallet branch from 8c09b7e to 354116e Compare March 20, 2024 10:43

Daanvdplas reviewed Mar 21, 2024

View reviewed changes

weezy20 added 2 commits March 21, 2024 13:05

review fixes 3 + fix add_new_line cursor

99f4b64

Merge branch 'main' into add-pallet

0e682f5

weezy20 added 3 commits March 21, 2024 13:36

use pallet-parachain-template path relative to workspace root

159b998

fix order for pallet-engine construction

4309bb2

include checks for try-runtime and runtime-benchmarks features in run…

0363844

…time manifest

fix(deps): remove deprecated dependency

7e7478d

As per https://crates.io/crates/tempdir, tempdir was deprecated ages ago with functionality moved into tempfile which already exists as a dependency. Primary motivation was a security advisory triggered by dependabot.

weezy20 force-pushed the add-pallet branch from 0a245ef to 7e7478d Compare March 21, 2024 10:27

AlexD10S requested changes Mar 21, 2024

View reviewed changes

weezy20 added 8 commits March 22, 2024 10:54

remove todo comment for name-conflict-resolution-strategy

6f7ddd8

remove commented code

a939826

add help msg for add pallet

0754e6a

Merge branch 'main' into add-pallet

75fceb9

Merge branch 'main' into add-pallet

e93dc0c

fmt

44eed89

Merge branch 'main' into add-pallet

dd05112

rename test

e6733e5

weezy20 force-pushed the add-pallet branch from 9f55c5e to e6733e5 Compare April 8, 2024 07:31

weezy20 closed this Apr 25, 2024

weezy20 deleted the add-pallet branch June 27, 2024 08:04

		@@ -0,0 +1,913 @@
		#![allow(unused)]
		// This file is part of Substrate modified to fit pallet manipulation needs for pop-cli

Feature: Pallet Engine (Add pallet - MVP) #43

Feature: Pallet Engine (Add pallet - MVP) #43

Conversation

weezy20 commented Mar 8, 2024 • edited Loading

AlexD10S commented Mar 9, 2024 • edited Loading

weezy20 commented Mar 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daanvdplas commented Mar 20, 2024 • edited Loading

weezy20 commented Mar 20, 2024

Daanvdplas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weezy20 Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

Daanvdplas Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daanvdplas Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

weezy20 Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

weezy20 commented Mar 21, 2024 • edited Loading

AlexD10S commented Mar 21, 2024

weezy20 commented Mar 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weezy20 Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weezy20 Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weezy20 Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexD10S commented Mar 21, 2024

weezy20 commented Mar 8, 2024 •

edited

Loading

AlexD10S commented Mar 9, 2024 •

edited

Loading

Daanvdplas commented Mar 20, 2024 •

edited

Loading

weezy20 Mar 21, 2024 •

edited

Loading

Daanvdplas Mar 22, 2024 •

edited

Loading

Daanvdplas Mar 21, 2024 •

edited

Loading

weezy20 Mar 21, 2024 •

edited

Loading

weezy20 commented Mar 21, 2024 •

edited

Loading

weezy20 Mar 22, 2024 •

edited

Loading

weezy20 Mar 22, 2024 •

edited

Loading

weezy20 Mar 22, 2024 •

edited

Loading