-
Notifications
You must be signed in to change notification settings - Fork 50
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.
Verification is broken for system that doesn't run init
before verify
, as init
modifies the static import of the signer, instead of a specific instance.
also CI reports that the yarn.lock is invalid
src/signing/chains/StarknetSigner.ts
Outdated
|
||
static async verify(_pk: Buffer, message: Uint8Array, _signature: Uint8Array, _opts?: any): Promise<boolean> { | ||
let message_to_felt = convertToFelt252(message); | ||
let TypedDataMessage = typed_domain({ chainId: StarknetSigner.chainId, message: message_to_felt }); |
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.
chainId would be undefined here as nothing sets it (data item verification doesn't run the init
function) - this would cause verification to fail.
I'd suggest adding the chainID to the public key/signature data so you can use it in the verify function.
src/signing/chains/StarknetSigner.ts
Outdated
const pub_key = await signer.getPubKey(); | ||
let hexKey = pub_key.startsWith("0x") ? pub_key.slice(2) : pub_key; | ||
this.publicKey = Buffer.from(0 + hexKey, "hex"); | ||
StarknetSigner.chainId = await StarknetSigner.provider.getChainId(); |
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.
overriding a static signer property is a very bad behaviour - right now your test cases pass because this change sticks around between test runs - if you removed this, or tried running verify
before init
, you would see that verification doesn't work
yarn.lock
Outdated
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.
CI reports that this yarn.lock is invalid
Hey @codeWhizperer - please copy this PR over to the new bundles repository |
No description provided.