Skip to content

Conversation

@JGiter
Copy link
Collaborator

@JGiter JGiter commented Feb 23, 2023

Description

Contributor checklist

  • Breaking changes - check for any existing interfaces changes that are not backward compatible, removed method etc.
  • Documentation - document your code, add comments for method, remember to check if auto generated docs were updated.
  • Tests - add new or updated existed test for changes you made.
  • Migration guide - add migration guide for every breaking change.
  • Configuration correctness - check that any configuration changes are correct ex. default URLs, chain ids, smart contract verification on Volta explorer or EWC explorer.

@JGiter JGiter changed the title dd sign non eip-191 Add sign non eip-191 Feb 23, 2023
Comment on lines +348 to +359
/**
* Signs message with whatever signing implementation underlying signer provides. Unlike `signMessage` this method does not guaraties that signature will conform to EIP-191
* @param message Stringified message
*/
async sign(message: string): Promise<string> {
const messageBytes = Buffer.from(message);
const signature = await this.signer.signMessage(messageBytes);
if (!this._publicKey || this._isEthSigner === undefined) {
await this._determineSigner(messageBytes, signature);
}
return signature;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get the "this method does not guaraties that signature will conform to EIP-191" as signer.signMessage performs an EIP-191 signature, right?

https://docs.ethers.org/v5/api/signer/#Signer-signMessage

A signed message is prefixd with "\x19Ethereum Signed Message:\n" and the length of the message, using the hashMessage method, so that it is EIP-191 compliant. If recovering the address in Solidity, this prefix will be required to create a matching hash.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is requirements to implementers of Signer interface. But not all implementations are conform to this. If I remember correctly this is the reason why we are prefixing message manually before passing it to Metamask signer

const messageHash = this._isEthSigner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants