feat: Add `signMessage` method #779

Elabar · 2023-04-25T13:56:07Z

Description

This PR is largely based on work from #604

signMessage NEP

Added and exported interfaces for signMessage
Added signMessage to all wallets, all of them just throws until they add the implementation
Updated guestbook in react and angular example with verify signed message

Wallets that support `signMessage` in this PR

Meteor Wallet

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings

Type of change.

frol

Disclaimer: I am just a regular reviewer, I don't have the power of code owner here, so my review comments are just suggestions.

Added signMessage to all wallets, all of them just throws until they add the implementation

I know it was a pattern used previously, but I wonder how useful this is. How to even know which wallets support this feature? Imagine I am a dApp developer, I would have a hard time integrating this feature unless I manually test all of the wallets.

packages/core/src/lib/helpers/verify-signature/payload.ts

packages/core/src/lib/helpers/verify-signature/verify-signature.ts

packages/core/docs/api/wallet.md

packages/core/docs/guides/custom-wallets.md

frol · 2023-04-26T12:04:51Z

packages/core/docs/api/wallet.md

+  let nonceArray: Uint8Array = new Uint8Array(32);
+  nonceArray = crypto.getRandomValues(nonceArray);
+  const nonce = Buffer.from(nonceArray);


If the internal implementation is in charge of the protocol security, and developers do not have any use for the nonce, I think it is fair to remove it from the public interface. What do you think?

As a dApp developer, they could use nonce to verify the signature, right? Correct me if I'm wrong.

Oh, well, validation is currently expected to be done manually by dApp developer. I think it would be great to have a higher-level API for dApp developers. This documentation example needs the validation to be featured

Do you mean adding the following to the sample?

import { verifySignature, verifyFullKeyBelongsToUser } from "@near-wallet-selector/core"; // ... const verifiedSignature = verifySignature({ message, nonce, recipient, publicKey: signedMessage.publicKey, signature: signedMessage.signature, }); const verifiedFullKeyBelongsToUser = await verifyFullKeyBelongsToUser({ publicKey: signedMessage.publicKey, accountId: signedMessage.accountId, network: selector.options.network, }); const ok = verifiedSignature && verifiedFullKeyBelongsToUser

Both functions are now exported from @near-wallet-selector/core. Not sure if we really need a 'higher-level' API for dApp dev.

Yep, that is what I missed in the example.

I like this low-level API, but I would still consider having an API that won't expect developers don't forget to do the checks. I would expect that once the message is returned, it is already verified. What would be the use case for dApp developers needing to intervene and need control over each stage and have access to a potentially maliciously constructed message?

Fair point.
I'm thinking alternating it like so, where we have a second params (verify will be defaulted to true)

const signedMessage = await wallet.signMessage({ message, recipient, }, {verify: true});

Or we just derived the verify flag from the availability of nonce field.
eg:

// This will verify the message await wallet.signMessage({ message, recipient, }) // This will NOT verify the message await wallet.signMessage({ message, nonce, recipient, })

Either way, I believe the verification part should be the responsibility of Wallet builders?

I kinda agree about making the verification step a bit more intentional from the side of the dApps, since they could potentially do their own (short-lived) caching of a valid public key per user, which would allow for instant actions on their apps instead of always doing an extra API call to check for account valid keys.

In the future, there could also be a standard text payload for simple things like "proof of account ownership" which use the same API, but don't require the user to intentionally complete the action in the wallet every single time (in the case of extension wallets, if it was done once per requesting and currently connected dApp, it could be done automatically each time afterwards using background script communication). To require each of these types of calls to still verify on the blockchain would make things like simply "liking a post" or whatnot, take a lot longer than they need to- deteriorating the user experience.

lostpebble · 2023-05-05T08:22:14Z

@frol this NEP and PR is a long time coming. We'd really like to get this implemented so our users can start making use of the feature. We've done it to the latest definition of the standard and followed previous code patterns in this project.

I know it was a pattern used previously, but I wonder how useful this is. How to even know which wallets support this feature? Imagine I am a dApp developer, I would have a hard time integrating this feature unless I manually test all of the wallets.

I think your reservations requires a larger discussion about wallet features and support, and building that into Wallet Selector. The churning of NEPs and features in this space definitely does leave a lot of feature gaps in some wallets, so its a relevant discussion to have.

For this NEP specifically, wallets have had a long time to figure out their implementations. I think sticking with the error message for now is not the end of the world- dApps can handle that error and let the user know they should use a different wallet to make use of that feature.

Again, this has really been a long time coming- the first implementation of which was verifyOwner, which not many wallets actually decided to pick up and implement. When the timeline has been visible for so long, I don't think we should slow down progress for the sake of those that drag their feet.

petarvujovic98 · 2023-06-24T18:46:54Z

packages/my-near-wallet/src/lib/my-near-wallet.ts

+    async signMessage({ message, nonce, recipient, callbackUrl, state }) {
+      logger.log("MyNearWallet:signMessage", {
+        message,
+        nonce,
+        recipient,
+        callbackUrl,
+        state,
+      });
+
+      throw new Error(`Method not supported by ${metadata.name}`);
+    },
+


I don't know why this wouldn't be supported by MyNearWallet/NearWallet since all we need to sign a message is available without needing to redirect to the wallet website. This is how it could be done:

const { contract } = store.getState(); if (!_state.wallet.isSignedIn() || !contract) { throw new Error("Wallet not signed in"); } const account = _state.wallet.account(); const { networkId } = account.connection; const accountId = account.accountId; const keyPair = await _state.keyStore.getKey(networkId, accountId); const signer = await nearAPI.InMemorySigner.fromKeyPair( networkId, accountId, keyPair ); return signer.signMessage(new Uint8Array(Buffer.from(message)));

That would sign a message using the function call access key which was added on the Dapp side. I think this API is meant to sign messages with a full access key, which is only available on the wallet side.

Which makes sense- because otherwise you're handing DApps "signing power" when you sign into them. Only Full Access keys should be allowed to be the source of a wallet account's signed message.

petarvujovic98 · 2023-06-24T18:56:12Z

What is the blocker for this PR? It's something that would greatly enhance the BOS experience, being able to sign messages and authenticate with third party servers without having to make the users sign in again and handle the sign in state separately would enable for a much more comprehensive experience.

This would enable developers to enhance the UX of apps on BOS, the prime example being Horizon. There are some flows users should be able to perform (mostly including sensitive data that shouldn't be stored on-chain - or at least not without encrypting it first, but that's a whole different issue). And enabling message signing would make this flow's for one doable and for two it would be a better experience for auth on servers compared to current Web2.0 primitives making it an additional selling point.

amirsaran3 · 2023-07-06T07:53:15Z

Hey @Elabar ,
When I was testing this feature I got this error:

Can you help me with this?

Elabar · 2023-07-06T11:32:37Z

Hi, it is a bug on our extension. Can you try again with extension disabled for now?
Will update here when the fix for extension is deployed.

amirsaran3 · 2023-07-10T05:49:42Z

Hi, it is a bug on our extension. Can you try again with extension disabled for now? Will update here when the fix for extension is deployed.

Hey, what do you mean to disable the extension? If I disable it I will not be able to use this feature at all with Meteor wallet.

Is this feature still in progress for the Meteor wallet?

lostpebble · 2023-07-20T17:48:21Z

Hi, it is a bug on our extension. Can you try again with extension disabled for now? Will update here when the fix for extension is deployed.

Hey, what do you mean to disable the extension? If I disable it I will not be able to use this feature at all with Meteor wallet.

Is this feature still in progress for the Meteor wallet?

A new version of the extension has been released and it should solve this issue now.

AZbang · 2023-07-22T12:00:27Z

HERE Wallet already support signMessage method. I support that this should be implemented by other wallets as soon as possible. 👍

…s not implemented.

kujtimprenkuSQA · 2023-08-01T08:53:03Z

HERE Wallet already support signMessage method. I support that this should be implemented by other wallets as soon as possible. 👍

Hi, @AZbang after trying out the current implementation of signMessage on HERE Wallet it looks like it's not aligned with the latest specification as it's stated in the NEP: near/NEPs#413 .

The params this function takes are not exactly as in the NEP: https://github.com/here-wallet/js-sdk/blob/main/src/wallet.ts#L255 , can you double-check and implement it as stated in the NEP?

Also, it would be great to show some information for users like Meteor Wallet at this step:

kujtimprenkuSQA · 2023-08-01T09:24:11Z

@Elabar thank you for submitting this PR, we are planning to merge the PR soon but we have a few suggestions and changes that might need to make.

We are planning to remove the empty signMessage which by default throws from wallets that don't implement it and just decorate modules in the core package, this is something that we plan to do for signInMulti function too based on this comment: #811 (comment)

I have already prepared a PR based your current work: Elabar#1

Remove empty signMessage from wallets that do not implement it

Elabar · 2023-08-01T13:42:53Z

@kujtimprenkuSQA Thanks for preparing the changes! It is merged now.

This adds the signMessage at the types when getting the module/wallet from the selector at runtime so typescript does not complain and there is no need to check if this function is defined or not.

kujtimprenkuSQA · 2023-08-01T14:19:49Z

@kujtimprenkuSQA Thanks for preparing the changes! It is merged now.

Thanks for merging it, I will be adding a small fix based on this comment: Elabar#1 (comment) .

You can check the new PR here: Elabar#2 .

Define signMessage when accessing a wallet/module

kujtimprenkuSQA · 2023-08-02T07:53:01Z

Hi, @Elabar do you mind addressing the conflicts?

Elabar · 2023-08-02T10:23:51Z

@kujtimprenkuSQA done

kujtimprenkuSQA · 2023-08-02T11:13:27Z

@kujtimprenkuSQA done

Thanks, can you run yarn lint --verbose to find and fix the linting issues I think after that the PR is good to be merged.

Elabar · 2023-08-02T13:34:00Z

fixed linting issue.

erditkurteshiSQA · 2023-08-31T12:21:37Z

Hi @Elabar,

The signMessage is not working correctly on Meteor Wallet. Can you guys check if the implementation of the signMessage
is as in the example of the NEP: https://github.com/gagdiez/near-login/blob/main/tests/authentication/wallet.ts#L60

lostpebble · 2023-08-31T21:03:57Z

Hi @erditkurteshiSQA , @Elabar currently is occupied elsewhere. Can direct any Meteor stuff my way.

We'll look into this now- thanks for reporting the issue!

lostpebble · 2023-09-01T15:32:03Z

Hi @erditkurteshiSQA - would you mind following up with some more context in this issue please: Meteor-Wallet/meteor-public#5

Elabar added 2 commits April 20, 2023 22:48

feat: added sign message to all wallets and guestbook example

248b8f2

feat: implement sign message for meteor wallet

5fc0124

github-actions bot changed the title ~~Add signMessage method~~ feat: Add signMessage method Apr 25, 2023

frol reviewed Apr 26, 2023

View reviewed changes

fix suggestions

b060ce0

petarvujovic98 approved these changes Jun 24, 2023

View reviewed changes

kujtimprenkuSQA added 3 commits July 28, 2023 08:40

Make signMessage optional.

016e1ad

Decorate modules to throw consistent error message when signMessage i…

6c8898c

…s not implemented.

Remove signMessage from wallets that do not support it yet.

a63dcb4

Add await to _signMessage

22f7786

Merge pull request #1 from kujtimprenkuSQA/meteor-sign-message-optional

1f69772

Remove empty signMessage from wallets that do not implement it

Define signMessage when getting the module/wallet from the selector

73d57aa

This adds the signMessage at the types when getting the module/wallet from the selector at runtime so typescript does not complain and there is no need to check if this function is defined or not.

Merge pull request #2 from kujtimprenkuSQA/fix-sign-message-at-runtime

0a5c299

Define signMessage when accessing a wallet/module

Merge branch 'dev' into meteor-sign-message

451e1ab

chore: fix linting issue

bbc4331

kujtimprenkuSQA approved these changes Aug 2, 2023

View reviewed changes

kujtimprenkuSQA merged commit 465e7b5 into near:dev Aug 2, 2023
5 checks passed

This was referenced Aug 3, 2023

v8.3.0 Release (dev -> main) #871

Merged

Create Example for signMessage #434

Closed

lostpebble mentioned this pull request Sep 1, 2023

signMessage interface not matching example in NEP Meteor-Wallet/meteor-public#5

Closed

charleslavon mentioned this pull request Sep 6, 2023

Support signmessage testing near/near-discovery#360

Merged

kujtimprenkuSQA mentioned this pull request Jan 24, 2024

Expose API to sign arbitrary data #405

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add `signMessage` method #779

feat: Add `signMessage` method #779

Elabar commented Apr 25, 2023 •

edited

Loading

frol left a comment

frol Apr 26, 2023

Elabar Apr 26, 2023

frol Apr 26, 2023

Elabar Apr 27, 2023

frol Apr 27, 2023

Elabar Apr 27, 2023 •

edited

Loading

lostpebble May 2, 2023

lostpebble commented May 5, 2023

petarvujovic98 Jun 24, 2023

lostpebble Jun 26, 2023

lostpebble Jun 26, 2023

petarvujovic98 commented Jun 24, 2023

amirsaran3 commented Jul 6, 2023

Elabar commented Jul 6, 2023

amirsaran3 commented Jul 10, 2023 •

edited

Loading

lostpebble commented Jul 20, 2023

AZbang commented Jul 22, 2023

kujtimprenkuSQA commented Aug 1, 2023

kujtimprenkuSQA commented Aug 1, 2023

Elabar commented Aug 1, 2023

kujtimprenkuSQA commented Aug 1, 2023

kujtimprenkuSQA commented Aug 2, 2023

Elabar commented Aug 2, 2023

kujtimprenkuSQA commented Aug 2, 2023 •

edited

Loading

Elabar commented Aug 2, 2023

erditkurteshiSQA commented Aug 31, 2023

lostpebble commented Aug 31, 2023 •

edited

Loading

lostpebble commented Sep 1, 2023

feat: Add signMessage method #779

feat: Add signMessage method #779

Conversation

Elabar commented Apr 25, 2023 • edited Loading

Description

Wallets that support signMessage in this PR

Checklist:

Type of change.

frol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Elabar Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lostpebble commented May 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petarvujovic98 commented Jun 24, 2023

amirsaran3 commented Jul 6, 2023

Elabar commented Jul 6, 2023

amirsaran3 commented Jul 10, 2023 • edited Loading

lostpebble commented Jul 20, 2023

AZbang commented Jul 22, 2023

kujtimprenkuSQA commented Aug 1, 2023

kujtimprenkuSQA commented Aug 1, 2023

Elabar commented Aug 1, 2023

kujtimprenkuSQA commented Aug 1, 2023

kujtimprenkuSQA commented Aug 2, 2023

Elabar commented Aug 2, 2023

kujtimprenkuSQA commented Aug 2, 2023 • edited Loading

Elabar commented Aug 2, 2023

erditkurteshiSQA commented Aug 31, 2023

lostpebble commented Aug 31, 2023 • edited Loading

lostpebble commented Sep 1, 2023

feat: Add `signMessage` method #779

feat: Add `signMessage` method #779

Elabar commented Apr 25, 2023 •

edited

Loading

Wallets that support `signMessage` in this PR

Elabar Apr 27, 2023 •

edited

Loading

amirsaran3 commented Jul 10, 2023 •

edited

Loading

kujtimprenkuSQA commented Aug 2, 2023 •

edited

Loading

lostpebble commented Aug 31, 2023 •

edited

Loading