near / near-wallet

Web wallet for NEAR Protocol which stores keys in browser's localStorage
https://wallet.near.org
MIT License
220 stars 176 forks source link

Improve AddKey #1972

Open ilyar opened 3 years ago

ilyar commented 3 years ago

I think the page with the list of access keys (https://wallet.near.org/full-access-keys) is not informative enough. This may lead to additional questions or erroneous user actions.

image

If we add additional information when adding a key, this will make the user's experience more intuitive.

- export class AddKey extends IAction { publicKey: PublicKey; accessKey: AccessKey; }
+ export class AddKey extends IAction { publicKey: PublicKey; accessKey: AccessKey; reason?: string; expires?: Date}

image

Tell me what you think about this?

MaximusHaximus commented 3 years ago

I don't believe that expiration of access keys is supported by the protocol itself; are you envisioning this would be exclusively part of the wallet itself and not part of any protocol restrictions? If the former, I would be a bit concerned that it could be misleading for users if we show expired times next to keys that can still be used on-chain.

We can absolutely add metadata to the access key on creation, and we do in one case currently, which is when you use a Ledger device -- we add a type metadata property with a value of ledger in this one case :)

We are looking at enhancing access key management Very Soon:tm:. We're definitely on the same page that this needs to be a lot better.

Refs: https://github.com/near/near-wallet-roadmap/issues/21 https://github.com/near/near-wallet/issues/1710

We've got some concerns about putting too specific of metadata on the keys on-chain (which is public).

Who are you envisioning is responsible for defining the reason metadata being added to a newly added key? Are you imagining that we would prompt the user when adding a full access key for the 'reason', or that it would always be inferred by the code that is responsible for creating the key?

ilyar commented 3 years ago

@MaximusHaximus My proposal relates to changing the protocol, the wallet example is a special case, apparently that's why @bowenwang1996 moved my proposal here, it doesn't make sense to do it only at the wallet level.

stefanopepe commented 3 years ago

I remember discussing internally the matter a while back. The identified opportunities were mostly two:

Unfortunately, this would unnecessarily complicate the protocol (a master key can delete a full-access key -- but -- can a full-access key delete another full-access key?). Overall this logic wouldn't really simplify the life of the user, the problem would still be present in the user interface and the logic behind this powerful tool.

A familiar example is address re-use for Bitcoin UTXOs: technically you can receive tokens to an address that already spent inputs (was quite common with paper wallets, a while back) but applications do everything possible to avoid this wrong behavior.

Lastly, rather than just the wallet, I believe this is an effort including also APIs and developer tools, with the goal to simplify the approach at the application level. In any case, that link is disabled from the standard UI, you can only use it via deep-linking, and we suggest using it only if you really know what you are doing =)