danfinlay commented 6 years ago

Opening for discussion, to hone the approach and criteria for the goal of allowing arbitrary signers to be added to MetaMask.

Extending from #328, but adding a visual component. The related design issue is #731. This could be done in parallel with #731 as long as it was kept design-agnostic.

Blocking #717, #605, #716.

MetaMask Mutli-Signer Acceptance Criteria

An Aspect of MetaMask that has waited far too long is our ability to support multiple signing strategies. At time of writing, MetaMask only accepts two types of signer (both key pairs), the 12-word mnemonic, and the imported private key.

Going forward, MetaMask strives to allow users to sign blockchain transactions representing any of a multitude of strategies, including:

Hardware Wallets (Ledger, Trezor, Firefly)
Offline Signers (copy the params, paste the signed tx, or all QR codes?)
Contract Based Accounts (Multi Sig, uPort, 2FA, etc)
Remote/Hosted signers (Imagine if Coinbase signed as a service)

The Keyring Class

Our initial effort towards this end was defining The Keyring Class, a JavaScript protocol that objects can adhere to, and be added to MetaMask as new signing strategies.

What we never added to the Keyring protocol was a notion of custom signing views, which would definitely be necessary for things like offline signers or contract based accounts, since these might have options, preferences, and parameters that only relate to that view itself.

The Work To Go

This work would be composed of several sub-features, which should stem from our New UI work, which currently lives on the uat branch (hopefully to merge this week and deploy next).

A complete implementation of this platform would be to implement a single minimum-viable new signing strategy (like the Ledger, or an offline signer with custom UI). The exact signer would not be as important as the platform's easy extensibility.

The roadmap would roughly include:

The account list should group by Keyring, with headers.
The import account view should list a new signer type.
That new signer import screen should include whatever that signer requires to set up. (Opening in new tab is probably a good idea for a complex form).
When an account from that signer is selected, and a transaction is proposed, this signer would get to provide its own confirmation screen/view.

Confirmation View Considerations

The confirmation view itself is not perfectly defined. Currently our confirmation screen strives to provide all info that the user needs to approve a signature, and no other action is required, since MetaMask holds those keys.

For something like a hardware wallet, it is easy to imagine that a subsequent screen or series of steps could be provided after this confirmation screen, and this might be acceptable for only adding Ledger support, but other signers have other requirements!

Consider a multi-sig or contract account. The confirmation screen in this scenario would need to refer to either:

The transaction the user intends to send
The transaction to their contract account, which will then proxy that other transaction.

Since the former is the one that is intelliglbe to the user, we might prefer to show this by default, but since the latter would show the gas parameters that are actually relevant to the user, it is clear that the actual confirmation screen for a contract account would need to be some kind of fusion.

For this reason, custom confirmation screens and flows probably need to be (at least an optional) customization provided by new Keychains.

Signer Composability

The last major criteria, and some might argue whether it is truly a requirement, would be signer composability. This is the notion that some signers might require another signer to sign, and so MetaMask could empower these signers greatly by providing the user options of how to compose their signers.

For example, in the case of a multi-sig wallet, a user might add a contract account which will forward any transaction which includes a signature from two of its authorized keys. The user might hold one key on their phone, and one key on their computer.

This detail could be left to the custom signer, and as a minimum viable product, that might be what we agree to do. However, what if the user wants their local key to be a hardware wallet? In this case, the user wants to use a contract account, which might impose a spending limit, and they only want to keep their keys on a hardware device. I don't think we should expect each contract wallet keyring author to implement their own hardware wallet support, and so MetaMask could offer some powerful leverage for contract wallets by allowing signers to be arbitrarily composed.

In the most complicated case, this might mean a hardware wallet is authorized to sign fot a contract identity which is authorized to sign for a multi-sig account.

I know, this may be premature optimization, and we can definitely ship smaller pieces sooner, but I think keeping a consideration for this long-term path could help our architecture stay flexible going forward, because I think signer composability is eventually going to be an obviously desired feature; People will want to browse web3 sites on behalf of organizations that they are part of, and they will want to propose transactions to that organization, and they will propose those transactions from their uPort identities (contract accounts), which they will likely authorize from their phones or hardware devices.

Acceptable Formats

Ideally, Keyrings themselves can be defined within a specified bundle or module, paving the way for a "signer store", so that eventually we can further abstract the job of selecting signers away from the core codebase to whatever mechanism we agree is the most secure and usable.

Bounty Warnings

If you pursue a bounty on this issue, to be clear: While you pursue it, the issue is not reserved for you. We will pursue this issue as soon as possible, and the only way you will earn a bounty for this issue is if you both deliver this feature before we're able to, and it satisfies our very high standards, which we will enforce rigorously.

Pursue at your own risk!

danfinlay commented 6 years ago

Sandboxed execution

Especially if we were making a signer store, or trying to accept signing strategies more casually, isolating new signers, either through their UI in an iframe, or through their background process in a web-worker, would be an important set of considerations.

danfinlay commented 6 years ago

Bounty Details

We're considering posting a bounty on this, as a challenge to the very rambunctious and sometimes impatient crowd eager for these valuable features.

If you pursue a bounty on this issue, to be clear: While you pursue it, the issue is not reserved for you. We will pursue this issue as soon as possible, and the only way you will earn a bounty for this issue is if you both deliver this feature before we're able to, and it satisfies our very high standards, which we will enforce rigorously.

Pursue at your own risk!

gitcoinbot commented 6 years ago

This issue now has a funding of 1.1 ETH (1010.24 USD) attached to it.

If you would like to work on this issue you can claim it here.
If you've completed this issue and want to claim the bounty you can do so here
Questions? Get help on the Gitcoin Slack
$9560.46 more Funded OSS Work Available at: https://gitcoin.co/explorer

danfinlay commented 6 years ago

@lazaridiscom You're completely right, both about this feature being sub-project sized, and our team being very busy 😬.

I definitely think this bounty is currently insufficient, and I think we will gradually add more to it (and encourage others to add to it), but we also are budgeted to complete this internally (and we are hiring to help on it), so it's a weird issue for us to add bounties on.

If you're seriously interested in contributing more (it sounds like you'd be a great help), please reach out, maybe via email or something. A sub-project this large is hard to treat as a simple bounty, but with enough coordination, we could probably work something out.

vs77bb commented 6 years ago

@lazaridiscom Glad to see you taking this on! This is definitely an experiment so we'll be excited to hear feedback from both you and @danfinlay as things progress 🙂

danfinlay commented 6 years ago

@lazaridiscom Whoah, what's wrong here? The Gitcoin staff posted this bounty, not us, so this is very much their experiment as much as it is ours. What could possibly be wrong with him chiming in with support here?

I'll add here that @lazaridiscom and I have talked a bit via email, and I agreed that since this issue is underpriced, it may be fair to award it for sufficient documenting/planning with potential proposed architecture. It probably would be most fair to clearly define that task if it's going to be treated as the bounty subject.

danfinlay commented 6 years ago

I "fight" with gitcoin and its staff since days, mainly to reduce the side-noise within issues. I do this in the role of a "user" or "customer", so I possibly sound sometimes a bit itchy

As long as you're just trying to improve the product, and they understand the spirit, that's fine with me. It looked explosive without that context.

owocki commented 6 years ago

hey @lazaridiscom hit me up on slack when you get a sec tomorrow

vs77bb commented 6 years ago

Hi @omyreaon33 can you update with progress here? If not, Gitcoin Bot will likely open this one back up to the community. Hope you're having a good weekend!

gitcoinbot commented 6 years ago

@Omyreon33 are you still working on this issue?

gitcoinbot commented 6 years ago

@Omyreon33 are you still working on this issue?

gitcoinbot commented 6 years ago

@Omyreon33 are you still working on this issue?

gitcoinbot commented 6 years ago

@Omyreon33 are you still working on this issue?

[x] warning 1 (3 days)
[x] warning 2 (6 days)
[ ] auto removal (10 days)

vs77bb commented 6 years ago

This one has been opened back up on Gitcoin due to inactivity. Please take a look here if you're interested in claiming!

Apologies for the many comments from Gitcoin Bot all, we're cleaning this up. cc @owocki

gitcoinbot commented 6 years ago

@St-Matthew Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

[x] warning (3 days)
[ ] auto removal (6 days)

gitcoinbot commented 6 years ago

@St-Matthew Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

[x] warning (3 days)
[ ] auto removal (6 days)

gitcoinbot commented 6 years ago

@St-Matthew Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

[x] warning (3 days)
[ ] auto removal (6 days)

gitcoinbot commented 6 years ago

@St-Matthew Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

[x] warning (3 days)
[ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

gitcoinbot commented 6 years ago

@St-Matthew Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

[x] warning (3 days)
[ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

matthewarieta commented 6 years ago

I am not. This was selected by accident while testing the iOS app.

gitcoinbot commented 6 years ago

@rav8815 Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

[x] warning (3 days)
[ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

gitcoinbot commented 6 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 1 month, 2 weeks ago. Please review their questions below:

thjnhcolag has started work.

gitcoinbot commented 6 years ago

@thjnhcolag Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

[x] warning (3 days)
[ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

gitcoinbot commented 6 years ago

@thjnhcolag Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

[x] warning (3 days)
[ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

gitcoinbot commented 6 years ago

@thjnhcolag Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

[x] warning (3 days)
[ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

spm32 commented 6 years ago

Hi @thjnhcolag unfortunately we'll need to give this issue back to the crowd as we haven't heard any updates from you. If you think we've made a mistake please let us know!

spm32 commented 6 years ago

@lazaridiscom thanks for clarifying that

@danfinlay I see you're marked as the funder, would you be willing to close this bounty and reopen it on #4060? Much appreciated!

gitcoinbot commented 6 years ago

@thjnhcolag Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

[x] warning (3 days)
[ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

danfinlay commented 6 years ago

My opinion on this issue has returned to its original state: This is too large an issue to entrust to an external contributor or bounty hunter, and so the MetaMask team will be focusing on it internally, and removing the bounty from this issue.

The problem is mostly that tightly integrated refactors benefit from rapid communication, and we tend to have dramatically stronger/faster communication within the team than without. We'll continue to work towards improving our outwards-facing communication, but until all the required channels are in place, it doesn't make sense to encourage contributors like @ceresstation to pursue these kinds of tasks.

gitcoinbot commented 6 years ago

@thjnhcolag Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

[x] warning (3 days)
[ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

danfinlay commented 6 years ago

Closing as it has been replaced by more specific issues.

gitcoinbot commented 6 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

The funding of 1.1 ETH (513.11 USD @ $466.46/ETH) attached to this issue has been approved & issued.

Learn more at on the Gitcoin Issue Details page
Questions? Checkout Gitcoin Help or the Gitcoin Slack
$18,307.10 more funded OSS Work available on the Gitcoin Issue Explorer

gitcoinbot commented 6 years ago

(edit: comment removed and we put in a root fix to stop gitcoinbot from commenting on closed issues)

MetaMask / metamask-extension

Create visually customizable keyring format #3249

MetaMask Mutli-Signer Acceptance Criteria

The Keyring Class

The Work To Go

Confirmation View Considerations

Signer Composability

Acceptable Formats

Bounty Warnings

Sandboxed execution

Bounty Details