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

Closed
bkontur opened this issue Sep 13, 2024 · 9 comments · Fixed by #5804
Closed

Refactor get_account_id_from_seed / get_from_seed to one common place #5705

bkontur opened this issue Sep 13, 2024 · 9 comments · 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 <noreply@reusable.software>
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 <noreply@reusable.software>
(cherry picked from commit 8735c66)
Merged
@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.

This was referenced Oct 1, 2024
Merged
Merged
@iulianbarbu
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

iulianbarbu commented Oct 1, 2024 •
edited
Loading

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.

github-merge-queue bot pushed a commit that referenced this issue Oct 17, 2024
@programskillforverification @iulianbarbu @michalkucharczyk
Refactor get_account_id_from_seed / get_from_seed to one common place (…
a2c7575 
…#5804)

Closes #5705

---------

Co-authored-by: Iulian Barbu <14218860+iulianbarbu@users.noreply.github.com>
Co-authored-by: Michal Kucharczyk <1728078+michalkucharczyk@users.noreply.github.com>
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