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

Enable cloud backup of vault & preferences #1459

Closed
danfinlay opened this issue May 19, 2017 · 26 comments
Closed

Enable cloud backup of vault & preferences #1459

danfinlay opened this issue May 19, 2017 · 26 comments
Assignees
Labels

Comments

@danfinlay
Copy link
Contributor

Wallet nicknames, currency preferences, all that stuff.

Either by file, or by cloud service provider, would be nice to let people sync their metamask across computers.

@nrhirsch
Copy link

I use two machines. I export the bids that I do on one machine and save the json files from ENS website into a dropbox location. I can later import the transactions into the second machine and continue on and then save the new json file again to dropbox. On metamask however, each machine has the separate transactions but no import/export function. It would be nice to be able to backup what is on one metamask machine (home) to a file so that I can import it on the second machine (work) and see all the transactions that were created on both machines. like the export/import function in ENS. Loading a file should be smart so it doesn't replace, it, just adds the new transactions from the file like ENS does.

@2-am-zzz 2-am-zzz added type-enhancement P3-soon area-background Issues relating to the extension background process. needs-design Needs design support. type-discussion and removed needs-design Needs design support. labels May 22, 2017
@2-am-zzz 2-am-zzz added this to the Core Maintenance milestone May 22, 2017
@step21
Copy link

step21 commented Dec 11, 2017

I agree, just thought about this and wanted to see if there were plans. Files are good for key backups (and this exists) but for everything else it is too cumbersome I think. Easiest as long as it is not too much data is browser sync I think. Reasonably secure for preferences and similar. See also: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/storage/sync

@owocki
Copy link

owocki commented Dec 21, 2017

i'd ❤️ to see this done soon. posting a bounty on this now

@gitcoinbot
Copy link

This issue now has a funding of 0.34 ETH (270.95 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
  • $12426.96 more Funded OSS Work Available at: https://gitcoin.co/explorer

@gitcoinbot
Copy link

The funding of 0.34 ETH (281.04 USD) attached has been claimed by @heyellieday.

@heyellieday, please leave a comment to let the funder (@owocki) and the other parties involved your implementation plan. If you don't leave a comment, the funder may expire your claim at their discretion.

@heyellieday
Copy link
Contributor

heyellieday commented Dec 21, 2017

Hi @owocki and interested parties! I recently discovered Gitcoin and I'm excited to use this bounty as an opportunity to contribute to a Metamask feature :) I'll start by outlining my implementation plan to enable cross browser syncing of MetaMask preferences and config.

The implementation consists of two main parts:

  1. Syncing to and from a supported browser's storage service through [storage.StorageArea], which is supported in all browsers that MetaMask supports, except Opera. (https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/storage/sync) (as suggested by @step21)
  2. Managing this feature via a settings section in the metamask-extension/ui application

While I will need clarification on the exact slices of store that should be synced, I have a good idea of how the syncing will take place, so here's on outline:

As some of you may be aware, MetaMask currently syncs via local storage as its main form of persistence. While the specific details are less important, it is useful to this implementation that updates to the versioned localStorage store occur via a stream that is at the end of set of chained streams, updatable as the application's main store changes. This chained stream is set up via a library called pump in app/scripts/background.js on line 76 and allows any number of stream transformation functions to be added to the chain. The start and the end of the chain are obversable objects acting as streams, enabled via obs-store.

Given the above information and the desire to keep things simple, I plan on evoking storage.StorageArea.set, with the desired store values, from within a chained stream transformation. To me, this is straightforward and does not require unneeded abstractions. However, there is an existing pattern to interface with external stores (currently localStorage) via something like obs-store. If it is important to the maintainers of the repository that such an interface be made, I can make an effort to do so, but again, I don't think it is needed at this time.

Then, in order to receive updates from the browser's StorageArea, the existing loadStateFromPersistence method in background.js would need to be modifying to read from the StorageArea, checking and then merging the persisted store if present. Since the usage of StorageArea would require the merging of external and local states, I think it is important to decide on a merging priority for each source. For reference, localStorage is currently loaded like so:

let versionedData = diskStore.getState() || migrator.generateInitialState(firstTimeState)

Luckily, once that is decided, loading data from the StorageArea is quite simple: storage.StorageArea.get. In addition to merging data on startup, it will be necessary to refresh the data from StorageArea at a recurring interval.

Lastly, it seems like it would be useful to be able to toggle syncing on and off via a settings item, updatable in the same way an existing config value would be set. The syncing code would then check if syncing is enabled before executing. I'm guessing this setting would be disabled by default.

My only potential concerns are summed up nicely from the MDN doc page:

The main use case of this API is to store preferences about
your extension and allow the user to sync them to different
profiles. You can store up to 100KB of data using this API. If
you try to store more than this, the call will fail with an error
message. The API is provided without any guarantees about
uptime or performance.

Lastly, to summarize needed points of clarification I've come across so far:

  • Is the lack of Opera support ok?
  • What parts of store should be synced across browsers? All of it? Just parts? Specific keys in the object will be needed, if not all of them.
  • What should the merging priority be for external vs local stores?
  • What is a good update interval?
  • Should browser sync be disabled by default?

Those are the main implementation details I want to start with, but can definitely go into more detail if needed.

I look forward to implementing this feature and can respond to questions, suggestions, or comments. Thanks!

@owocki
Copy link

owocki commented Dec 21, 2017

welcome @heyellieday :) good to work with you. ill leave the questions to @jrmoreau & co

@danfinlay
Copy link
Contributor Author

Hi @heyellieday, great thorough analysis, you've cut right to the meat of how we do our persistence!

Lots to unpack there, so to start:

it will be necessary to refresh the data from StorageArea at a recurring interval.

I'm curious how this will behave with two metamask browsers open at once, synced together. If one sends a transaction, will that new transaction be propagated to the other browser, or would it potentially be a race for which one pushes an update right before the other one pulls down from the storageArea? I just want to make sure this feature really makes sense, as a good experience for users.

Is the lack of Opera support ok?

I think it would be better to fall back to localStorage for browsers that don't support this API. We do actually publish to the Opera store, just haven't given it the love it deserves.

What parts of store should be synced across browsers? All of it? Just parts? Specific keys in the object will be needed, if not all of them.

This is a great question. Reducing synced scope could simplify that earlier concern. Maybe we don't need to cloud sync everything. The most obvious portions to include (to me) would be:

  • PreferencesController
  • KeyringController
  • TransactionController (maybe, but as the most-changing portion, could be the buggiest)

What should the merging priority be for external vs local stores?

People will have local stores to start, but if they have a cloud store, we should probably assume that's the newer one, and move to it.

What is a good update interval?

Do you mean for pulling down, or pushing up? Either way, let's keep it a constant at first, maybe at like 5 minutes or so, just keep it easy to change.

Should browser sync be disabled by default?

This is probably the most subjective question, I really don't know how the average person will feel about this. I think I'd keep it on personally, but I treat MetaMask as a very hot wallet, and don't use it for large sums. Some people use it recklessly, and would probably be safer with less syncing. Off by default to start is probably the safest choice, but a good thing to consider going forward.

@nrhirsch
Copy link

For simplicity, if you could just export the transactions on one machine to a file and import that file on another machine, that would do it for me. Similar to backing up a json file in ens and importing.

@heyellieday
Copy link
Contributor

@danfinlay, thanks for your answers! I'll reply with some additional comments on the main points.

Sync Interval: From my understanding, if we tie into the existing store stream, changes from an active client will be pushed to the browser's StorageArea in real time. The browser would then propagate those changes to other browsers at some point in the future. Pulling in data from the StorageArea is more manual. It would happen on initial load, but afterwards, we'd have to reload the data at a set interval. There is a likely a way to listen to changes to the StorageArea as they come in.

There also seems to be no downside to syncing to the browser's StorageArea frequently in the same way that syncing to localStorage on every change works fine. Behind the scenes, the browser may batch propagate changes, but at least we know we've captured all of the changes. I could see someone changing a setting and then quickly closing down a browser and a set interval of 5 minutes, for example, may not capture those changes.

Browser Support: I agree that for browsers that don't support this API, falling back to localStorage only is a good option.

What to sync: I can make it easy to change what slices we sync and then I'm thinking as we test out the dev build, we can see what makes the most sense to sync. I agree that transaction history will be harder to sync as it's more of a merge of data than a replacement from another source, which brings me to my next point.

Merging multiple sources: I think adding an updatedAt key to the data in the synced StorageArea could help determine the freshness of what is in the StorageArea. I'll have to see how it works in practice, but conceptually, the StorageArea should be a common store that multiple clients are updating or reading from, with the caveat that there is likely a delay in syncing. This tells me that the browser likely has its own merging process that could help us handle conflicting updates. Overall, how data is merged together will be something to continue to tinker with.

One more question for now: what is the best way to test in-development code on a published extension? This seems like it will be needed to test the syncing. I was able to add an unpacked extension in one browser, but it failed to run it in another chrome instance. The Publishing doc explains the process for an official release, but I don't see a mention of a dev build. If it wasn't needed until this time, maybe we need to add one?

@nrhirsch, maybe file-based exporting/importing could be requested in a different issue? This feature seems to be going the route of using the browser's sync API.

@owocki
Copy link

owocki commented Dec 27, 2017

looking forward to seeing what comes of this WIP PR :) @heyellieday

@danfinlay
Copy link
Contributor Author

Hi @heyellieday sorry for the slow responses, most of our team is taking a much-needed break for the holidays.

what is the best way to test in-development code on a published extension?

That's a great question, I genuinely don't know, we've never had a feature quite like this to deal with. It might be a thing where we want to add some simple counter test running in the background that we can push to production first, to check if it's working the way we want. Maybe something where you can access it from the console, to write to that shared storage space.

https://github.com/MetaMask/faq/blob/master/LOGS.md#background-logs-chrome

Or maybe you can just open the background console and write to the shared storage space anyway as an experiment? Very weird development issue...

@heyellieday
Copy link
Contributor

heyellieday commented Dec 30, 2017

@danfinlay, no worries! I've been enjoying some time with family the past week as well :)

Thanks for the reply about testing. I made some good progress last week in a WIP PR (#2802). The syncing, at least in one browser, works well! I am having issues with getting the test suite to run properly though.

I plan on spending some time this weekend working on it more, so if you have any ideas on why the test suite crashes due to an unexpected syntax error warning, I'd love to hear them! We can also move this discussion over to that thread if you want.

Update on failing test suite: I was running an outdated version of node that didn't support async/await 😅

@owocki
Copy link

owocki commented Jan 2, 2018

👋 happy new year folks! @danfinlay @heyellieday looking forward rto seeing the progress on this :)

@kumavis
Copy link
Member

kumavis commented Jan 2, 2018

what is the best way to test in-development code on a published extension?

we have an unlisted UAT extension that we could use for testing this feature

@heyellieday
Copy link
Contributor

happy new year @owocki! I've just updated the PR linked above with some further progress and questions for the maintainers, if you want to check it out.

Also, I don't know if this is the right place to ask this, but I noticed that the bounty you posted will expire tomorrow (Jan 3rd). Is there any way to extend it? Perhaps it is taking longer than anticipated, but due to the holidays and the number of clarifications needed, I think good progress is being made :)

@vs77bb
Copy link

vs77bb commented Jan 9, 2018

@owocki Bumping @heyellieday's request to extend the bounty. Was this completed? Ellie, I'm Vivek (new member of the Gitcoin team) - if you have any updates / questions for us, just let us know. Happy new year everyone :)

@owocki
Copy link

owocki commented Jan 10, 2018

i'm just a lowly bounty payout person. @kumavis and @danfinlay can review the work here :)

@vs77bb
Copy link

vs77bb commented Jan 17, 2018

A bump for the very busy @kumavis and @danfinlay 🙂

@bdresser
Copy link
Contributor

Relates to #5030 and draft PR #6438

@bdresser
Copy link
Contributor

Could use 3Box to automatically backup user preferences & contacts (with an option to disable)

No design needed - would happen entirely in the background.

@bdresser bdresser added this to the UI Sprint 13 [May 27] milestone May 23, 2019
@danfinlay
Copy link
Contributor Author

3box reference draft PR: #6438

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.