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

Remove usage of sp-std from Substrate #7043

Merged
merged 17 commits into from
Jan 7, 2025

Conversation

conr2d
Copy link
Contributor

@conr2d conr2d commented Jan 4, 2025

Description

This PR removes usage of deprecated sp-std from Substrate. (following PR of #5010)

Integration

This PR doesn't remove re-exported sp_std from any crates yet, so downstream projects using re-exported sp_std will not be affected.

Review Notes

The existing code using sp-std is refactored to use alloc and core directly. The key-value maps are instantiated from a vector of tuples directly instead of using sp_std::map! macro.

sp_std::Writer is a helper type to use Vec<u8> with core::fmt::Write trait. This PR copied it into sp-runtime, because all crates using sp_std::Writer (including sp-runtime itself, frame-support, etc.) depend on sp-runtime.

If this PR is merged, I would write following PRs to remove remaining usage of sp-std from bridges and cumulus.

@conr2d conr2d requested a review from a team as a code owner January 4, 2025 16:54
substrate/frame/contracts/proc-macro/src/lib.rs Outdated Show resolved Hide resolved
prdoc/pr_7043.prdoc Outdated Show resolved Hide resolved
prdoc/pr_7043.prdoc Outdated Show resolved Hide resolved
prdoc/pr_7043.prdoc Outdated Show resolved Hide resolved
@jasl
Copy link
Contributor

jasl commented Jan 5, 2025

Great! Thank you!

CC @athei : so conr2d take the job. When these following changes get merged, that issue can be closed.

substrate/frame/system/src/lib.rs Outdated Show resolved Hide resolved
@@ -1051,6 +1051,29 @@ pub use alloc::borrow::Cow;
#[deprecated = "Use String or Cow<'static, str> instead"]
pub type RuntimeString = alloc::string::String;

/// A target for `core::write!` macro - constructs a string in memory.
#[derive(Default)]
pub struct Writer(vec::Vec<u8>);
Copy link
Member

Choose a reason for hiding this comment

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

We could just replace the usage of writer with alloc::string::String to no require a custom type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's great to hear! My concern, however, is that, as far as I know, using alloc::string::String is discouraged to avoid bloating the runtime size.

@@ -650,7 +650,7 @@ fn expand_functions(def: &EnvDef, expand_mode: ExpandMode) -> TokenStream2 {
let result = #body;
if ::log::log_enabled!(target: "runtime::contracts::strace", ::log::Level::Trace) {
use core::fmt::Write;
let mut w = sp_std::Writer::default();
let mut w = sp_runtime::Writer::default();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let mut w = sp_runtime::Writer::default();
let mut w = alloc::string::String::default();

@bkchr bkchr added the T17-primitives Changes to primitives that are not covered by any other label. label Jan 5, 2025
prdoc/pr_7043.prdoc Outdated Show resolved Hide resolved
prdoc/pr_7043.prdoc Outdated Show resolved Hide resolved
@bkchr
Copy link
Member

bkchr commented Jan 6, 2025

/cmd prdoc --bump patch --audience runtime_dev --force

prdoc/pr_7043.prdoc Outdated Show resolved Hide resolved
@gui1117 gui1117 enabled auto-merge January 6, 2025 10:27
@gui1117 gui1117 disabled auto-merge January 6, 2025 10:28
Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

Can you also remove sp_std from the Cargo.toml of contracts and revive? It is no longer used there now.

@conr2d conr2d requested a review from koute as a code owner January 6, 2025 12:25
@bkchr bkchr enabled auto-merge January 6, 2025 20:45
@athei athei requested a review from pgherveou January 6, 2025 23:51
@bkchr bkchr added this pull request to the merge queue Jan 7, 2025
Merged via the queue into paritytech:master with commit c139739 Jan 7, 2025
199 of 203 checks passed
ordian added a commit that referenced this pull request Jan 7, 2025
* master:
  workflows: add debug input for sync templates act (#7057)
  Remove usage of `sp-std` from Substrate (#7043)
  Fix typos (#7027)
  [core-fellowship] Add permissionless import_member (#7030)
  Avoid incomplete block import pipeline with full verifying import queue (#7050)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T17-primitives Changes to primitives that are not covered by any other label.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants