better debugging for accountid32 in debug build

[pgherveou]

No description provided.

[pgherveou]

I am also for using debug_assertions instead of a custom "force-debug" flag here to make verbose debug impl the default in debug build.

@bkchr thoughts?

polkadot-sdk/substrate/primitives/debug-derive/src/impls.rs

Lines 61 to 65 in 0c166ae

	#[cfg(any(feature = "std", feature = "force-debug"))]
	mod implementation {
	use super::*;
	use proc_macro2::Span;
	use syn::{token::SelfValue, Ident, Index};

[bkchr]

We should really check the size impact of just having Debug everywhere. And then probably just enable it.

[pgherveou]

We should really check the size impact of just having Debug everywhere. And then probably just enable it.

that's a 8kb change for me when compiling node_template_runtime.compact.wasm size without debug-derive

[liamaharon]

We should really check the size impact of just having Debug everywhere. And then probably just enable it.

that's a 8kb change for me when compiling node_template_runtime.compact.wasm size without debug-derive

what's the % increase on both node-template and rococo-runtime?

[pgherveou]

We should really check the size impact of just having Debug everywhere. And then probably just enable it.

that's a 8kb change for me when compiling node_template_runtime.compact.wasm size without debug-derive

what's the % increase on both node-template and rococo-runtime?

wasm	patched	master	diff	% increase
rococo_runtime.compact.compressed.wasm	1549204	1533452	15752	1.02
node_template_runtime.compact.compressed.wasm	246729	243338	3391	1.39

• used the non compressed before, my bad
• patch is

diff --git a/substrate/primitives/core/src/crypto.rs b/substrate/primitives/core/src/crypto.rs
index 2da38d44be4..58d8616368e 100644
--- a/substrate/primitives/core/src/crypto.rs
+++ b/substrate/primitives/core/src/crypto.rs
@@ -593,14 +593,16 @@ impl std::fmt::Display for AccountId32 {
 }
 
 impl sp_std::fmt::Debug for AccountId32 {
-	#[cfg(feature = "std")]
 	fn fmt(&self, f: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result {
-		let s = self.to_ss58check();
-		write!(f, "{} ({}...)", crate::hexdisplay::HexDisplay::from(&self.0), &s[0..8])
-	}
+		#[cfg(feature = "serde")]
+		{
+			let s = self.to_ss58check();
+			write!(f, "{} ({}...)", crate::hexdisplay::HexDisplay::from(&self.0), &s[0..8])?;
+		}
+
+		#[cfg(not(feature = "serde"))]
+		write!(f, "{}", crate::hexdisplay::HexDisplay::from(&self.0))?;
 
-	#[cfg(not(feature = "std"))]
-	fn fmt(&self, _: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result {
 		Ok(())
 	}
 }
diff --git a/substrate/primitives/debug-derive/src/impls.rs b/substrate/primitives/debug-derive/src/impls.rs
index 76ef8367277..1b971e219ed 100644
--- a/substrate/primitives/debug-derive/src/impls.rs
+++ b/substrate/primitives/debug-derive/src/impls.rs
@@ -43,22 +43,6 @@ pub fn debug_derive(ast: DeriveInput) -> proc_macro::TokenStream {
 	gen.into()
 }
 
-#[cfg(all(not(feature = "std"), not(feature = "force-debug")))]
-mod implementation {
-	use super::*;
-
-	/// Derive the inner implementation of `Debug::fmt` function.
-	///
-	/// Non-std environment. We do nothing to prevent bloating the size of runtime.
-	/// Implement `Printable` if you need to print the details.
-	pub fn derive(_name_str: &str, _data: &Data) -> TokenStream {
-		quote! {
-			fmt.write_str("<wasm:stripped>")
-		}
-	}
-}
-
-#[cfg(any(feature = "std", feature = "force-debug"))]

[liamaharon]

We should really check the size impact of just having Debug everywhere. And then probably just enable it.

@pgherveou I removed from the merge queue because maybe we want to do this instead, since only very small increase to runtime size. @bkchr sound good?

[pgherveou]

We should really check the size impact of just having Debug everywhere. And then probably just enable it.

@pgherveou I removed from the merge queue because maybe we want to do this instead, since only very small increase to runtime size. @bkchr sound good?

fine for me, this can be done in a follow up, but if we are fine with the size bump then we can use this patch instead, or maybe just get rid of debug-derive?

[liamaharon]

Yeah that's what I meant just use your patch then we get it all done in this pr instead of needing to make another one, get reviews again, etc :)

[athei]

We should really check the size impact of just having Debug everywhere. And then probably just enable it.

Agreed. The use of Debug vs RuntimeDebug seems really arbitrary. Do you suggest just getting rid of RuntimeDebug all together if the size impact is not too bad after compression?

[pgherveou]

We should really check the size impact of just having Debug everywhere. And then probably just enable it.

Agreed. The use of Debug vs RuntimeDebug seems really arbitrary. Do you suggest just getting rid of RuntimeDebug all together if the size impact is not too bad after compression?

Happy to add that here if we have consensus.
The most important for us is to have the current patch merged as having AccountId stripped from logs make debugging contracts especially nested contract much harder

[athei]

I am not really sure what is being discussed here. Your PR does derive Debug for the type unconditionally. We can only format ss58 if serde is available. So this is covered. We won't be changing the whole of substrate over to Debug in this PR. So I don't get why it was removed from the merge queue. Can you elaborate @liamaharon ?

[liamaharon]

We won't be changing the whole of substrate over to Debug in this PR.

I must have misunderstood Basti's comment. Feel free to merge if this is ready.

[ggwpez]

We won't be changing the whole of substrate over to Debug in this PR.

Yea we would first have to re-check the size impact again as well.

[athei]

We won't be changing the whole of substrate over to Debug in this PR.

I must have misunderstood Basti's comment. Feel free to merge if this is ready.

I understood it more as a general thing we should do. But not necessarily in this PR. Could be wrong though. Shouldn't stop us here. So let's go on and merge this. Eradication of RuntimeDebug should be a separate PR.

[bkchr]

I understood it more as a general thing we should do

Yes, sorry for not being more specific 🙈

[bkchr]

Created an issue: #3005