Skip to content
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

Merged
merged 13 commits into from
May 19, 2023

Conversation

amirsaran3
Copy link
Contributor

@amirsaran3 amirsaran3 commented Nov 14, 2022

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:

`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
@amirsaran3 amirsaran3 requested a review from a team as a code owner November 14, 2022 11:39
@ori-near ori-near added WG-wallet-standards Wallet Standards Work Group should be accountable A-NEP-Extension A new functionality proposal to existing NEP. Once original author merges changes, we close this. S-review/needs-wg-to-assign-sme A NEP that needs working group to assign two SMEs. labels Dec 5, 2022
@ori-near
Copy link
Contributor

ori-near commented Dec 5, 2022

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
  • First, review the proposal within one week. If you have any suggestions that could be fixed, leave them as comments to the author. It may take a couple of iterations to resolve any open comments.
  • Second, once all the suggestions are addressed, produce a Technical Summary, which helps the working group members make a weighted decision faster. Without the summary, the working group will have to read the whole discussion and potentially miss some details.
    Technical Summary guidelines:
    • A recommendation for the working group if the NEP is ready for voting (it could be approving or rejecting recommendation). Please note that this is the reviewer's personal recommendation.
    • A summary of benefits that surfaced in previous discussions. This should include a concise list of all the benefits that others raised, not just the ones that the reviewer personally agrees with.
    • A summary of concerns or blockers, along with their current status and resolution. Again, this should reflect the collective view of all commenters, not just the reviewer's perspective.
      Here is a nice example and a template for your convenience:
## Example
### Recommendation
Add recommendation
### Benefits
* Benefit
* Benefit
### Concerns
| # | Concern | Resolution | Status |
| - | - | - | - |   
| 1 | Concern | Resolution | Status |
| 2 | Concern | Resolution | Status |

Please tag the @near/nep-moderators once you are done, so we can move this NEP to the voting stage. Thanks again.

Copy link
Contributor

@MaximusHaximus MaximusHaximus left a 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 :)

specs/Standards/Wallets/InjectedWallets.md Outdated Show resolved Hide resolved
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
@amirsaran3 amirsaran3 changed the title SignInParams wrong permission parameter Update Injected Wallet Standard Dec 21, 2022
@MaximusHaximus
Copy link
Contributor

@amirsaran3

  1. Per our last meeting, please remove the portion of this PR that makes network change related modifications, especially given that the current communication flow is driven from the dapp rather than from the wallet, and that account IDs cannot be the same on testnet and mainnet due to the fact that the top-level accounts are different and the same seed phrase used on testnet will result in a different implicit account than that same seed phrase used on mainnet (due to BIP39)

  2. Unfortunately, this change as written will be a breaking change to a very low-level/fundamental API that all dapps use; we can't control how quickly dapps upgrade their wallet-selector implementation, nor can we control the speed at which wallets support changes to the API, so we need to make this a non-breaking change.

    • Instead of making a breaking change to the existing API, I suggest that we propose an entirely new method be added to the API -- my suggestion would be something like signInMulti({ accounts: Array<string>, permissions: Array<transactions.FunctionCallPermission> }). By making this a separate method, we gain two benefits:
      • We can easily detect support for the new API
      • We can ensure that this functionality does not represent a breaking change for existing wallets

@amirsaran3
Copy link
Contributor Author

@amirsaran3

  1. Per our last meeting, please remove the portion of this PR that makes network change related modifications, especially given that the current communication flow is driven from the dapp rather than from the wallet, and that account IDs cannot be the same on testnet and mainnet due to the fact that the top-level accounts are different and the same seed phrase used on testnet will result in a different implicit account than that same seed phrase used on mainnet (due to BIP39)

  2. Unfortunately, this change as written will be a breaking change to a very low-level/fundamental API that all dapps use; we can't control how quickly dapps upgrade their wallet-selector implementation, nor can we control the speed at which wallets support changes to the API, so we need to make this a non-breaking change.

    • Instead of making a breaking change to the existing API, I suggest that we propose an entirely new method be added to the API -- my suggestion would be something like signInMulti({ accounts: Array<string>, permissions: Array<transactions.FunctionCallPermission> }). By making this a separate method, we gain two benefits:

      • We can easily detect support for the new API
      • We can ensure that this functionality does not represent a breaking change for existing wallets

I've made some updates.

The only change to the signIn method will be that the permission will be optional.

I've created a new signInMulti method that will have permissions as an argument and that will take in an array.

@amirsaran3
Copy link
Contributor Author

@MaximusHaximus I updated the permission parameter.

@nearbuild
Copy link

Does this need the wallet working group to assign 2 SMEs?

@frol
Copy link
Collaborator

frol commented Mar 14, 2023

@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
  • First, review the proposal within one week. If you have any suggestions that could be fixed, leave them as comments to the author. It may take a couple of iterations to resolve any open comments.

  • Second, once all the suggestions are addressed, produce a Technical Summary, which helps the working group members make a weighted decision faster. Without the summary, the working group will have to read the whole discussion and potentially miss some details.

Technical Summary guidelines:

  • A recommendation for the working group if the NEP is ready for voting (it could be approving or rejecting recommendation). Please note that this is the reviewer's personal recommendation.

  • A summary of benefits that surfaced in previous discussions. This should include a concise list of all the benefits that others raised, not just the ones that the reviewer personally agrees with.

  • A summary of concerns or blockers, along with their current status and resolution. Again, this should reflect the collective view of all commenters, not just the reviewer's perspective.

Here is a nice example and a template for your convenience:


## Example

### Recommendation

Add recommendation

### Benefits

* Benefit

* Benefit

### Concerns

| # | Concern | Resolution | Status |

| - | - | - | - |   

| 1 | Concern | Resolution | Status |

| 2 | Concern | Resolution | Status |

Please tag the @near/nep-moderators once you are done, so we can move this NEP to the voting stage. Thanks again.

@esaminu
Copy link
Contributor

esaminu commented Mar 22, 2023

As a working group member, I’d like to nominate @andy-haynes as a Subject Matter Expert to review this NEP.

@pvolnov
Copy link

pvolnov commented Mar 22, 2023

We already maintain previous multi-transactions standard in HERE Wallet
why we need new one?

{
    "transactions": [
        {
            "actions": {
                    "type": "function call",
                    "params": {
                        "gas": 30000000000000,
                        "method_name": "hi",
                        "args":"",
                        "deposit": 1
                    },
                },
            "receiverId": "bob.near",
        }
    ],
    "network": "mainnet",
}

https://docs.herewallet.app/tools-for-dapps/universal-sign-links

@pvolnov
Copy link

pvolnov commented Mar 22, 2023

Moreover, this standard does not require a signer_id and dApps will be able to get rid of mandatory authorization

@Pawel-Szydlo
Copy link

Hi,
I have a few questions:

  • Does the new functionality allow you to add permissions to several contracts simultaneously?
  • What happens if a party is dishonest and adds false information to unsafe contracts? Will I be able to see the names of the contracts on the approval pop-up

@Cameron-Banyan
Copy link

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.

@andy-haynes
Copy link

We already maintain previous multi-transactions standard in HERE Wallet
why we need new one?

@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 signIn-based approach. If there are viable alternatives for dApps seeking a solution to this then those would ostensibly be options for the dApp developer as well.

@andy-haynes
Copy link

@Pawel-Szydlo

Does the new functionality allow you to add permissions to several contracts simultaneously?

Yes, in the sense that you could now create multiple function call access keys in a single transaction (per account).

What happens if a party is dishonest and adds false information to unsafe contracts? Will I be able to see the names of the contracts on the approval pop-up

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

@edwardchew97
Copy link

edwardchew97 commented Mar 30, 2023

Technical Summary

This NEP update proposes adding a new signInMulti method that allows the creation of function calls access keys for multiple smart contracts at once. Currently, we have a signIn method but it only allows a single smart contract signIn.

It is important to note that signIn and connect are different, with signIn generating a function call access key that may allow a DAPP to sign non-payable transactions on behalf of users.

Recommendation

I endorse the approval of this update.

Benefits

  1. The signInMulti method will enable a more seamless user experience for dapps that require function call access keys for multiple smart contracts, which is likely to become more common with the introduction of BOS and gateways.
  2. This update will provide a better developer experience by eliminating the need for developers to initiate multiple transactions and check their output one by one.

Concerns

# Concern Resolution Status
1 Backward compatibility. As @MaximusHaximus suggested, the update should not break existing dapps. The existing signIn method should not be updated Resolved.
2 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 recommend that the signInMulti method take in only one account, as it is uncommon for users to want to create function call keys for multiple accounts at once. Creating multiple keys for multiple accounts is complex to display and costs higher storage fees. This is currently also an issue with the signIn method and is not being addressed yet but it might be better to address them in another proposal.
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. Wallets should display contract ids properly during sign-in and potentially detect if they are FT or NFT contracts by checking if certain key methods are implemented. This doesn't fall under the responsibility of this update but the update introduces a bigger risk. We might need to push this to be implemented on the wallet selector side.

@andy-haynes
Copy link

andy-haynes commented Mar 30, 2023

Summary

The 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.

Recommendation

I recommend the NEP to be approved.

Benefits

DApp 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.

Concerns

I have nothing to new to add without restating the cogent points raised by @edwardchew97

  1. I agree with the assessment that this is an awkward signature inherited by the existing signIn method and that it would make sense to address this in the context of operations taken against individual accounts as opposed to a set of accounts.

  2. This is an interesting point on the FT contract scenario that I had not considered. Wallets having some awareness of the contracts deployed to accounts it manages certainly sounds like it would mitigate some risk around this, but I agree that it is part of a broader topic not directly relevant to this NEP.

@ori-near
Copy link
Contributor

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.

@ori-near ori-near added S-voting/needs-wg-voting-indication A NEP in the VOTING stage that needs the working group voting indication. and removed S-review/needs-wg-to-assign-sme A NEP that needs working group to assign two SMEs. labels Mar 31, 2023
@Cameron-Banyan
Copy link

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:

  • The NEP addresses the concerns of backward compatibly with no breaking changes.

  • As addressed by @edwardchew97 - although it is uncommon for users to want to create function calls keys for multiple accounts, I believe the option to do so can enable use cases for those who are interacting with multiple accounts at the same time for something like financial transactions. (ie: approving two accounts for HideYourCash + Rainbow Bridge).

  • Although it will cost storage fees and be more complex to display, wallets will have the option to display this alongside contract_ids and the type of contract by detecting certain methods or through the passed NEP351 (feat: add standards key to ContractSourceMetadata #351). I would appreciate if a reference implementation on displaying signInMulti with multiple function access keys on multiple accounts, contract detection, and gas estimation to show Wallet Builders to increase adoption of this NEP.

  • If the design choice for just 1 account is favored, then I agree with @edwardchew97 that this be addressed in a separate proposal.

@MaximusHaximus MaximusHaximus changed the title Update Injected Wallet Standard Add support for signing in to more than one contract at once to Injected Wallet Standard Apr 14, 2023
@MaximusHaximus
Copy link
Contributor

Summary

The 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.

Recommendation

Overall, 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.

Benefits

DApp 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.

Concerns

2. 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 deposit of 1 yoctoNEAR in order to call ft_transfer or any other methods that can cause ownership of an asset to change, which forces a FAK to be used to sign any transactions that are dangerous. Because of this requirement, such transactions must be done via a wallet which should be programmed to show the user the action they are approving before signing it.

Barriers to implementation / changes in concerns for dapp developers

I'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 accountId->Key. In order to allow dapps to actually seamlessly sign transactions for more than 1 key on the same account without hacking their own persistence and keystores, we will need a new keystore that can store keys in a mapping of accountId + contractId -> Key.

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 NotEnoughAllowance error and prompt the user to sign in to their dapp again. With the introduction of multiple keys, this could lead to 'key bloat' if the dapp developer always requests the user re-sign in for all of the contracts that they had originally been issued LAKs for. Since it is highly unlikely all of the keys will run out of allowance at once, signing in again for all contracts will probably leave keys behind with allowances on them that are not exhausted, which may confuse the user.

@ori-near
Copy link
Contributor

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
📅 Date: Thursday, April 20, 5pm UTC
✏️ Register

@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.

@ori-near ori-near added S-voting/final-comment-period A NEP in the final REVIEW stage. Last chance to bring up concerns before the voting call. and removed S-voting/needs-wg-voting-indication A NEP in the VOTING stage that needs the working group voting indication. labels Apr 17, 2023
@esaminu
Copy link
Contributor

esaminu commented Apr 18, 2023

As a wallet working group member, I lean towards approving this NEP.

Benefits

This 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

  • The currently available keystores will have to catch up in order to support multiple keys per account
  • We should add the new method to the Wallet interface for clarity in the NEP doc

@esaminu
Copy link
Contributor

esaminu commented Apr 20, 2023

Thanks for the submission @amirsaran3!

Could you make the following modifications to this NEP?

  1. Make signInMulti and signIn take a single Account instead of Array<Account> in the argument
  2. Add signInMulti method to the Wallet interface
  3. Embed the Benefits & Concerns section to the NEP document for future reference

Then we should be ready to land this!

@ori-near
Copy link
Contributor

The Wallet Standards Work Group met today to review this NEP extension and reached the following consensus:

Status: Approved

Meeting Recording:
https://youtu.be/l-cUuFCylKY

Thank you @amirsaran3 for authoring this NEP! And for all the participants who were involved during the review process.

Next Steps:

  • @amirsaran3 Please complete the steps @esaminu outlined above
  • @near/nep-moderators To approve and merge the NEP
  • [TBD] Add supporting keystore to NAJ
  • [TBD] Add wallet selector interface

@amirsaran3
Copy link
Contributor Author

Thanks for the submission @amirsaran3!

Could you make the following modifications to this NEP?

  1. Make signInMulti and signIn take a single Account instead of Array<Account> in the argument
  2. Add signInMulti method to the Wallet interface
  3. Embed the Benefits & Concerns section to the NEP document for future reference

Then we should be ready to land this!

I updated the PR.

@nearbuild
Copy link

Whats the status on this @frol thought it was voted on?

@frol frol merged commit ce3e46d into near:master May 19, 2023
@frol frol added S-approved A NEP that was approved by a working group. and removed S-voting/final-comment-period A NEP in the final REVIEW stage. Last chance to bring up concerns before the voting call. labels May 19, 2023
@kujtimprenkuSQA
Copy link

Hi, we have worked on adding the signInMulti to the API of the Wallet Selector, we would appreciate if the contributors on this NEP update could take a look/review the PR in Wallet Selector: near/wallet-selector#811 so that we're all on the same page, Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NEP-Extension A new functionality proposal to existing NEP. Once original author merges changes, we close this. S-approved A NEP that was approved by a working group. WG-wallet-standards Wallet Standards Work Group should be accountable
Projects
Status: APPROVED NEPs
Development

Successfully merging this pull request may close these issues.