-
Notifications
You must be signed in to change notification settings - Fork 8
add recovery bit to allow restoring the public key #137
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
Conversation
356e1b9
to
9ed05ce
Compare
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, one question/suggestion but can be addressed separately
accountId: author.accountId, | ||
publicKey: author.signaturePublicKey, | ||
signature, | ||
signature: signatureResult.toCompactHex(), |
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.
What do you think of grouping the hex part and recovery of the signature in a SignatureWithRecovery type like I did here? https://github.com/graphprotocol/hypergraph/pull/135/files#diff-46bf560b871193fa360550e8a148fcf76963723d89bd6252c4cfc4e729d9704eR5-R15
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.
yeah, sounds good. Will update the PR
btw our addresses use 0x
as prefix to indicate hex, but signatures don't. Should we add it for consistency or is it only relevant for addresses? What's the default in the Ethereum eco-system?
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.
Good question, I think it would be good to standardize and use the 0x prefix in any hex strings, so yeah that'd include signatures. Most (if not all) ethereum tooling uses that everywhere.
signatureInstance = signatureInstance.addRecoveryBit(event.author.recovery); | ||
const authorPublicKey = signatureInstance.recoverPublicKey(sha256(encodedTransaction)); | ||
|
||
const isValidSignature = secp256k1.verify(event.author.signature, encodedTransaction, authorPublicKey.toRawBytes(), { |
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.
Actually, one more thing - I think this might not be necessary, as recoverPublicKey essentially verifies the signature too (it will only give the correct public key if the corresponding private key signed that message)
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 might even be redundant, because the public key is recovered from the signature, so it will probably always be valid - I think we should instead validate that the public key is a valid signing key for the author's accountId
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.
Yeah, as a next step we ned to retrieve the publicKey from the author and pass it in. At the moment afaik I need to reconstruct it, since it's a required argument for the verify
function.
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.
What I mean is we don't need to call the verify
function. Once you retrieve the author's public key and compare it to the one you recovered, you have essentially verified the signature
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.
added an issue for it: #138
Uh oh!
There was an error while loading. Please reload this page.