-
Notifications
You must be signed in to change notification settings - Fork 39
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
feat: add ECDSA signature support #147
Conversation
packages/snap/src/rpc/signMessage.ts
Outdated
); | ||
|
||
const btcNetwork = getNetwork(snapNetwork); | ||
const path = [...pathMap[ScriptType.P2SH_P2WPKH], "0'", '0', '0']; |
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.
Instead of hardcode the path here, it should be passed in as the parameter.
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.
Could you please elaborate on this? How do you see it?
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.
Adding the path as the parameter to the signMessage 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.
I've tried to support the current API of the snap. neither btc_signPsbt and btc_signLNInvoice allow to change the path, should we do it here?
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.
The PSBT data includes the BIP32 path in the inputs field to indicate which key to use for signing. For signMessage, this information should also be included to specify which key is being used for signing.
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.
So I'll do then next,
I'm going to remove scriptType from arguments, and will add derivePath argument so users can pass full derivation path, is it ok?
Also I'm going to test the path against request bip32 entropies supported by snap
network: btcNetwork, | ||
}).address as string; | ||
|
||
const result = await snap.request({ |
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.
A warning should be included, if the message is not an hummable readable string, like a hex string.
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've included warning for any message, the same way metamask does.
db783c2
to
a299f73
Compare
@aaronisme updated |
This PR introduces a new RPC method to the Bitcoin Snap that allows for the signing of messages.
This feature enhances the BTC Snap by providing users with the ability to sign messages directly from the Snap, improving functionality and user control.
The added tests ensure that the feature is reliable and behaves as expected in various conditions.