[wallet-kit] Add support for signMessage
#8690
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| messageWithIntent(IntentScope.PersonalMessage, message), | ||
| ); | ||
|
|
||
| return { | ||
| // TODO: Should this include the intent, or just be the raw bytes that are signed? |
There was a problem hiding this comment.
yes we should definitely include the intent bytes, otherwise one might ask you to sign a transaction masqueraded like a personal message.
There was a problem hiding this comment.
This todo isn't for the message that the user is asked to sign, it's for if the return value includes the intent, or if we just automatically add the intent when verifying the message instead.
| @@ -156,6 +156,11 @@ export type SignedTransaction = { | |||
| signature: SerializedSignature; | |||
| }; | |||
|
|
|||
| export type SignedMessage = { | |||
There was a problem hiding this comment.
I'd suggest renaming this to SignedPersonalMessage as this is the standard term; and avoid confusion with a transaction message (many times in crypto we generically use the term message to define any message type).
There was a problem hiding this comment.
I think in the SDK context it's fairly unambiguous, as we have signMessage + SignedMessage and signTransaction + SignedTransaction.
| "@mysten/sui.js": minor | ||
| --- | ||
|
|
||
| Change `signMessage` to return message bytes. Add support for sui:signMessage in the wallet standard |
There was a problem hiding this comment.
ditto, signPersonalMessage, let's use the term personalMessage everywhere
There was a problem hiding this comment.
sui:signMessage naming is designed to align with wallet standards from other chains, so would like to keep that the same. We could change the internal method names but then they don't align across our SDK, which seems non-ideal.
There was a problem hiding this comment.
Not that I like what the other wallets do, as we're probably the first blockchain who puts the intent as part of the default L1 specs (not as a community proposal), but I'm fine assuming we have documentation that we refer to personal message and the only reason was compatibility with the industry. (because at the same time we are now incompatible with our rust specs re intents that we call it PersonalMessage)
There was a problem hiding this comment.
Yeah I get the desire to keep alignment with rust, although we abstract over that by adding the intent automatically, so the PersonalMessage intent is effectively invisible to consumers of these APIs.
There was a problem hiding this comment.
yes that's fine, I think a small doc comment could slightly help here, but overall we're good with the PR
Jordan-Mysten
deleted the
jordan--Add_signMessage_to_the_wallet_standard
branch
Description
This adds support for
signMessagein the wallet kit ecosystem (adapters + UI + standard).Test Plan
Validated TypeScript types
If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.
Type of Change (Check all that apply)
Release notes