Remove `sp_runtime::RuntimeString` and replace with `Cow<'static, str>` or `String` depending on use case #5693

nazar-pc · 2024-09-12T15:36:26Z

Description

As described in #4001 RuntimeVersion was not encoded consistently using serde. Turned out it was a remnant of old times and no longer actually needed. As such I removed it completely in this PR and replaced with Cow<'static, str> for spec/impl names and String for error cases.

Fixes #4001.

Integration

For downstream projects the upgrade will primarily consist of following two changes:

#[sp_version::runtime_version]
pub const VERSION: RuntimeVersion = RuntimeVersion {
-	spec_name: create_runtime_str!("statemine"),
-	impl_name: create_runtime_str!("statemine"),
+	spec_name: alloc::borrow::Cow::Borrowed("statemine"),
+	impl_name: alloc::borrow::Cow::Borrowed("statemine"),

		fn dispatch_benchmark(
			config: frame_benchmarking::BenchmarkConfig
-		) -> Result<Vec<frame_benchmarking::BenchmarkBatch>, sp_runtime::RuntimeString> {
+		) -> Result<Vec<frame_benchmarking::BenchmarkBatch>, alloc::string::String> {

SCALE encoding/decoding remains the same as before, but serde encoding in runtime has changed from bytes to string (it was like this in std environment already), which most projects shouldn't have issues with. I consider the impact of serde encoding here low due to the type only being used in runtime version struct and mostly limited to runtime internals, where serde encoding/decoding of this data structure is quite unlikely (though we did hit exactly this edge-case ourselves 😅).

Review Notes

Most of the changes are trivial and mechanical, the only non-trivial change is in substrate/primitives/version/proc-macro/src/decl_runtime_version.rs where macro call expectation in sp_version::runtime_version implementation was replaced with function call expectation.

Checklist

My PR includes a detailed description as outlined in the "Description" and its two subsections above.
My PR follows the labeling requirements of this project (at minimum one label for T required)
- External contributors: ask maintainers to put the right label on your PR.
I have made corresponding changes to the documentation (if applicable)

nazar-pc · 2024-09-12T16:08:00Z

I do not understand why formatter in CI wants those trailing whitespaces, I'm unable to commit them for some reason in IDE.

UPD: nano did it.

…>` or `String` depending on use case

cumulus/parachains/runtimes/starters/shell/src/lib.rs

…>` or `String` depending on use case (paritytech#5693)

# Conflicts: # cumulus/parachains/runtimes/assets/asset-hub-rococo/src/genesis_config_presets.rs # polkadot/runtime/rococo/src/genesis_config_presets.rs # templates/parachain/runtime/src/genesis_config_presets.rs

nazar-pc · 2024-09-27T13:14:04Z

Fixed merge conflicts and restored create_runtime_str

nazar-pc · 2024-10-18T16:07:35Z

Anything I can do to move this PR forward? It seems to just accumulate merge conflicts over time.

substrate/primitives/runtime/src/runtime_string.rs

gui1117 · 2024-11-04T11:05:41Z

substrate/primitives/runtime/src/lib.rs

+		$crate::Cow::Borrowed($y)
+	}};
+}
+// Re-export for ^ macro, should be removed once macro is gone
+#[doc(hidden)]
+pub use alloc::borrow::Cow;


This doesn't work?

Suggested change

$crate::Cow::Borrowed($y)

}};

}

// Re-export for ^ macro, should be removed once macro is gone

#[doc(hidden)]

pub use alloc::borrow::Cow;

$crate::alloc::borrow::Cow::Borrowed($y)

}};

}

Even better to wrap it in a mod __private module, so others dont assume that it is part of the public API.

$crate::alloc::borrow::Cow::Borrowed($y) does not work because alloc is a private import of the crate.

I only did it like this originally due similar example code on line 52 above. I could move both into __private, but I wanted to keep it close to the now deprecated macro. Let me know if you have a strong opinion about this one or feel free to push changes directly.

Ah yes, you can also have a __private_create_runtime_str module if you want to keep stuff together.

It is good so user don't start using it in when it is recommended by rustc.

Anyway it is fine.

substrate/primitives/version/proc-macro/src/decl_runtime_version.rs

gui1117 · 2024-11-04T12:01:32Z

/cmd prdoc --bump major --audience runtime_dev

…ntime_dev'

prdoc/pr_5693.prdoc

bkchr · 2024-11-04T13:15:35Z

cumulus/polkadot-parachain/polkadot-parachain-lib/src/fake_runtime_api/utils.rs

@@ -202,7 +202,7 @@ macro_rules! impl_node_runtime_apis {

 				fn dispatch_benchmark(
 					_: frame_benchmarking::BenchmarkConfig
-				) -> Result<Vec<frame_benchmarking::BenchmarkBatch>, sp_runtime::RuntimeString> {
+				) -> Result<Vec<frame_benchmarking::BenchmarkBatch>, alloc::string::String> {


Suggested change

) -> Result<Vec<frame_benchmarking::BenchmarkBatch>, alloc::string::String> {

) -> Result<Vec<frame_benchmarking::BenchmarkBatch>, String> {

This code is never compiled for no_std.

It actually is used like that in CI, had to revert this suggestion

cumulus/polkadot-parachain/polkadot-parachain-lib/src/fake_runtime_api/mod.rs

# Conflicts: # cumulus/parachains/runtimes/starters/seedling/src/lib.rs # cumulus/parachains/runtimes/starters/shell/src/lib.rs # cumulus/test/runtime/src/lib.rs # substrate/test-utils/runtime/src/lib.rs

nazar-pc

Applied review suggestions improving upgrade experience and resolved merge conflicts

substrate/primitives/runtime/src/runtime_string.rs

nazar-pc · 2024-11-05T04:21:51Z

substrate/primitives/runtime/src/lib.rs

+		$crate::Cow::Borrowed($y)
+	}};
+}
+// Re-export for ^ macro, should be removed once macro is gone
+#[doc(hidden)]
+pub use alloc::borrow::Cow;


$crate::alloc::borrow::Cow::Borrowed($y) does not work because alloc is a private import of the crate.

I only did it like this originally due similar example code on line 52 above. I could move both into __private, but I wanted to keep it close to the now deprecated macro. Let me know if you have a strong opinion about this one or feel free to push changes directly.

gui1117 · 2024-11-05T08:58:48Z

needs to fix CI:

error[E0433]: failed to resolve: use of undeclared crate or module `alloc`
   --> cumulus/polkadot-omni-node/lib/src/fake_runtime_api/utils.rs:205:58
    |
205 |                 ) -> Result<Vec<frame_benchmarking::BenchmarkBatch>, alloc::string::String> {
    |                                                                      ^^^^^ use of undeclared crate or module `alloc`
    |
   ::: cumulus/polkadot-omni-node/lib/src/fake_runtime_api/mod.rs:29:2
    |
29  |     impl_node_runtime_apis!(FakeRuntime, CustomBlock, sp_consensus_aura::sr25519::AuthorityId);
    |     ------------------------------------------------------------------------------------------ in this macro invocation
    |
    = help: add `extern crate alloc` to use the `alloc` crate
    = note: this error originates in the macro `impl_node_runtime_apis` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider importing this module
   --> cumulus/polkadot-omni-node/lib/src/fake_runtime_api/mod.rs:26:2
    |
26  +     use std::string;
    |

error[E0433]: failed to resolve: use of undeclared crate or module `alloc`
   --> cumulus/polkadot-omni-node/lib/src/fake_runtime_api/utils.rs:205:58
    |
205 |                 ) -> Result<Vec<frame_benchmarking::BenchmarkBatch>, alloc::string::String> {
    |                                                                      ^^^^^ use of undeclared crate or module `alloc`
    |
   ::: cumulus/polkadot-omni-node/lib/src/fake_runtime_api/mod.rs:36:2
    |
36  |     impl_node_runtime_apis!(FakeRuntime, CustomBlock, sp_consensus_aura::ed25519::AuthorityId);
    |     ------------------------------------------------------------------------------------------ in this macro invocation
    |
    = help: add `extern crate alloc` to use the `alloc` crate
    = note: this error originates in the macro `impl_node_runtime_apis` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider importing this module
   --> cumulus/polkadot-omni-node/lib/src/fake_runtime_api/mod.rs:33:2
    |
33  +     use std::string;
    |

nazar-pc · 2024-11-05T15:20:06Z

Right, that is why it was in there from the beginning, bringing it back too

nazar-pc requested review from sorpaas, a team and koute as code owners September 12, 2024 15:36

nazar-pc mentioned this pull request Sep 12, 2024

serde for RuntimeString is broken #4001

Closed

2 tasks

nazar-pc force-pushed the remove-RuntimeString branch 2 times, most recently from f0df22e to 308d2a9 Compare September 12, 2024 16:02

nazar-pc force-pushed the remove-RuntimeString branch 2 times, most recently from 096e2d6 to f3778c1 Compare September 12, 2024 16:30

Remove sp_runtime::RuntimeString and replace with `Cow<'static, str…

d9de518

…>` or `String` depending on use case

nazar-pc force-pushed the remove-RuntimeString branch from f3778c1 to d9de518 Compare September 12, 2024 18:55

bkchr reviewed Sep 15, 2024

View reviewed changes

cumulus/parachains/runtimes/starters/shell/src/lib.rs Outdated Show resolved Hide resolved

michalkucharczyk mentioned this pull request Sep 25, 2024

sp_genesis_builder::PresetId: improvement #5532

Open

nazar-pc added a commit to autonomys/polkadot-sdk that referenced this pull request Sep 27, 2024

Remove sp_runtime::RuntimeString and replace with `Cow<'static, str…

fc759a1

…>` or `String` depending on use case (paritytech#5693)

nazar-pc added 2 commits September 27, 2024 15:49

Merge remote-tracking branch 'upstream/master' into remove-RuntimeString

85b42fb

# Conflicts: # cumulus/parachains/runtimes/assets/asset-hub-rococo/src/genesis_config_presets.rs # polkadot/runtime/rococo/src/genesis_config_presets.rs # templates/parachain/runtime/src/genesis_config_presets.rs

Restore sp_runtime::create_runtime_str in deprecated form

f4397cc

nazar-pc requested a review from bkchr September 27, 2024 13:14

nazar-pc mentioned this pull request Sep 30, 2024

Upgrade Substrate to stable2409 autonomys/subspace#3078

Merged

1 task

gui1117 reviewed Nov 4, 2024

View reviewed changes

gui1117 added the T17-primitives Changes to primitives that are not covered by any other label. label Nov 4, 2024

Update from gui1117 running command 'prdoc --bump major --audience ru…

bc037ed

…ntime_dev'

gui1117 reviewed Nov 4, 2024

View reviewed changes

prdoc/pr_5693.prdoc Outdated Show resolved Hide resolved

Update prdoc/pr_5693.prdoc

3aca5d3

ggwpez reviewed Nov 4, 2024

View reviewed changes

prdoc/pr_5693.prdoc Outdated Show resolved Hide resolved

bkchr approved these changes Nov 4, 2024

View reviewed changes

nazar-pc added 2 commits November 5, 2024 06:55

Address review comments

ae0d77f

Merge remote-tracking branch 'upstream/master' into remove-RuntimeString

b700516

# Conflicts: # cumulus/parachains/runtimes/starters/seedling/src/lib.rs # cumulus/parachains/runtimes/starters/shell/src/lib.rs # cumulus/test/runtime/src/lib.rs # substrate/test-utils/runtime/src/lib.rs

nazar-pc commented Nov 5, 2024

View reviewed changes

nazar-pc requested review from gui1117 and ggwpez November 5, 2024 05:04

nazar-pc added 2 commits November 5, 2024 07:05

Remove crates that no longer exist from prdoc

5cdffd4

Fix CI

00ca444

gui1117 approved these changes Nov 5, 2024

View reviewed changes

nazar-pc and others added 2 commits November 5, 2024 17:22

Fix runtime benchmarks compilation in omni node

50a07a7

Only remove where I said

635e556

bkchr enabled auto-merge November 5, 2024 21:26

bkchr added this pull request to the merge queue Nov 5, 2024

Merged via the queue into paritytech:master with commit c5444f3 Nov 5, 2024
191 of 194 checks passed

nazar-pc deleted the remove-RuntimeString branch November 12, 2024 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `sp_runtime::RuntimeString` and replace with `Cow<'static, str>` or `String` depending on use case #5693

Remove `sp_runtime::RuntimeString` and replace with `Cow<'static, str>` or `String` depending on use case #5693

nazar-pc commented Sep 12, 2024 •

edited

Loading

nazar-pc commented Sep 12, 2024 •

edited

Loading

nazar-pc commented Sep 27, 2024

nazar-pc commented Oct 18, 2024

gui1117 Nov 4, 2024

ggwpez Nov 4, 2024

nazar-pc Nov 5, 2024

gui1117 Nov 5, 2024

gui1117 commented Nov 4, 2024

bkchr Nov 4, 2024

nazar-pc Nov 5, 2024

nazar-pc left a comment

nazar-pc Nov 5, 2024

gui1117 commented Nov 5, 2024

nazar-pc commented Nov 5, 2024

	) -> Result<Vec<frame_benchmarking::BenchmarkBatch>, alloc::string::String> {
	) -> Result<Vec<frame_benchmarking::BenchmarkBatch>, String> {

Remove sp_runtime::RuntimeString and replace with Cow<'static, str> or String depending on use case #5693

Remove sp_runtime::RuntimeString and replace with Cow<'static, str> or String depending on use case #5693

Conversation

nazar-pc commented Sep 12, 2024 • edited Loading

Description

Integration

Review Notes

Checklist

nazar-pc commented Sep 12, 2024 • edited Loading

nazar-pc commented Sep 27, 2024

nazar-pc commented Oct 18, 2024

gui1117 Nov 4, 2024

Choose a reason for hiding this comment

ggwpez Nov 4, 2024

Choose a reason for hiding this comment

nazar-pc Nov 5, 2024

Choose a reason for hiding this comment

gui1117 Nov 5, 2024

Choose a reason for hiding this comment

gui1117 commented Nov 4, 2024

bkchr Nov 4, 2024

Choose a reason for hiding this comment

nazar-pc Nov 5, 2024

Choose a reason for hiding this comment

nazar-pc left a comment

Choose a reason for hiding this comment

nazar-pc Nov 5, 2024

Choose a reason for hiding this comment

gui1117 commented Nov 5, 2024

nazar-pc commented Nov 5, 2024

Remove `sp_runtime::RuntimeString` and replace with `Cow<'static, str>` or `String` depending on use case #5693

Remove `sp_runtime::RuntimeString` and replace with `Cow<'static, str>` or `String` depending on use case #5693

nazar-pc commented Sep 12, 2024 •

edited

Loading

nazar-pc commented Sep 12, 2024 •

edited

Loading