-
Notifications
You must be signed in to change notification settings - Fork 140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new crypto features #852
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, nice work!
I added a breaking changes label because of the naming change from ECDSA_Secp256k1
to ECDSA_secp256k1
, which we need to communicate
let isValid: Bool | ||
|
||
/// Verifies a signature. Checks whether the signature was produced by signing | ||
/// the given tag and data, using this public key and the given hash algorithm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not quite what the verification checks, I suggest updating the text to be more precise:
/// Verifies a signature under the given tag, data and public key. It uses the given hash algorithm to hash the tag and data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another point, I'm not sure if this is right place to mention that all verifications with an invalid public key will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be fine to add it here, or we can also add it as a note outside of the code block.
// Hash the data using the given hashing algorithm and returns the hashed data. | ||
pub fun hash(_ data: [UInt8], algorithm: HashAlgorithm): [UInt8] | ||
|
||
// Hash the data using the given hashing algorithm and the tag. Returns the hashed data. | ||
pub fun hashWithTag(_ data: [UInt8], tag: string, algorithm: HashAlgorithm): [UInt8] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I leave this comment here, it's about the function at this line:
Shouldn't we also update isValid
to Verify
here too? or is that a big breaking change?
// Hash the data using the given hashing algorithm and returns the hashed data. | ||
pub fun hash(_ data: [UInt8], algorithm: HashAlgorithm): [UInt8] | ||
|
||
// Hash the data using the given hashing algorithm and the tag. Returns the hashed data. | ||
pub fun hashWithTag(_ data: [UInt8], tag: string, algorithm: HashAlgorithm): [UInt8] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have suggested hashWithTag
as I wasn't sure if PublicKey.Verify
would be implemented entirely by the fvm or would be partly implemented in Cadence code. In the case of the former, I don't think hashWithTag
is needed in Cadence, we would only need it as an fvm function.
@@ -167,6 +167,16 @@ struct AccountKey { | |||
struct PublicKey { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can PublicKey
fields (data, signature algo, isValid..) be updated after the creation of the object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, they are defined as constant fields (i.e. read-only).
|
||
/// Verifies a signature. Checks whether the signature was produced by signing | ||
/// the given tag and data, using this public key and the given hash algorithm | ||
pub fun verify( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @turbolent mentioned about breaking changes, I think the change from isValid
to verify
is a breaking one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, both were added, so this isn't a breaking change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah my bad, I somehow though isValid
already existed.
Closes #582
Description
This PR introduces the Cadence changes needed for the new crypto features. Includes the following changes:
Cadence Changes
Hash Algorithms
Signature Algorithms
ECDSA_Secp256k1
signature algorithm toECDSA_secp256k1
PublicKey Native Type
isValid
fieldValidatePublicKey
method on the host env.verify
method to validate a signaturepub fun verify(signature: [UInt8], signedData: [UInt8], domainSeparationTag: String, hashAlgorithm: HashAlgorithm): Bool
Crypto Contract
hashWithTag
functionpub fun hashWithTag(_ data: [UInt8], tag: string, algorithm: HashAlgorithm): [UInt8]
Changes to the FVM Interface
ValidatePublicKey(key *PublicKey) (bool, error)
methodtag string
in theHash
method.Hash(data []byte, tag string, hashAlgorithm HashAlgorithm) ([]byte, error)
PublicKey
has two new fields:IsValid
andValidated
For contributor use:
master
branchFiles changed
in the Github PR explorer