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

Application Crypto cleanup #13746

Merged
merged 9 commits into from
Mar 30, 2023
Merged

Application Crypto cleanup #13746

merged 9 commits into from
Mar 30, 2023

Conversation

davxy
Copy link
Member

@davxy davxy commented Mar 29, 2023 •
edited
Loading

  • Blanket implementation of RuntimeAppPublic trait for any type implementing AppPublic and for which AppPubic::Generic implements RuntimePublic (in practice implements the trait for any application-specific Public that can be used within the runtime).

    • Moving out this from app_crypto! allows to use the macro to implement application specific crypto types that are not necessarily usable by the runtime.
    • This is required for upcoming BLS crypto key types and Sassafras keys as well

  • Blanket implementation of BoundToRuntimeAppPublic trait for any type implementing RuntimeAppPublic

  • Remove unused CRYPTO_ID from RuntimeAppPublic

  • Documentation improvement

  • Relax some trait bounds

@davxy davxy self-assigned this Mar 29, 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 29, 2023
@davxy davxy requested a review from a team March 29, 2023 12:46
@davxy davxy mentioned this pull request Mar 29, 2023
Merged
michalkucharczyk
michalkucharczyk approved these changes Mar 29, 2023
View reviewed changes
Copy link
Contributor

@michalkucharczyk michalkucharczyk left a comment

Choose a reason for hiding this comment

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

lgtm. Out of curiosity - why did you remove MaybeHash? Is this because it was not used anywhere? Or maybe it was to restrictive for some type that will be used in this context in future?

@davxy
Copy link
Member Author

davxy commented Mar 29, 2023

lgtm. Out of curiosity - why did you remove MaybeHash? Is this because it was not used anywhere? Or maybe it was to restrictive for some type that will be used in this context in future?

Because I'm dumb 🤦‍♂️
Is required by cumulus. Restoring it

@davxy davxy requested a review from a team March 29, 2023 15:43
@altaua
Copy link
Contributor

altaua commented Mar 29, 2023

This needs to be rebased on current master, there have been breaking changes to the check-crate-publishing job.

melekes
melekes reviewed Mar 30, 2023
View reviewed changes
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.

lgtm

primitives/application-crypto/src/lib.rs Outdated Show resolved Hide resolved
davxy and others added 2 commits March 30, 2023 09:35
@davxy davxy requested review from melekes and a team March 30, 2023 07:35
@davxy davxy merged commit 14f1a39 into master Mar 30, 2023
@davxy davxy deleted the davxy-app-crypto-cleanup branch March 30, 2023 08:30
pgherveou pushed a commit that referenced this pull request Apr 4, 2023
@davxy @melekes @pgherveou
Application Crypto cleanup (#13746)
27aa298 
* Adjust application crypto docs

* Blanket implementation for 'RuntimeAppPublic' trait

* Blanket implementation for 'BoundToRuntimeAppPublic' for 'RuntimeAppPublic'

* Relax type bounds

* Docs fix

* restore MaybeHash

* Commit suggestion

Co-authored-by: Anton <anton.kalyaev@gmail.com>

---------

Co-authored-by: Anton <anton.kalyaev@gmail.com>
gpestana pushed a commit that referenced this pull request Apr 23, 2023
@davxy @melekes @gpestana
Application Crypto cleanup (#13746)
8929804 
* Adjust application crypto docs

* Blanket implementation for 'RuntimeAppPublic' trait

* Blanket implementation for 'BoundToRuntimeAppPublic' for 'RuntimeAppPublic'

* Relax type bounds

* Docs fix

* restore MaybeHash

* Commit suggestion

Co-authored-by: Anton <anton.kalyaev@gmail.com>

---------

Co-authored-by: Anton <anton.kalyaev@gmail.com>
@kacperzuk-neti kacperzuk-neti mentioned this pull request Jun 23, 2023
Merged
15 tasks
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
@davxy @melekes @nathanwhit
Application Crypto cleanup (paritytech#13746)
97e37fc 
* Adjust application crypto docs

* Blanket implementation for 'RuntimeAppPublic' trait

* Blanket implementation for 'BoundToRuntimeAppPublic' for 'RuntimeAppPublic'

* Relax type bounds

* Docs fix

* restore MaybeHash

* Commit suggestion

Co-authored-by: Anton <anton.kalyaev@gmail.com>

---------

Co-authored-by: Anton <anton.kalyaev@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@skunert skunert skunert approved these changes

@michalkucharczyk michalkucharczyk michalkucharczyk approved these changes

@melekes melekes Awaiting requested review from melekes

Assignees

@davxy davxy

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@davxy @altaua @melekes @michalkucharczyk @skunert