Skip to content

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 29, 2025
Merged

add recovery bit to allow restoring the public key #137

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ed05ce
add recovery bit to allow restoring the public key
nikgraf Jan 29, 2025
af6afcd
implement feedback
nikgraf Jan 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions packages/hypergraph/src/space-events/accept-invitation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,16 @@ export const acceptInvitation = ({
previousEventHash,
};
const encodedTransaction = stringToUint8Array(canonicalize(transaction));
const signature = secp256k1
.sign(encodedTransaction, hexToBytes(author.signaturePrivateKey), { prehash: true })
.toCompactHex();
const signatureResult = secp256k1.sign(encodedTransaction, hexToBytes(author.signaturePrivateKey), {
prehash: true,
});

return Effect.succeed({
transaction,
author: {
accountId: author.accountId,
publicKey: author.signaturePublicKey,
signature,
signature: signatureResult.toCompactHex(),
recovery: signatureResult.recovery,
},
});
};
19 changes: 9 additions & 10 deletions packages/hypergraph/src/space-events/apply-event.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { secp256k1 } from '@noble/curves/secp256k1';
import { sha256 } from '@noble/hashes/sha256';
import { Effect, Schema } from 'effect';
import type { ParseError } from 'effect/ParseResult';

import { canonicalize, hexToBytes, stringToUint8Array } from '../utils/index.js';
import { canonicalize, stringToUint8Array } from '../utils/index.js';
import { hashEvent } from './hash-event.js';
import {
InvalidEventError,
Expand Down Expand Up @@ -41,14 +41,13 @@ export const applyEvent = ({

const encodedTransaction = stringToUint8Array(canonicalize(event.transaction));

const isValidSignature = secp256k1.verify(
event.author.signature,
encodedTransaction,
hexToBytes(event.author.publicKey),
{
prehash: true,
},
);
let signatureInstance = secp256k1.Signature.fromCompact(event.author.signature);
signatureInstance = signatureInstance.addRecoveryBit(event.author.recovery);
const authorPublicKey = signatureInstance.recoverPublicKey(sha256(encodedTransaction));

const isValidSignature = secp256k1.verify(event.author.signature, encodedTransaction, authorPublicKey.toRawBytes(), {
Copy link
Member

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)

Copy link
Member

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

Copy link
Collaborator Author

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.

Copy link
Member

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

Copy link
Collaborator Author

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

prehash: true,
});

if (!isValidSignature) {
return Effect.fail(new VerifySignatureError());
Expand Down
10 changes: 5 additions & 5 deletions packages/hypergraph/src/space-events/create-invitation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,16 @@ export const createInvitation = ({
previousEventHash,
};
const encodedTransaction = stringToUint8Array(canonicalize(transaction));
const signature = secp256k1
.sign(encodedTransaction, hexToBytes(author.signaturePrivateKey), { prehash: true })
.toCompactHex();
const signatureResult = secp256k1.sign(encodedTransaction, hexToBytes(author.signaturePrivateKey), {
prehash: true,
});

return Effect.succeed({
transaction,
author: {
accountId: author.accountId,
publicKey: author.signaturePublicKey,
signature,
signature: signatureResult.toCompactHex(),
recovery: signatureResult.recovery,
},
});
};
10 changes: 5 additions & 5 deletions packages/hypergraph/src/space-events/create-space.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,16 @@ export const createSpace = ({ author }: Params): Effect.Effect<CreateSpaceEvent,
creatorAccountId: author.accountId,
};
const encodedTransaction = stringToUint8Array(canonicalize(transaction));
const signature = secp256k1
.sign(encodedTransaction, hexToBytes(author.signaturePrivateKey), { prehash: true })
.toCompactHex();
const signatureResult = secp256k1.sign(encodedTransaction, hexToBytes(author.signaturePrivateKey), {
prehash: true,
});

const event: CreateSpaceEvent = {
transaction,
author: {
accountId: author.accountId,
publicKey: author.signaturePublicKey,
signature,
signature: signatureResult.toCompactHex(),
recovery: signatureResult.recovery,
},
};

Expand Down
10 changes: 5 additions & 5 deletions packages/hypergraph/src/space-events/delete-space.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,16 @@ export const deleteSpace = ({ author, id, previousEventHash }: Params): Effect.E
previousEventHash,
};
const encodedTransaction = stringToUint8Array(canonicalize(transaction));
const signature = secp256k1
.sign(encodedTransaction, hexToBytes(author.signaturePrivateKey), { prehash: true })
.toCompactHex();
const signatureResult = secp256k1.sign(encodedTransaction, hexToBytes(author.signaturePrivateKey), {
prehash: true,
});

const event: DeleteSpaceEvent = {
transaction,
author: {
accountId: author.accountId,
publicKey: author.signaturePublicKey,
signature,
signature: signatureResult.toCompactHex(),
Copy link
Member

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

Copy link
Collaborator Author

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?

Copy link
Member

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.

recovery: signatureResult.recovery,
},
};
return Effect.succeed(event);
Expand Down
4 changes: 1 addition & 3 deletions packages/hypergraph/src/space-events/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,8 @@ import * as Schema from 'effect/Schema';

export const EventAuthor = Schema.Struct({
accountId: Schema.String,
// must be validated if it belongs to the accountId before being used
// Note: could be removed, but also might be useful to keep around in case accounts rotate their keys
publicKey: Schema.String,
signature: Schema.String,
recovery: Schema.Number, // allows to recover the publicKey
});

export type EventAuthor = Schema.Schema.Type<typeof Author>;
Expand Down
Loading