paritytech / polkadot-sdk

The Parity Polkadot Blockchain SDK
https://polkadot.com/
1.89k stars 696 forks source link

Refactor `get_account_id_from_seed` / `get_from_seed` to one common place #5705

Closed bkontur closed 1 month ago

bkontur commented 2 months ago

These functions are copied all over the place.

bkontur commented 2 months ago

check @kianenigma's comment here: https://github.com/paritytech/polkadot-sdk/pull/5327#discussion_r1758961989

Keyring::iter() would give the same, but I donno why no one uses it 🤷

programskillforverification commented 2 months ago

I will work on this

michalkucharczyk commented 2 months ago

tagging @iulianbarbu , as I know he is working on similar matter.

iulianbarbu commented 2 months ago

thanks for the tag! I am happy to leave it to @programskillforverification. Leaving here the direction I got based on the comms on this subject lately and my own investigation.

There is a mention of adding sp_keyring as a frame dependency as noted here: https://github.com/paritytech/polkadot-sdk/pull/4739#discussion_r1758825837. My understanding is that the overall suggestion is to re-export sp_keyring::Keyring from frame and use Keyring::iter() instead of the widely used get_account_id_from_seed and friends. The re-export wouldn't be usable without importing frame everywhere we have the duplicated business logic (all places that implement/import get_account_id_from_seed/get_from_seed at least). These places have dependencies on sp-core and sp-runtime on their own, so we might very well remove them and use frame::deps::sp_core/sp_runtime instead where needed (hopefully feature flags will not cause issues here).

programskillforverification commented 1 month ago

thanks for the tag! I am happy to leave it to @programskillforverification. Leaving here the direction I got based on the comms on this subject lately and my own investigation.

There is a mention of adding sp_keyring as a frame dependency as noted here: #4739 (comment). My understanding is that the overall suggestion is to re-export sp_keyring::Keyring from frame and use Keyring::iter() instead of the widely used get_account_id_from_seed and friends. The re-export wouldn't be usable without importing frame everywhere we have the duplicated business logic (all places that implement/import get_account_id_from_seed/get_from_seed at least). These places have dependencies on sp-core and sp-runtime on their own, so we might very well remove them and use frame::deps::sp_core/sp_runtime instead where needed (hopefully feature flags will not cause issues here).

Thanks @iulianbarbu , your investigation help me too much I try to remove all get_account_id_from_seed/get_from_seed

iulianbarbu commented 1 month ago

thanks for the tag! I am happy to leave it to @programskillforverification. Leaving here the direction I got based on the comms on this subject lately and my own investigation. There is a mention of adding sp_keyring as a frame dependency as noted here: #4739 (comment). My understanding is that the overall suggestion is to re-export sp_keyring::Keyring from frame and use Keyring::iter() instead of the widely used get_account_id_from_seed and friends. The re-export wouldn't be usable without importing frame everywhere we have the duplicated business logic (all places that implement/import get_account_id_from_seed/get_from_seed at least). These places have dependencies on sp-core and sp-runtime on their own, so we might very well remove them and use frame::deps::sp_core/sp_runtime instead where needed (hopefully feature flags will not cause issues here).

Thanks @iulianbarbu , your investigation help me too much I try to remove all get_account_id_from_seed/get_from_seed

hey @programskillforverification ! Glad it helps! At this point I am not 100% that the direction is valid (e.g. using frame as a dependency might be too much given its size, even involving features), but we can try it out (or anything else that you conclude based on your research), and keep discussing on the PR. I saw you opened a draft PR which is great, and you can tag people involved in this issue to ask them questions or add as reviewers.

iulianbarbu commented 1 month ago

hey @programskillforverification ! I glanced over your draft #5804 and looks like you're on the right track. I saw there are still a few get_account_id_from_seed occurrences, so I imagine you want to replace those too, but once you feel it is almost good enough for a review, please let us know (add us as reviewers) and we'll take it from there.

programskillforverification commented 1 month ago

hey @programskillforverification ! I glanced over your draft #5804 and looks like you're on the right track. I saw there are still a few get_account_id_from_seed occurrences, so I imagine you want to replace those too, but once you feel it is almost good enough for a review, please let us know (add us as reviewers) and we'll take it from there.

hey @iulianbarbu how can I add you as reviewers? I can't find gear icon in reviews section. Maybe I don't have the permission.

iulianbarbu commented 1 month ago

hey @programskillforverification ! I glanced over your draft #5804 and looks like you're on the right track. I saw there are still a few get_account_id_from_seed occurrences, so I imagine you want to replace those too, but once you feel it is almost good enough for a review, please let us know (add us as reviewers) and we'll take it from there.

hey @iulianbarbu how can I add you as reviewers? I can't find gear icon in reviews section. Maybe I don't have the permission.

You can choose people from the Reviewers section on the right side of the picture below (I added myself so no need to do it for now): Screenshot from 2024-10-01 19-16-45.

LE: Not sure why you can't see the gear on the Reviewers section. I'll make sure we get sufficient eyes on the PR when reviewing it. BTW, if you feel you're good with the changes for now and want us to look over them you should make the PR as ready to review.