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.

Sign up for GitHub

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

Jump to bottom

Refactor get_account_id_from_seed / get_from_seed to one common place #5705

Open
bkontur opened this issue Sep 13, 2024 · 6 comments · May be fixed by #5804
Open

Refactor get_account_id_from_seed / get_from_seed to one common place #5705

bkontur opened this issue Sep 13, 2024 · 6 comments · May be fixed by #5804
Assignees
@programskillforverification
Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK.

Comments

@bkontur
Copy link
Contributor

bkontur commented Sep 13, 2024

These functions are copied all over the place.
@bkontur bkontur added C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. labels Sep 13, 2024
@bkontur bkontur mentioned this issue Sep 13, 2024
Merged
8 tasks
@bkontur
Copy link
Contributor Author

bkontur commented Sep 13, 2024

check @kianenigma's comment here: #5327 (comment)

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

@programskillforverification
Copy link
Contributor

I will work on this

@michalkucharczyk
Copy link
Contributor

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

@iulianbarbu
Copy link
Contributor

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).

@programskillforverification
Copy link
Contributor

programskillforverification commented Sep 22, 2024 •
edited
Loading

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

github-merge-queue bot pushed a commit that referenced this issue Sep 22, 2024
@bkontur @ordian
Moved presets to the testnet runtimes (#5327)
8735c66 
It is a first step for switching to the `frame-omni-bencher` for CI.

This PR includes several changes related to generating chain specs plus:

- [x] pallet `assigned_slots` fix missing `#[serde(skip)]` for phantom
- [x] pallet `paras_inherent` benchmark fix - cherry-picked from
#5688
- [x] migrates `get_preset` to the relevant runtimes
- [x] fixes Rococo genesis presets - does not work
https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7317249
- [x] fixes Rococo benchmarks for CI 
- [x] migrate westend genesis
- [x] remove wococo stuff

Closes: #5680

## Follow-ups
- Fix for frame-omni-bencher
#5655
- Enable new short-benchmarking CI -
#5706
- Remove gitlab pipelines for short benchmarking
- refactor all Cumulus runtimes to use `get_preset` -
#5704
- #5705
- #5700
- [ ] Backport to the stable

---------

Co-authored-by: command-bot <>
Co-authored-by: ordian <[email protected]>
bkontur added a commit that referenced this issue Sep 22, 2024
@bkontur
Moved presets to the testnet runtimes (#5327)
59dfea3 
It is a first step for switching to the `frame-omni-bencher` for CI.

This PR includes several changes related to generating chain specs plus:

- [x] pallet `assigned_slots` fix missing `#[serde(skip)]` for phantom
- [x] pallet `paras_inherent` benchmark fix - cherry-picked from
#5688
- [x] migrates `get_preset` to the relevant runtimes
- [x] fixes Rococo genesis presets - does not work
https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7317249
- [x] fixes Rococo benchmarks for CI
- [x] migrate westend genesis
- [x] remove wococo stuff

Closes: #5680

- Fix for frame-omni-bencher
#5655
- Enable new short-benchmarking CI -
#5706
- Remove gitlab pipelines for short benchmarking
- refactor all Cumulus runtimes to use `get_preset` -
#5704
- #5705
- #5700
- [ ] Backport to the stable

---------

Co-authored-by: command-bot <>
Co-authored-by: ordian <[email protected]>
(cherry picked from commit 8735c66)
@programskillforverification programskillforverification linked a pull request Sep 23, 2024 that will close this issue
Draft
@iulianbarbu
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@programskillforverification programskillforverification

Labels
C1-mentor A task where a mentor is available. Please indicate in the issue who the mentor could be. C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK.
Projects
None yet
Milestone
No milestone
4 participants
@michalkucharczyk @bkontur @iulianbarbu @programskillforverification