-
Notifications
You must be signed in to change notification settings - Fork 249
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
Implement signMessage method #1210
Implement signMessage method #1210
Conversation
🦋 Changeset detectedLatest commit: 661cc65 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Before this patch the logic was:
After this patch, the logic is: during requestSignIn we always create a key. The problem is that creating a key with no The issue mentioned (#678) is not an issue, but an UX decision. The right way to go is: login users through wallet selector, and then ask the user to sign a message using |
} | ||
|
||
const accessKey = KeyPair.fromRandom('ed25519'); |
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.
This flow will create a full access key if no contractId
is given
@gagdiez Thank you for the quick and thorough feedback! @vikinatora The issue predates the NEP-0413 signMessage. The issue has never been properly triaged, and what @gagdiez said is true, instead of leading users of near-api-js down the wrong path of creating a useless Full Access key, we should integrate signMessage to just "confirm that the Wallet is authenticated and thus will be usable for transaction signing when it will be necessary in the future":
"signMessage" adoption and implementation in wallet-selector also got stuck: near/wallet-selector#883, and it would be awesome if you will be able to push it through the finish line. In order to be productive, please, comment on the issues with some initial thoughts on the implementation you plan to execute and mention @gagdiez and myself, so we can help you to avoid going a false route. |
@gtsonevv @frol please notice that Here is how the |
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.
Unless I miss something, this is not the right direction.
I am not even sure if anything actually needs to be added in near-api-js at all. I am not familiar with the current relationship with wallet-selector, but as a developer who wants to add "login" that won't need to have a limited access key, I would integrate Wallet Selector and use signMessage
there: https://github.com/search?q=repo%3Anear%2Fwallet-selector%20signMessage&type=code
@gagdiez Do you have more context here or can you invite someone else?
Hi, I am a member of the wallet-selector team. Adding the Since it's not recommended to have a @gagdiez what do you think? |
@gtsonevv So let me try to explain all the context regarding #678. Here is the original request:
So the request was to authenticate a user - confirm that they have access to the account, and if there would be any requests that needs to be sent on behalf of the user (authorize) those will require another interaction with the wallet, the dApp won't be signing those requests. This is done this way to avoid dApps requesting Full Access keys for general operatons, thus minimize chances of leaking user's private key. The flow was historically called "verifyOwner", but then it turned into a more generic "signMessage" standard:
Regarding the "random message", it can be just |
@kujtimprenkuSQA @gagdiez @andy-haynes That being said, I believe this "verifyOwner" belongs to wallet-selector library, and not near-api-js, which tries to be platform-agnostic and such features do not make sense for Node.js. Should we document how to use wallet-selector for the scenario reported in #678 and close this issue? |
I agree that "verifyOwner" -> "signMessage" should belong to wallet-selector where the API is consistent for different wallets. There's documentation for the "login" with signMessage in near docs: The signMessage API is documented in the wallet selector too: The original issue here was opened before the NEP-413 was approved. Do we need more documentation than what currently is available? |
I agree that this needs to be done by a wallet, since a fullAccessKey needs to be used, lets not add it to |
Pre-flight checklist
pnpm changeset
to create achangeset
JSON document appropriate for this change.Motivation
Implement
signMessage
, that allows users to sign a message for a specific recipient using their NEAR account. It returns theSignedMessage
based on the NEP413.Test Plan
Added unit tests
Related issues/PRs
Issue #678