Andrews pop version #7
Conversation
|
All issues should be handled from check list from the following issue: Bandersnatch left empty let me know if this is ok for now.. |
| /// Dummy implementation. Returns `None`. | ||
| fn sign<M: AsRef<[u8]>>(&self, _key_type: KeyTypeId, _msg: &M) -> Option<Self::Signature> { | ||
| None | ||
| fn sign<M: AsRef<[u8]>>(&self, key_type: KeyTypeId, msg: &M) -> Option<Self::Signature> { |
There was a problem hiding this comment.
I'll comment on the other pull request to ask @davxy if it is ok to add these given that we are not adding a new host function.
| sp_io::crypto::ecdsa_sign(key_type, self, pop_statement.as_slice()) | ||
| } | ||
|
|
||
| fn verify_pop(&self, pop: &Self::Signature) -> bool { |
There was a problem hiding this comment.
because they haven't use AppPair::verify for signature verification, perhaps we follow the same path and call ecdsa_verify here.
| sp_io::crypto::ed25519_sign(key_type, self, pop_statement.as_slice()) | ||
| } | ||
|
|
||
| fn verify_pop(&self, pop: &Self::Signature) -> bool { |
There was a problem hiding this comment.
Same here calling into sp_io directly.
| @@ -135,6 +135,14 @@ impl sp_application_crypto::RuntimeAppPublic for UintAuthorityId { | |||
| traits::Verify::verify(signature, msg.as_ref(), &self.0) | |||
| } | |||
|
|
|||
| fn generate_pop(&mut self) -> Option<Self::Signature> { | |||
There was a problem hiding this comment.
Maybe we add a gen and verify test for PoP in primitive/runtime/src/lib.rs for all crypto types.
There was a problem hiding this comment.
So I looked into this and imo it isnt really making sense to do these tests there. All that will be tested is the generate_pop and verify_pop from sp_core::crypto (Which already these functions have unit tests for) For instance other unit tests with respect to other crypto stuff is like a MultiSignature for ecdsa and a few other non-crypto related things. Perhaps we should look for somewhere else to add some tests?
✄ -----------------------------------------------------------------------------
Thank you for your Pull Request! 🙏 Please make sure it follows the contribution guidelines outlined in this
document and fill out the
sections below. Once you're ready to submit your PR for review, please delete this section and leave only the text under
the "Description" heading.
Description
A concise description of what your PR is doing, and what potential issue it is solving. Use Github semantic
linking
to link the PR to an issue that must be closed once this is merged.
Integration
In depth notes about how this PR should be integrated by downstream projects. This part is mandatory, and should be
reviewed by reviewers, if the PR does NOT have the
R0-Silentlabel. In case of aR0-Silent, it can be ignored.Review Notes
In depth notes about the implementation details of your PR. This should be the main guide for reviewers to
understand your approach and effectively review it. If too long, use
<details>.Imagine that someone who is depending on the old code wants to integrate your new code and the only information that
they get is this section. It helps to include example usage and default value here, with a
diffcode-block to showpossibly integration.
Include your leftover TODOs, if any, here.
Checklist
Trequired)You can remove the "Checklist" section once all have been checked. Thank you for your contribution!
✄ -----------------------------------------------------------------------------