-
Notifications
You must be signed in to change notification settings - Fork 1k
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
EC Recover #3696
base: master
Are you sure you want to change the base?
EC Recover #3696
Conversation
@neo-project/core Please take special look into |
/// <param name="hashAlgorithm">The hash algorithm to be used hash the message.</param> | ||
/// <returns>The recovered public key in compressed format, or null if recovery fails.</returns> | ||
[ContractMethod(Hardfork.HF_Echidna, CpuFee = 1 << 10, Name = "recoverSecp256K1")] | ||
public static byte[] RecoverSecp256K1(byte[] message, byte[] signature, HashAlgorithm hashAlgorithm) |
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.
@shargon Hi shagon, i think what we should handle is passing the hash and signature, since the message sometimes require special format to be hashed which is not possible to be handled by our native contract, such as keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", hash))
. So we should leave the hash thing to the costum contract.
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.
HasAlgoritm.None
is allowed
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.
@Jim8y Check this recent conversation #3696 (comment).
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.
Maybe Jimmy's input could be taken as a hint that the current function signature might be misleading if one is not aware of HashAlgorithm.None
.
I'm not strongly opinionated on this, but it might be better to just remove the HashAlgorithm
parameter from the function signature. Then, it might be more clear that all the function does - as its name says - is recover the public key.
Description
Close #3628
Alternative to #3633
Type of change
How Has This Been Tested?
Test Configuration:
Checklist: