Refactor get_account_id_from_seed / get_from_seed to one common place

[programskillforverification]

Closes #5705

[iulianbarbu]

There are usages of get_account_id_from_seed in templates/parachain/runtime/src/genesis_config_presets.rs, can you update this too?

Not sure what IDE you're using, but if it is using rust-analyzer it might see the errors there if you set WASM_BUILD_CARGO_ARGS="--message-format=json" in the environment before starting your IDE and waiting for a full build/indexing. Also, please mark this PR as ready for review as soon as you changed everything you wanted (merging it with latest master might be a good thing to do before then too).

[programskillforverification]

@iulianbarbu Could you add labels for this pr to make ci happy?

No gear icon is on my all right items.

[iulianbarbu]

LGTM! Thanks @programskillforverification ! 👍

[michalkucharczyk]

How do we know the static value was passed to this function?

This messege for sure needs to be adjusted.

If we don't want to return Option to avoid spreading expects or unwraps around the code, maybe we should rename it to clearly indicate that function will panic if wrong value is passed?
Some ideas: get_from_seed_or_panic or expect_from_seed or maybe ensure_public_from_seed? However I am not sure if that is a good practice.

[michalkucharczyk]

Second thought: the naming is also not accurate. The function is returning the Public part, this shall reflected in the name. Seed has a different meaning within crypto module. I think it should be public_from_string (_or_panic or whatever we choose)?

Maybe this should be a helper, and not a trait member?

[programskillforverification]

Second thought: the naming is also not accurate. The function is returning the Public part, this shall reflected in the name. Seed has a different meaning within crypto module. I think it should be public_from_string (_or_panic or whatever we choose)?

Maybe this should be a helper, and not a trait member?

Yeah, this is a helper. I agree @iulianbarbu that add method on Pair struct. That's the only way to avoid repetition. However, it's hard to find a place to put helper func in sp_core crate. I know it is not good to add it in trait member.

[michalkucharczyk]

I would just move this function to the sp-core/crypto (as the stand-alone function not a method), and rename it to expect_public_from_string (or get_public_from_string_or_panic, or some other more sexy name) to communicate better what the function is actually doing (and highlight that it may panic).

@davxy, what do you think about this proposal?

To give you some context:
We are trying to find a home for a get_from_seed function that is duplicated across many places in the codebase (and this duplication annoys us), e.g.:

polkadot-sdk/cumulus/parachains/common/src/genesis_config_helpers.rs

Lines 25 to 29 in b45f89c

	pub fn get_from_seed<TPublic: Public>(seed: &str) -> <TPublic::Pair as Pair>::Public {
	TPublic::Pair::from_string(&format!("//{seed}"), None)
	.expect("static values are valid; qed")
	.public()
	}

and later used in some other places, e.g:

polkadot-sdk/polkadot/runtime/westend/src/genesis_config_presets.rs

Lines 76 to 80 in b45f89c

	get_from_seed::<BabeId>(seed),
	get_from_seed::<GrandpaId>(seed),
	get_from_seed::<ValidatorId>(seed),
	get_from_seed::<AssignmentId>(seed),
	get_from_seed::<AuthorityDiscoveryId>(seed),

polkadot-sdk/polkadot/runtime/rococo/src/genesis_config_presets.rs

Lines 75 to 79 in b45f89c

	get_from_seed::<BabeId>(seed),
	get_from_seed::<GrandpaId>(seed),
	get_from_seed::<ValidatorId>(seed),
	get_from_seed::<AssignmentId>(seed),
	get_from_seed::<AuthorityDiscoveryId>(seed),

polkadot-sdk/substrate/bin/node/cli/src/chain_spec.rs

Lines 272 to 277 in b45f89c

	get_from_seed::<GrandpaId>(seed),
	get_from_seed::<BabeId>(seed),
	get_from_seed::<ImOnlineId>(seed),
	get_from_seed::<AuthorityDiscoveryId>(seed),
	get_from_seed::<MixnetId>(seed),
	get_from_seed::<BeefyId>(seed),

For me it seems that sp-core/crypto is a good place for this kind of util.

[iulianbarbu]

Yep, my suggestion all along, but realizing now it is not the best 🙈 (sorry for misleading you). The main issues with the approach are:

1. we don't suggest the method can panic - I would add a suffix like _or_panic, even if it sounds less idiomatic.
2. the function name is conflicting with another, and misuses the notion of a seed (haven't looked that much on what Pair trait exposes, my bad) - I would name it from_string_to_public_or_panic (long name, but looks usable and clear). It is also beneficial to scope it under Pair trait. Think about the old function which was found in multiple files that was called get_from_seed<TPair>(s: &str) -> TPair:Public, or which lived (would live) in a helper module (it would need extra scope info in the name to make proper sense, so we would argue a little bit more on the naming).
3. I see it as a combination of constructor/helper/getter (I blame Rust and myself for lack of imagination).

• It either lives in the Pair trait (being somewhat of a non-idiomatic/strange use case helper living in a trait for convenience and easier scoping)
• we create another module somewhere appropriately (sp_core being my first choice right now, but still, a bit hard to reason on this new module, its ownership, and if it is worth creating it with a single function for now).
• or we use the previous convoluted way of doing this without this helper at all, which I am fine with as well, considering the API is clear, and consumers will own the complexity of making their code more compact, until Rust will come with a proper way (if it doesn't exist already and we don't know) for handling such things.

From all above I would still keep the function inside the Pair trait and renamed to not conflict with the seed notion within the Pair trait, and suggest it can panic.

[iulianbarbu]

I find it clear because I know how it was used before. But there is no mention of what this seed is, which is more like a derivation path when compared to the section with the same name under this page: https://docs.substrate.io/reference/command-line-tools/subkey/. Isn't the seed we might be referring to here the one displayed bellow as secret seed?

Secret Key URI `//Alice` is account:
  Secret seed:       0xe5be9a5092b81bca64be81d212e7f2f9eba183bb7a90954f7b76361f6edb5c0a
  Public key (hex):  0xd43593c715fdd31c61141abd04a99fd6822c8558854ccde39a5684e7a56da27d
  Account ID:        0xd43593c715fdd31c61141abd04a99fd6822c8558854ccde39a5684e7a56da27d
  Public key (SS58): 5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY
  SS58 Address:      5GrwvaEF5zXb26Fz9rcQpDWS57CtERHpNehXCPcNoHGKutQY

I think modifying get_from_seed to from_derivation_path_or_panic is clearer.

[michalkucharczyk]

As stated before I would lean towards: sth like from_string_or_panic as it internally uses from_string.

The reason for this is that the string may contain a full address uri in different formats, not just derivation path, so from_derivation_path may not be accurate.

[davxy]

@michalkucharczyk as you suggested a standalone function is sp-core/crypto is reasonable. But why or_panic? Can't we just return a Result?

[michalkucharczyk]

We don't want to copy except all over the places. Usually this function is called with static predefined String, so we know it will work. Getting rid of expect and moving it to the function looks nicer and more compact as function is called many times. See examples I provided in comment.

I personally feel that expect instead of result in this function is fine. We could use ensure_public_from_string or expect_public_from_string if or_panic sounds bad.

[iulianbarbu]

I am also fine with placing the function in core/crypto, but it is still my second option. Not a big deal though and if we find better reasons to do so in the future we can move it then in Pair trait anyways, so lets add it as a standalone function for now 👍 If it comes to naming I would favor any of 'expect_public_from_string' or 'from_string_to_public_or_panic'. It is helpful to add the public word in the name of the function (even if the function return type clarifies what's the end result) because of easier search when autocompleting in an IDE.