Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Keystore overhaul #13615

Merged
merged 7 commits into from
Mar 17, 2023
Merged

Keystore overhaul #13615

merged 7 commits into from
Mar 17, 2023

Conversation

davxy
Copy link
Member

@davxy davxy commented Mar 15, 2023

Partially address #13556

This PR is part of a small overhaul of the keystore related traits and structures.

A followup PR will focus on the keystore methods... I prefer to keep this PR small and quickly reviewable


Mostly delivers:

  • removal of the CryptoStoreasync trait.
    It was introduced a long ago in the hope of a remote keystore.
    Today the 90% of our code still depend on the sync version thus there's not point of having it around. If one day™️ it will be really required then well can trivially reintroduce it.
    But in the meantime is better to have no bloat in an already giant repo.
  • trivial renaming to better express the traits intent in a less verbose way. May be a bit opinionated but I think that overall the introduced names are a bit more coherent:
    • trait SyncCryptoStore -> Keystore
    • trait CryptoStore → removed 🔴
    • SyncCryptoKeyStorePtrKeystorePtr
    • struct testing::KeyStoretesting::MemoryKeystore (this is what it is and is more inline with the other implementation we provide: LocalKeystore).
  • removal of a couple of unused methods from the trait interface: sign_with_any and sign_with_all
    • the former was unused
    • the latter was used in one single place and can be trivially performed in the iteration that was required anyway for additional operations in the caller.

NOTE: big part of the changes spread through the project are due to the renamings 🫤.

polkadot companion: paritytech/polkadot#6892
cumulus companion: paritytech/cumulus#2336

- SyncCryptoStore -> Keystore
- SyncCryptoStorePtr -> KeystorePtr
- KeyStore -> MemoryKeystore
@davxy davxy requested review from bkchr and a team March 15, 2023 18:30
@davxy davxy self-assigned this Mar 15, 2023
@davxy davxy added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Mar 15, 2023
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

primitives/keystore/src/lib.rs Outdated Show resolved Hide resolved
client/service/src/builder.rs Outdated Show resolved Hide resolved
@davxy davxy requested a review from skunert March 16, 2023 09:54
@paritytech paritytech deleted a comment from paritytech-cicd-pr Mar 17, 2023
@davxy
Copy link
Member Author

davxy commented Mar 17, 2023

bot merge

@paritytech-processbot
Copy link

Error: "Check reviews" status is not passing for paritytech/polkadot#6892

@davxy
Copy link
Member Author

davxy commented Mar 17, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit a2d4f7d into master Mar 17, 2023
@paritytech-processbot paritytech-processbot bot deleted the davxy-keystore-overhaul branch March 17, 2023 11:24
@nazar-pc nazar-pc mentioned this pull request Apr 2, 2023
1 task
breathx pushed a commit to gear-tech/substrate that referenced this pull request Apr 22, 2023
* Remove 'supported_keys' 'sign_with_any' and 'sign_with_all' from keystore trait

* Remove the aync keystore

* Renaming:
- SyncCryptoStore -> Keystore
- SyncCryptoStorePtr -> KeystorePtr
- KeyStore -> MemoryKeystore

* Fix authority discovery worker and tests

* Rename 'insert_unknown' to 'insert'

* Remove leftover
AurevoirXavier added a commit to darwinia-network/darwinia that referenced this pull request Jul 11, 2023
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* Remove 'supported_keys' 'sign_with_any' and 'sign_with_all' from keystore trait

* Remove the aync keystore

* Renaming:
- SyncCryptoStore -> Keystore
- SyncCryptoStorePtr -> KeystorePtr
- KeyStore -> MemoryKeystore

* Fix authority discovery worker and tests

* Rename 'insert_unknown' to 'insert'

* Remove leftover
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants