Add sr25519_verify() #1840
Add sr25519_verify() #1840
Conversation
public key byte[] and/or signature byte[] may not be constructed into a PublicKey or Signature if it's in the wrong format. These errors originate from the schnorrkel library and need to be caught, otherwise it'll panic and propagate up to smart contracts in ink.
there is no need to make the context directly, we can delegate that to the schnorrkel library
Docs + example + notes about context set to "substrate"
Helpful for debugging why the error has occurred, as there is only ever one of 3 reasons: invalid public key, invalid message or invalid signature
Not required according to clippy
Tested invalid public key, invalid signature, invalid message and valid case
…sr25519 into goastler-add-sr25519_verify
| @@ -146,6 +146,15 @@ mod sys { | |||
| output_ptr: Ptr32Mut<[u8]>, | |||
| ) -> ReturnCode; | |||
|
|
|||
| /// **WARNING**: this function is from the [unstable interface](https://github.com/paritytech/substrate/tree/master/frame/contracts#unstable-interfaces), | |||
| /// which is unsafe and normally is not available on production chains. | |||
| pub fn sr25519_verify( | |||
There was a problem hiding this comment.
Yep . I think we can feature-gate the function with the sr25519-verify verify until it gets stabilized
There was a problem hiding this comment.
Features are a last resort and should only be used when absolutely necessary (i.e not here). We don't need a feature here. A clear doc that this is an unstable functionality is enough. If someone ignores that hint nothing bad will happen. The contract will simply not deploy on a production chain.
There was a problem hiding this comment.
@goastler Could you propagate this warning to the public method then, please(available for users)? Because this comment is useful only for people who dive deep into the ink! codebase.
Co-authored-by: Green Baneling <XgreenX9999@gmail.com>
|
Please merge |
…sr25519 into goastler-add-sr25519_verify
done |
|
Providing @athei @xgreenx @SkymanOne and @cmichi are happy, this is ready to merge |
| } | ||
|
|
||
| #[ink(message)] | ||
| pub fn noop(&self) {} |
There was a problem hiding this comment.
I presume since you must have a message in the ink! contacts, but you can't use sr25519_verify on the substrate-contracts-node, you left the function blank. Can you please either document it somehow, or double check that you can't use sr25519_verify with the substrate-contracts-node?
There was a problem hiding this comment.
You're correct, I left it blank due to the >0 function in an ink contract requirement. I wanted to provide a demonstration of the sr25519_verify function in the unit tests for a contract which would be the same usage as in production for a contract, thus making a good example. I can move it to run directly against substrate-contracts-node but this would drop the example.
In the meantime, I've added a comment documenting this
This is an extension of another PR which attempted to add sr25519 signature verification to ink.
Several points were made in that PR but not all were addressed. The original author has not resolved those issues as they do not have the time, so I have instead.
I have fixed the panics which were being thrown due to invalid public keys or signatures.
I've added tests for each signature verification outcome:
and put these in the integrations-tests folder (hopefully that's the right place?)
At the moment, the context parameter of sr25519 is fixed to "substrate". This is set in the substrate repo, so I've mirrored it here. It probably should be a parameter rather than being hard-coded, but that would require a change on the substrate end and ink end, so should be left as a job for after this PR.
I've also applied clippy and rust fmt recommendations. Also added docs.