-
Notifications
You must be signed in to change notification settings - Fork 67
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
ECRecover contract upgrade #414
Conversation
# Conflicts: # src/Stratis.Bitcoin.Features.SmartContracts.Tests/Stratis.Bitcoin.Features.SmartContracts.Tests.csproj # src/Stratis.Bitcoin.Features.SmartContracts/Stratis.Bitcoin.Features.SmartContracts.csproj # src/Stratis.SmartContracts.CLR.Validation/Stratis.SmartContracts.CLR.Validation.csproj
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.
LGTM. Can merge when CI passes.
/// <returns>The base58 address for the signer of a signature.</returns> | ||
public Address GetSigner(byte[] message, byte[] signature) | ||
{ | ||
// TODO: Error handling for incorrect signature format etc. |
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 this needs to be resolved before this can be used in production.
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.
@rowandh PubKey.RecoverCompact already detects incorrect format and raise an exception.
There is something else to do in here ?
Perhaps include support for
|
uint256 hashedUint256 = GetUint256FromMessage(message); | ||
PubKey pubKey = PubKey.RecoverCompact(hashedUint256, signature); | ||
|
||
return pubKey.GetAddress(this.network).ToString().ToAddress(this.network); |
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.
Perhaps this conversion can be done without using this.network
.
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.
Fixed
The stratisproject/StratisBitcoinFullNode#4194 moved here.
This pr needs to have package from stratisproject/Stratis.SmartContracts#13
This pr has been reviewed actually and should to be complete but it was require to have hard-fork because upgrade needed on Stratis.SmartContracts assemblies.
Basically
EcRecoverProvider.SignMessage(privateKey, message)
returns a signature that we can recoverPubKey
address and we can do this viaECRecoverProvider.GetSigner(message, offChainSignature)
in the smart contract code.I reviewed&tested it and i don't have anything to add.
I am going to remove dev prefix from version(2.0.1-dev) if it is approved.