-
Notifications
You must be signed in to change notification settings - Fork 1k
Adds message signing to wallet #4286
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
base: master
Are you sure you want to change the base?
Adds message signing to wallet #4286
Conversation
Adds the ability to sign arbitrary messages using wallet accounts. The user can specify the elliptic curve to use for signing (secp256r1 or secp256k1). It generates a payload using salt and the provided message, then signs the payload with each available wallet account, displaying the address, public key, salt and signature.
| /// </summary> | ||
| /// <param name="message">Message to sign</param> | ||
| /// <param name="curve">Optional curve (secp256r1 or secp256k1)</param> | ||
| [ConsoleCommand("sign_message", Category = "Wallet Commands")] |
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.
| [ConsoleCommand("sign_message", Category = "Wallet Commands")] | |
| [ConsoleCommand("sign message", Category = "Wallet Commands")] |
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 treid this approach, but it seems like it then ran "sign" and "sign message" commands at the same time. And outputted errors.
I feel the naming of "sign_message" is not good, but it works.
I also considered renaming "sign" to "sign transaction" (or somthing of that nature), but I was afraid that could be considered a braking change.
Any suggestions for a better command name that would not be a breaking change?
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.
We need to solve this issue before merging the code.
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 if we call it "signmsg"?
Or do you want to fix the issue that both commands are run @erikzhang?
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 prefer to fix it first.
| var signature = Crypto.Sign(payload, key.PrivateKey, selectedCurve); | ||
|
|
||
| ConsoleHelper.Info("Address: ", account.Address); | ||
| ConsoleHelper.Info(" PublicKey: ", key.PublicKey.EncodePoint(true).ToHexString()); |
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.
we dont really support secp256k1, the key you get is still p-256 pubkey.
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.
Ok. I think I saw in the code for Crypto.Sign that both were supported so that is why I added it.
Do you want me tor remove the option to select curve and only use secp256r1?
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.
We need a structured format that specifies the address, public key, and algorithm for signing. This also facilitates verification.
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.
Some questions:
-
Should we require the user specifiy either the public key or address for signing?
-
If yes to question 1: If there is only one address in the open wallet, could we default to using this one? I think this will make signing more user friendly.
-
For specifing the algorithim, do you mean the curve or padding, salt, etc.? Or both?
-
Can we provide defaults for each option so that the command become user friendly?
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 mean we need to define an output format including address, public key, and algorithm of the signature.
| w.Write((byte)0x01); | ||
| w.Write((byte)0x00); | ||
| w.Write((byte)0x01); | ||
| w.Write((byte)0xF0); | ||
| w.WriteVarBytes(paramBytes); | ||
| w.Write((ushort)0); | ||
| w.Flush(); |
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 is the meaning of this script, some standard?
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.
Yes, could you clarify it @adrian-fjellberg
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.
It is to ensure that the message is not also a valid NEO transaction. I do not think it is a defined standard but it is the way both NEON wallet and NeoLine signs messages.
Do you want me to add comments to the code to clearify?
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 see, its just some prefix.
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 can add comments to the code.
|
I also started on documenting the command line feature: |
Adds the ability to sign arbitrary messages using wallet accounts. The user can specify the elliptic curve to use for signing (secp256r1 or secp256k1). It generates a payload using salt and the provided message, then signs the payload with each available wallet account, displaying the address, public key, salt and signature.
Description
This PR introduces a new
sign_messagecommand to the Neo CLI, enabling message signing outside of transactions.Previously, the CLI only supported signing serialized transaction data, which made it impossible to produce verifiable message signatures for use in external applications, authentication workflows, or dApp integrations.
With this change, users can now easily sign and verify arbitrary messages directly from their local wallet.
Change Log
Fixes #4285
Type of change
How Has This Been Tested?
sign_messageon local CLI with both curvesChecklist: