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

Runtime: avoid duplication and test all signature#14663

Merged
bkchr merged 3 commits intoparitytech:masterfrom
ashWhiteHat:feature/signature-test-ashwhitehat
Aug 17, 2023
Merged

Runtime: avoid duplication and test all signature#14663
bkchr merged 3 commits intoparitytech:masterfrom
ashWhiteHat:feature/signature-test-ashwhitehat

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

ashWhiteHat commented Jul 28, 2023

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

@ashWhiteHat
Copy link
Contributor Author

ashWhiteHat commented Jul 28, 2023

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

@paritytech paritytech deleted a comment from command-bot bot Jul 28, 2023
@paritytech paritytech deleted a comment from command-bot bot Jul 28, 2023
liamaharon
liamaharon reviewed Jul 28, 2023
View reviewed changes
@michalkucharczyk michalkucharczyk requested a review from a team July 28, 2023 06:20
davxy
davxy approved these changes Jul 28, 2023
View reviewed changes
@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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@andresilva andresilva Awaiting requested review from andresilva

6 more reviewers

@koute koute koute left review comments

@ggwpez ggwpez ggwpez left review comments

@liamaharon liamaharon liamaharon approved these changes

@michalkucharczyk michalkucharczyk michalkucharczyk approved these changes

@bkchr bkchr bkchr approved these changes

@davxy davxy davxy approved these changes

Reviewers whose approvals may not affect merge requirements

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