No Private Keys Online #1466
Merged
No Private Keys Online #1466
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
While reviewing, I noticed that it could be difficult to differentiate the "Create Wallet" options between desktop and web. In order to make this more immediately clear to users, as well as provide users more guidance about recommended wallet options, I'd like to recommend that we add a color code to each wallet creation option, with no wallet option sharing a color with another. |
|
Added a splash of color, hope this differentiates enough without overpowering the view. Also added some color to the warning on generating a wallet:
|
This was referenced Apr 15, 2018
|
Is the text under MetaMask applicable to MetaMask only or all Web3? What text will appear if MetaMask is not installed, or if there are other web providers in the future re: #1700 Same question for the splash of color above "MetaMask." The orange is fitting for MetaMask but could clash visually if another Web3 provider is not orange-based. I recommend something more neutral, or have the color change according to the web3 provider and have them provide color info when making a PR to add themselves as a default. |
|
This is a suggestion for creating a new wallet, not for unlocking one, so it wouldn't be dynamic based on the provider. Also, if they're already using a different provider, they probably don't want a new wallet. |
|
For now it makes sense to only say MetaMask, but is there a way to make it dynamic? I think it would be beneficial to develop as such. The only real upcoming use case is with Blue's wallet but I imagine more and more will be implemented in the future and it would be beneficial (in the future) to not recommend one over another. |
|
Misunderstood on my first pass. We can add additional providers at a later point if we need, but at this point MetaMask has been a solid and secure option, so we would want to give time for new providers to earn their reputation. |
|
Updated with latest develop. Removed the deprecation warnings (Since only users with downloadable builds will see those now, they don't need the warning.) Also added an override button while in dev mode, in case you need to test insecure wallets and don't want to build / run |
wbobeirne
changed the title
dternyak
approved these changes
Jun 14, 2018
ConnorBryan
added a commit
that referenced
this pull request
Jun 16, 2018
dternyak
pushed a commit
that referenced
this pull request
Jun 18, 2018
* Add a new route for AddressBook * Further templating of the AddressBook view * Add initial functionality to handle a table of existing address labels * Make the linter happy * Adjust paths * Factor out TableRow and add common functionality * Add initial Redux boilerplate for addressBook | fix minor linting issues * Swap out terminology and types * Connect up to Redux * Connect data for AddressBookTable to Redux * Use temporary fields for addition * Remove alignment and index column * Stopping point * Adjust the sizing of rows to be consistent * Initial implementation of a dropdown for the address field * Minor styling to dropdown * Stopping point * Apply a focus concept onto the factory * Add keyboard controls for the address field dropdown * Adjust label of address field when it matches an addressBook entry * Properly handle attempting to blur a non-existent component * Minor styling changes on dropdown box * Standardize address casing, add accessibility to dropdown * Create an addressLabel component * Pass refs correctly and fix some typings * Exact version * Add module name mapping for shared/keycodes * addressBook reducer tests * Add functionality to DeterministicModal * Minor changes / Add test for addressBook selectors * Move out AddressBookTable to a component * Typing, translation and restructuring * More typing and translation fixes * More linting fixes * More type changes * Variable name for dropdown background * Fix TS type errors, lint errors, remove unused props * Used a different selector and removed method: AddressBookTable * Linter was mad * Linter mad again :( * Add a translation and adjust styling of AddressBookTable * Move the onBlur to a class method * Prevent the default behavior of up/down/enter for dropdown * Let's do it this way instead * Adjust the styling on DeterministicWalletModal labels * Change `AddressBookTable` into a pseudo-table using section and div * Use readable keys vs. keycodes * Put the dropdown in InputFactory and position it correctly * Sanitation of label adding and changing * Prevent duplicate labels in AddressBook and Row * Add a box shadow and use `invalid` class insted of custom * Use emphasis vs strong for address in dropdown * Display the label undernearth the input vs. changing it * Isolate AccountAddress into its own component * Introduce interactivity to AccountAddress * Fully incorporate with Redux * Validation for AccountAddress * Add validation notifications for address field on AddressBookTable * Minor formatting * Adjust wrappage for optimal flexboxxing * Make AddressBookTable responsive * Show an invalid input in "real time" instead of only on submit * Real time input validation for AddressBookTableRow * Responsive-ize the To address dropdown * Hide identicons instead at small enough screen sizes * Fix repsonsiveness of dropdown further * Fix responsiveness of table rows and inputs * Truncate account info and switch identicons to the right for consistency * Use classnames instead of targetting element directly for DWM * Display a notice if the entered query doesnt match a label and isnt an addr * Don't show an error on the To address if its a label entry * Display an error under AddressBookTableRow in real time * Display errors in real time for AddressBookTable temp inputs * Add realtime validation to AccountAddress * Ensure toChecksumAddress is used when entering labels to address manager * Show errors even after blurring. * Create a ducks/ implementation for addressBook * Duck-ize notifications * Duck-ize customTokens * Duck-ize deterministicWallets * Only show errors on address/label entry if they have been blurred * On certain inputs, show an invalid input immediately * spec files in same directory * Rename top-level redux directory * Duck-ize gas * Add displayed errors for labels with 0x and labels containing ens * Move ENS checking validation out * Add a saga for addLabelForAddress * Completely revamp the redux side of Address Manager and test it all * Adjust components to use new redux addressBook * Incorporate new redux into AddressBookTableRow and clean up for linter * Make linter and tests happy * Another reduxy overhaul * Still fixing it * More redux updates * Finalize redux stuff. * Incorporate new reduxy way into AddressBookTable & Row * Incorporate redux changes into Account Address * Small tests fix * Add and fix some selector tests * Addressing Will's comments * Shortened visibility class for line length reasons. * Incorporate ducks pattern on updates addressBook * Fix typeerror * Migrate messages to ducks * For Henry * Duckify onboardStatus * Duckify paritySigner * Duckify rates * Duckify transactions * Duckerize wallet * Reduckerate config * Adjust exports and tests of every duck so far * Duckify ENS * Duckerificate schedule * Duckificate swap * Actually use the new sagas; fix a circular dependency problem. * Duckify transaction (phew) * Add basics to redux/ directory * Remove non-ducked redux stuff * First sweep of redux/ directory * Combine redundant imports * Fix more linting stuff. * A few more type fixes * Welp... now I know not to use index. * Sweep components/ * Sweep through containers/ * Im really starting to get frustrated * The dawn of a new age. * Linter fixes. * De-flatten config/ reducers * Do my thang on config selectors * Adjust all references to config * Split up ens reducers * Wrap up de-nesting ENS * Big boy refactor * Split transaction into its reducers * Fix reducers in transaction/ * Stopping point * Adjust references to transaction from components * Fix references to selectors * Nest broadcast actions * Nest field actions * Nest meta actions * Nest network actions * Nest sign actions * Nest broadcast types * Nested fields types * Nest meta types * Nested network types * Nested sign types * Implement transaction saga changes * Huh? No prepush problems? * Update snappshot * Reintroduce deleted tests * A few missing tests found * Found three missing transaction tests * Found more tests * Found the rest of the tests, woohoo. * Renamed TypeKeys in TRANSACTION * Specify TRANSACTION_BROADCAST * Pretty up these imports * Specify TRANSACTION_FIELDS * Specify TRANSACTION_META * Specify TRANSACTION_NETWORK * Specify TRANSACTION_SIGN * Adjust imports and add translations * Update config snapshot * Post-merge * Temporary fix for DW/Config sagas so Daniel can continue smoke testing * Remove first circulat dependency * Fix more circular dependencies * Properly structure config indices * Further restructure config * Prepare for idea * Target directly from within features/ * Remove that circular dependency -- woohoo * Remove the circular dependency from Web3Wallet, temporarily comment some tests pending assistance * Un-comment the component-in-redux phenomenon * Move onLoad to the store file * Adjust addressBook imports/exports * Adjusted imports/exports for customTokens * Adjust imports/exports of deterministicWallets * Adjust imports/exports of ens * Restructure imports/exports of gas * Restructure imports/exports for message * Adjust imports/exports of notifications * Restructure onboardStatus imports/exports * Restructure paritySigner imports/exports * Restructure rates imports/exports * Restructure schedule imports/exports * Fix broadcastweb3handler test * Restructure swap imports/exports/ * Restructure transactionS imports/exports * Restructured wallet imports and exports * Hoist all necessary selectors aside from config/**/* and transaction/**/* * Hoist all top-level selectors from transaction * [Fix] Estimate Gas on Value Field Change (#1942) @ skubakdj * Implement right-click context menu (#1780) @ bryanwb * No Private Keys Online (#1466) @ wbobeirne * Fix Stuck Node on Metamask Logout (#1951) @ wbobeirne * [Fix] Make ENS Value Consistent (#1956) @ skubakdj * Auto token add (#1808) @ HenryNguyen5 * Electron Ledger + Trezor Support (#1836) @ wbobeirne * Fix Context Menu Popup Parameters (#1957) * Add RSK network w/ network agnostic refactors (#1939) @ wbobeirne * Change displayed notification back in helpers.tsx * Remove newline on shell files * Re-add newlines * Remove newling on .travis.yml * Prettier two files * Re-add index.scss import in OnboardModal * Restructure transaction subdirectories * Everything in transaction/ except for sagas * Restructure transaction imports/exports * Nest broadcast sagas * Nest fields * Nest meta * Nest network * Nest sign * Use generic names for reduxy stuff in the same directory to save space * Do everything every in the whole wide world
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
WHY?
Over the past few months, you've probably noticed warnings and notices when using wallet formats marked as unsafe.
Between hardware wallets like Ledger and TREZOR, applications like MetaMask and Parity Signer, and now the MyCrypto Desktop application, we feel that there are sufficient and low-friction alternatives to direct private key access.
Although our upcoming production release of MyCrypto Desktop will support Keystore, Mnemonic, and Private Key formats, we continue to suggest users switch to Hardware Wallets, MetaMask, and Parity Signer.
Closes #557
Description
It's finally here, no more private keys through the site. This both disables generating and unlocking private keys when accessing through the website (and, currently, in dev. I may add an escape hatch for that.)
When unlocking, users are presented the same view, but prompted to download when they select an insecure wallet.
When creating a new wallet, users are presented alternatives with hardware wallets, metamask, and the parity signer (#1349). They are also given the option to download the app. If they re-visit this screen on the app, the metamask option will be replaced with the option to create new wallets. They'll then see the old view with keystore file and mnemonic options.
Note that when testing, mycryptobuilds.com runs the downloadable HTML version of the site and therefore will allow you to use private keys, thinking it's running on your machine.
Changes
Steps to Test
No Unlocking
No Generating
Blocked Until the Following
^ To the above, I think that we should start posting the HTML downloadable versions of the site again. I think they're a great feature for people who don't want to grant us access to their computer via Electron, or want a portable build of the app to put on a USB drive. While I'm confident in the security of our Electron app, I understand people don't want to necessarily have something running natively on their machine. If we do add the HTML version back in, I'll probably want to update the download page to reflect that (Though still preferring Electron.)
Screenshots
Create New Wallet - Web
Create New Wallet - Running Locally
Insecure Wallet - Web
I'm not wild about the design here, I think some kind of illustration or some helpful links could go a long way to making this more friendly and less annoying to users.