Make account optional for SignInParams and SignInMultiParams on Injected Wallets

[kujtimprenkuSQA]

The Standard for Injected Wallets requires that an account must be passed to SignInParams and SignInMultiParams but since the standard has been set in place none of the wallets follow this approach and some of them keep the LAK inside the wallet.

While working on adding the singInMulti based on NEP-428 to the Wallet Selector near/wallet-selector#811 we noticed that this change is necessary for wallets that need to do the signIn and signInMulti implementation in a similar way, so the passing of an account does not become a blocker during implementation.

[frol]

As a moderator, I see this change to affect just 2 lines of NEP, but given that it is technically a breaking change I have to invite @near/wg-wallet-standards experts to review them. I feel that given that this NEP is not widely implemented yet, we can keep this NEP voting lightweight, so post a casual review comment, and once we're all good, let's merge it.

[robert-zaremba]

This is changing the parameter from required to optional. This means it doesn't change existing software (all code will continue to work).

[robert-zaremba]

@kujtimprenkuSQA could you also include motivation (from the PR description) in the document itself.

[kujtimprenkuSQA]

@kujtimprenkuSQA could you also include motivation (from the PR description) in the document itself.

Let me know if we need to change the wording or move the explanation to a different place in the document.

[esaminu]

@kujtimprenkuSQA Thanks for this change! Could we indicate on the document the behavior of wallets when the account is not passed in to these methods?

[kujtimprenkuSQA]

@kujtimprenkuSQA Thanks for this change! Could we indicate on the document the behavior of wallets when the account is not passed in to these methods?

If we don't pass the account to signIn and signInMulti from dApp/wallet-selector then the wallet will be responsible for creating the keyPair and exposing the LAK to the dApp. (this is how signIn is currently implemented by existing wallets).

The only way we could send an account is if a wallet is fully implemented according to the current Standard:

• Call the connect method and select the account
• The account will be exposed to window.near.wallet.accounts
• dApp/ wallet-selector creates a keyPair for the account
• Call the signIn method which adds the key to the chain with a addKey transaction.
• On successful signIn the dApp/ wallet-selector saves the LAK key in the localStorage.

This ensures that the dApp holds the limited access key and can sign transactions that don't require to be confirmed by the wallet.

[Cameron-Banyan]

Thank you @kujtimprenkuSQA for bringing this up!

As a Wallet Working Group member, I think we should merge the minimal changes suggested to provide a better user experience by ensuring dapps hold limited access keys and can sign transactions without confirmation from the wallet.

This way, wallets have the option not to be responsible for creating the key pair and exposing the limited access key to the dApp. I also appreciate how the account param in the SignInParams and SignInMultiParams is not a blocker for wallets that don’t need an account during sign-in.

This checks out to me and I’m supportive of it.

[frol]

@esaminu @MaximusHaximus Do you support this proposal?

[esaminu]

@kujtimprenkuSQA I like the new interface but I don't understand how the LAK will be exposed to the dapp if we return Promise<void> for the signIn methods:

NEPs/specs/Standards/Wallets/InjectedWallets.md

Lines 112 to 113 in c97a640

	signIn(params: SignInParams): Promise<void>;
	signInMulti(params: SignInMultiParams): Promise<void>;

Is the reason current injected wallets do this so that the user selects the account on the wallet instead of the dapp? If so maybe we can move the current functionality of the NEP into a signInWithAccount or similar and have signIn and signInMulti return the accountId that was signed in for and it's LAKs? Although I suspect the reason we are avoiding this is to avoid transmission of the sensitive data

[kujtimprenkuSQA]

@esaminu
It looks like there was a bit of a misunderstanding regarding why this change is requested:

The change was requested to give the option to wallets to implement signInMulti in a similar way to how they have implemented signIn.

The reason why I said that for wallets to add support for signInMulti might be a "blocker" is:

If this change is not made then before they add support for signInMulti wallets will need to implement the connect method to expose the accountId which was selected to window.near.wallet.accounts this way we can create the keyPair in dApp/wallet-selector and send the account (accountId and publicKey) to signInMulti.

NOTE: Currently signIn is not implemented by any Injected Wallet as in the Standard.

---

To answer the questions:

I like the new interface but I don't understand how the LAK will be exposed to the dApp if we return Promise for the signIn methods:

The job of signIn method is to add the key to the chain that's why it does not return the accounts, the getAccounts method then will return the accounts after signIn is successful. As to how the LAK is exposed this can be done since everything happens on the client side and extensions can write/store the LAK in the localStorage in the dApp.

Is the reason current injected wallets do this so that the user selects the account on the wallet instead of the dApp?

The Injected Wallets are not fully aligned with the Standard currently users will need to choose the account during signIn on the wallet and the wallet creates and adds the key to the chain, which is fine in a way as long as they save the LAK in the localStorage. But this approach is not how the standard states.

In the existing Injected Wallets the signIn somehow works in a similar way as for Browser Wallets since the more popular ones were implemented before the standard was created and they never got the chance to fully align with the Standard.

There can be a few routes we can take here:

• Allow this change so wallets can continue to keep signIn as they have now and implement signInMulti in a similar way.
• Discard this PR and ask or wait for wallets to align with the current Standard.

[esaminu]

Thanks for the explanation @kujtimprenkuSQA . IMO the NEP should allow for both implementations and I think this can be done by simply indicating in the NEP document that if account is not specified then the LAKs and accountId will be set in localStorage and specify the format or indicate that it will set them directly in BrowserLocalStorageKeyStore from NAJ. TBC there will be two modes of operation for signIn and signInMulti:

1. account is not passed in and the injected wallet sets the accountId and newly created LAK to localStorage via BrowserLocalStorageKeyStore or newly added keystore for signInMulti
2. account is passed in and the injected wallet adds the publicKey from account on chain and nothing is updated in localStorage

Both methods return Promise<void> whether account is passed in or not.

[kujtimprenkuSQA]

@esaminu Thank you for the comment. I have added some explanation in the document about this change, and if there are no other suggestions, I think this should be ready for final voting.

It would be good if someone can take a look at the PR for signInMulti in wallet-selector: near/wallet-selector#811 just so we're all on the same page.

[andy-haynes]

I have some concerns about the motivations behind this change. In particular I'd like to better understand the problems faced by wallet developers in conforming to the standard.

In the existing Injected Wallets the signIn somehow works in a similar way as for Browser Wallets since the more popular ones were implemented before the standard was created and they never got the chance to fully align with the Standard.

Relaxing the standard to meet existing implementations strikes me as a myopic route. It dilutes the intentions behind the standard, which would necessarily have been written with the understanding that current implementations did not adhere to it.

Allow this change so wallets can continue to keep signIn as they have now and implement signInMulti in a similar way.

What was the original rationale for requiring the account ID? Has anything changed that necessitates/motivates relaxing this constraint?

Discard this PR and ask or wait for wallets to align with the current Standard.

Is this a burdensome change for wallet developers to make? If there's friction with implementing the standard then it would be better to address that directly rather than doubling down on deviation from the standard. Adjusting the standard to fit the implementation because wallet developers have not yet adopted it really undercuts the purpose of having defined the standard this way in the first place.

TBC there will be two modes of operation for signIn and signInMulti

To me this really stresses that the behavior is fundamentally different between signing in with an account vs not specifying one at all. If that's the case then wouldn't it make more sense for there to be an entirely separate method for signing in without an account? e.g. something like signInWallet / signInWalletMulti or signInDApp / signInDAppMulti. By specifying a new method that makes a distinction between these behaviors, wallet developers have a path to conforming to the standard. Making this distinction very clear also provides a simpler, more granular interface for developers.

[kujtimprenkuSQA]

@andy-haynes

I have some concerns about the motivations behind this change. In particular I'd like to better understand the problems faced by wallet developers in conforming to the standard.

The motivation for this change was to make the standard less strict since none of the wallets have implemented even signIn as it's stated on the standard until now.

Relaxing the standard to meet existing implementations strikes me as a myopic route. It dilutes the intentions behind the standard, which would necessarily have been written with the understanding that current implementations did not adhere to it.

I have to agree with this point, this change would not be needed if wallets would have been aligned with the standard.

What was the original rationale for requiring the account ID? Has anything changed that necessitates/motivates relaxing this constraint?

Initially, when the wallet-selector team was working on creating the standard for Injected Wallets we had many discussions, and the idea for requiring account ID during signIn was mostly to shift the responsibility of creating the keyPair in the dApp side, this would ensure that the wallet does not hold the LAK inside the wallet as it happens for some wallets.
We have this kind of flow implemented for Bridge Wallets in wallet-selector in the wallet-connect package.
dApp Example: https://near.github.io/wallet-selector/
Wallet example: https://react-wallet.walletconnect.com/

Since the existing wallets have not aligned the signIn to accept an accountId during sign-in we thought that relaxing the standard would make sense to keep the implementation of signIn and signInMulti similar.

Is this a burdensome change for wallet developers to make? If there's friction with implementing the standard then it would be better to address that directly rather than doubling down on deviation from the standard. Adjusting the standard to fit the implementation because wallet developers have not yet adopted it really undercuts the purpose of having defined the standard this way in the first place.

I agree, this change would not be needed if wallets are pushed to align with the standard.

[victorchimakanu]

Hello @esaminu @MaximusHaximus , Id like to confirm the status of this NEP and your thoughts on progressing this NEP to the voting stage

[flmel]

As the moderator, I assume this PR is no longer active given that there was no activity for very long time. Therefore, I am closing it. If anyone is interested in reopening this PR, please submit a new one.