-
Notifications
You must be signed in to change notification settings - Fork 137
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
Add support for signing in to more than one contract at once to Injected Wallet Standard #428
Conversation
`SignInParams` should have a `permissions?: Array<transactions.FunctionCallPermission>;` instead of `permission: transactions.FunctionCallPermission;` This change is to provide more information for these two cases: - We will have cases when we want to sign in without creating a FunctionCall access key - We will have cases when we want to create more than one FunctionCall access key if our dApp uses multiple contracts
Thank you @amirsaran3 for submitting this NEP. As a moderator, I reviewed this NEP extension and it meets the proposed template guidelines. I am moving this NEP to the REVIEW stage and would like to ask the @near/wg-wallet-standards members to assign 2 Reviewers (Subject Matter Experts). If you want to assign yourself, please mention that you are acting as the Technical Reviewers. Please find some guidelines below for completing the technical review. Technical Review Guidelines
Please tag the @near/nep-moderators once you are done, so we can move this NEP to the voting stage. Thanks again. |
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.
Just a few questions :)
1. Added `changeNetwork` method - This method needs to exists for dApps to sync active/current network with wallet. Currently only the dApp can know when the wallet changes network using an event. This method alows the dApp to inform the wallet when it has changed its network. 2. Added missing description for `supportsNetwork` method
|
I've made some updates. The only change to the I've created a new |
@MaximusHaximus I updated the permission parameter. |
Does this need the wallet working group to assign 2 SMEs? |
@nearbuild Yes, I believe so. As a moderator, I would like to ask the @near/wg-wallet-standards working group members to assign 2 Technical Reviewers to complete a technical review (see expectations). If you want to assign yourself, please mention that you are acting as the Technical Reviewers. Please find some guidelines below for completing the technical review. Technical Review Guidelines
Technical Summary guidelines:
Here is a nice example and a template for your convenience:
Please tag the @near/nep-moderators once you are done, so we can move this NEP to the voting stage. Thanks again. |
As a working group member, I’d like to nominate @andy-haynes as a Subject Matter Expert to review this NEP. |
We already maintain previous multi-transactions standard in HERE Wallet
https://docs.herewallet.app/tools-for-dapps/universal-sign-links |
Moreover, this standard does not require a |
Hi,
|
As a Working Group member, I would like to nominate Edward Chew (@edwardchew97) from Meteor Wallet to be a Subject Matter Expert to review this proposed NEP update. |
@pvolnov is there an NEP for this? The proposal in this PR seeks to extend an existing API in a specific use case, providing a straightforward way for dApps to continue using the |
Yes, in the sense that you could now create multiple function call access keys in a single transaction (per account).
The user would be prompted to sign the transaction responsible for adding the function call access keys; there is no way for the caller of this method to hide or falsify information on the new keys. So long as the wallet implementation is clearly displaying all actions on the transaction when prompting for signing, the nature of these new access keys would be clear. My assumption is that most dApps would be specifying contracts they own, likely sub accounts under a dedicated account, simplifying the determination as to whether they are safe. @amirsaran3 feel free to add/correct anything |
Technical SummaryThis NEP update proposes adding a new It is important to note that RecommendationI endorse the approval of this update. Benefits
Concerns
|
SummaryThe introduction of a RecommendationI recommend the NEP to be approved. BenefitsDApp developers using a multi-contract architecture would be able to present a near-identical UX to wallets, providing a more simple and reliable login workflow. ConcernsI have nothing to new to add without restating the cogent points raised by @edwardchew97
|
As the moderator, I would like to thank @edwardchew97 and @andy-haynes for completing the technical review. Based on your comments above, it seems like this proposal is ready for the working group members for review. @near/wg-wallet-standards – Can you please fully read this NEP and comment in the thread if you are leaning towards approving or rejecting it? Please make sure to include your rationale and any feedback that you have for the author. Once we get 2/3 voting indications, we will schedule a public call for the author to present the NEP and for the working group members to formalize the voting. |
As a Wallet working group member, I approve this NEP because it enables developers to specify which specific methods on smart contracts to permit and it will give users the option to opt-in to a single non-payable transaction to skip approval for different function cal access key. Based on previous discussions:
|
SummaryThe introduction of a signInMulti method to the injected wallet interface enables dApp developers to facilitate logins for multiple contracts in a single transaction, functionality not currently supported by the signIn method. RecommendationOverall, recommend the NEP to be approved, but I do think that it should be modified slightly from its current proposal, in particular to make the signIn request specific to a single account rather than multiple accounts. BenefitsDApp developers using a multi-contract architecture would be able to present a near-identical UX to wallets, providing a more simple and reliable login workflow. Concerns2. Near Wallet selector's signIn implementation takes only one account and the current implementation allows an array of accounts. This creates inconsistency between the standard and implementation.I believe this is a valid concern -- the UI a wallet would need to display would be very complex and the flow for approving multiple transactions (1 per account) has a lot of possible failure modes compared to a single account. Furthermore, it makes the concept of being signed in on the dapp side far more complex, since you could be signed into more than 1 account with different contracts for each account, and when the limited-access-key allowance is exhausted for one of those contract <-> account pairings, the dapp and wallet UI experience for replenishing just that one key for one account will be made more complex also; atomicity in signing in per account makes all of these challenges easier to handle. 3. A malicious website could request signIn or signInMulti for users' fungible token smart contract and drain their wallets by calling ft_transfer. This is more likely to happen if the wallet doesn't interpret and show the target smart contract properly.I believe this isn't a big problem, since signing in doesn't create a full access key (FAK), but instead a limited access key (LAK) -- according to NEP141 an NEP171 specifications, you must attach a Barriers to implementation / changes in concerns for dapp developersI'd like to bring attention to two knock-on effects that need to be considered: A. Keystores - Currently, no keystores implemented by the NEAR JS ecosystem allow multiple keys to be stored for the same account ID at once; LocalStorageKeystore only contains a mapping of B. Limited Access Key Possible Bloat - Currently, when the LAK that a dapp has been using as a result of a previous sign in has run out of available allowance, the dapp developer must catch the |
As the moderator, I would like to thank the @amirsaran3 for submitting this Wallet Standards NEP, and for the @near/wg-wallet-standards working group members for your review. Based on the voting indications above, it seems like this proposal is close to a decision. We are therefore scheduling a Wallet Standards Working group meeting this week, where this NEP can enter the final voting stage. Anyone can discuss the technical details by adding your comments to this discussion. And join the call to learn about the final decision and how to get more involved. Meeting Info @amirsaran3 – please let me know if you'll be able to make the call and the best way to contact you to share the detailed agenda. |
As a wallet working group member, I lean towards approving this NEP. BenefitsThis NEP will optimize UX for multi contract DApps and avoid multiple redirects. These are more and more common in the ecosystem and this NEP will benefit the UX for those DApps. Concerns
|
Thanks for the submission @amirsaran3! Could you make the following modifications to this NEP?
Then we should be ready to land this! |
The Wallet Standards Work Group met today to review this NEP extension and reached the following consensus: Status: Approved
Meeting Recording: Thank you @amirsaran3 for authoring this NEP! And for all the participants who were involved during the review process. Next Steps:
|
I updated the PR. |
Whats the status on this @frol thought it was voted on? |
Hi, we have worked on adding the |
Added
signInMulti
method to solve the case where the dApp has more than one contract.NEP Status (Updated by NEP moderators)
SME reviews:
Wallet Standards WG voting indications: