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

Runtime: avoid duplication and test all signature #14663

Merged
merged 3 commits into from
Aug 17, 2023
Merged

Runtime: avoid duplication and test all signature #14663

merged 3 commits into from
Aug 17, 2023

Conversation

ashWhiteHat
Copy link
Contributor

@ashWhiteHat ashWhiteHat commented Jul 28, 2023 •
edited
Loading

Description

I found signature test code following duplication.

substrate/primitives/runtime/src/traits.rs

Lines 2424 to 2425 in fbdf008

assert!(signature.verify(msg, &pair.public()));
assert!(signature.verify(msg, &pair.public()));

ecdsa error case is not tested and ed25519/sr25519 cases are not tested.
I added following test cases

  • ecdsa error
  • ed25519 normal/error
  • sr25519 normal/error
  • bls377 normal/error
  • bls381 normal/error

Checklist

  • My PR includes a detailed description as outlined in the "Description" section above
  • My PR follows the labeling requirements of this project (at minimum one label for each A, B, C and D required)
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • If this PR alters any external APIs or interfaces used by Polkadot, the corresponding Polkadot PR is ready as well as the corresponding Cumulus PR (optional)

@ashWhiteHat ashWhiteHat requested a review from a team July 28, 2023 02:26
@ashWhiteHat
Copy link
Contributor Author

Hi there
I would like to add label A0-please_review but it seems I can't

@ashWhiteHat
Copy link
Contributor Author

I also found following std feature duplication.
https://github.com/paritytech/substrate/blob/fbdf00888d778bab4d31a8938de7bed7e53d2d53/test-utils/runtime/Cargo.toml#L85-LL86

@liamaharon liamaharon 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. T0-node This PR/Issue is related to the topic “node”. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jul 28, 2023
@liamaharon liamaharon requested a review from a team July 28, 2023 05:29
liamaharon
liamaharon approved these changes Jul 28, 2023
View reviewed changes
Copy link
Contributor

@liamaharon liamaharon left a comment

Choose a reason for hiding this comment

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

Not sure if using a macro is overkill, otherwise lgtm

primitives/runtime/src/traits.rs Outdated Show resolved Hide resolved
@paritytech paritytech deleted a comment from command-bot bot Jul 28, 2023
@paritytech paritytech deleted a comment from command-bot bot Jul 28, 2023
@michalkucharczyk michalkucharczyk requested a review from a team July 28, 2023 06:20
davxy
davxy approved these changes Jul 28, 2023
View reviewed changes
primitives/runtime/src/traits.rs Outdated Show resolved Hide resolved
@ashWhiteHat ashWhiteHat requested a review from andresilva as a code owner July 29, 2023 00:20
@ashWhiteHat ashWhiteHat requested a review from a team July 29, 2023 00:21
@ashWhiteHat ashWhiteHat force-pushed the feature/signature-test-ashwhitehat branch from 880ae14 to 5e3f2cc Compare July 29, 2023 00:27
@ashWhiteHat ashWhiteHat force-pushed the feature/signature-test-ashwhitehat branch from 5e3f2cc to f805691 Compare July 29, 2023 00:31
@bkchr bkchr merged commit ec6be6e into paritytech:master Aug 17, 2023
Ank4n pushed a commit that referenced this pull request Aug 20, 2023
@ashWhiteHat @Ank4n
Runtime: avoid duplication and test all signature (#14663)
858140e 
* runtime: all signature test

* test-utils: remove std duplication

* runtime: add bls verify test
@crystalin crystalin mentioned this pull request Oct 20, 2023
Closed
@JesseAbram JesseAbram mentioned this pull request Oct 26, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@liamaharon liamaharon liamaharon approved these changes

@ggwpez ggwpez ggwpez left review comments

@koute koute koute left review comments

@davxy davxy davxy approved these changes

@bkchr bkchr bkchr approved these changes

@michalkucharczyk michalkucharczyk michalkucharczyk approved these changes

@andresilva andresilva Awaiting requested review from andresilva

Assignees
No one assigned
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 T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ashWhiteHat @koute @michalkucharczyk @bkchr @davxy @ggwpez @liamaharon