Use storage.local instead of localStorage by danfinlay · Pull Request #3083 · MetaMask/metamask-extension

danfinlay · 2018-01-24T19:02:17Z

Fixes #3076, needs thorough QA:

Create a vault on master, rebuild on this branch, see if MM continues seamlessly.
Try destroying the old localStorage data manually, see if MM continues seamlessly.
Do full QA on this branch, to ensure it's internally reliable.
Maybe try filling up storage manually somehow. Could be by creating tons of accounts or tons of testnet transactions that publish large contracts.

…oller

…f firefox

… on it.

…anjm-1 Additions to the UseStorageLocalInstead branch

kumavis · 2018-01-24T19:09:16Z

app/scripts/background.js

+      try {
+        localStore.set(state)
+      } catch (err) {
+        log.error('error setting state in local store:', err)


i dont think these get submitted to sentry (but maybe they do)

What does get submitted for sure?

(we should document our correct logging practice)

im not certain atm, but you can test it with the Test-Metamask sentry project on dev

kumavis · 2018-01-24T19:10:44Z

app/scripts/lib/extension-store.js

+const KEYS_TO_SYNC = ['KeyringController', 'PreferencesController']
+const FIREFOX_SYNC_DISABLED_MESSAGE = 'Please set webextensions.storage.sync.enabled to true in about:config'
+
+const handleDisabledSyncAndResolve = (resolve, toResolve) => {


meaning of toResolve is not clear

This is part of the original Ellieday PR, we aren't actually using this file in this PR, so I'm not sure I want to bother cleaning it up, I'd rather either:

Delete the file and its test

Ignore it for now

kumavis

Nice -- looking forward to it

added some questions / comments

kumavis · 2018-01-24T19:11:29Z

app/scripts/lib/extension-store.js

+  }
+  async fetch() {
+    return new Promise((resolve) => {
+      extension.storage && extension.storage.sync.get(KEYS_TO_SYNC, (data) => {


we wanted to drop KEYS_TO_SYNC, right?

kumavis · 2018-01-24T19:16:32Z

app/scripts/background.js

+      log.error('error fetching state from local store:', err)
+    }
+
+   // TODO: handle possible exceptions (https://developer.chrome.com/apps/runtime#property-lastError)


looks like this is somewhat handled in ExtensionStore

const lastError = extension.runtime.lastError

I'm hesitant to merge a TODO in here, especially around error handling

kumavis · 2018-01-24T19:17:56Z

app/scripts/lib/local-store.js

+      log.error('Storage local API not available.')
+    }
+  }
+  async get() {


doesnt need to be marked as async when manually returning a promise but maybe this is a good practice anyways

tmashuang · 2018-01-24T19:35:12Z

app/manifest.json

    "http://localhost:8545/",
    "https://*.infura.io/"
    ],
+  "unlimitedStorage": true,


unlimitedStorage should be added to the permissions, the same as storage. Referenced: Declare Permissions

kumavis · 2018-01-24T19:46:53Z

@heyellieday we used some of your code in this PR so we'd like to credit you with partial bounty reward of the original bounty (synced storage). Can you give an ethereum address here or emailed to aaron at kumavis dot me thanks/

kumavis · 2018-01-24T20:34:50Z

cool, just needs QA then

danfinlay · 2018-01-25T21:59:47Z

Added to that branch:

No longer writes to localStorage.
Fixed issue where quantity of write attempts overwhelmed our pump to storage.local stream.

heyellieday · 2018-01-30T04:12:44Z

@kumavis, sorry, I missed these mentions!

I'm glad some of my work was able to be used for this issue and thanks for crediting me part of the bounty! Here's an ethereum address to send to: 0x1ab4C79D96a05b64742AE1B0213B84fB984917C8.

I'm hoping I can help out with more in the future :)

- Don't persist undefined data - Write to new storage strategy without waiting for completion. - Continue writing to localStorage as fallback.

kumavis · 2018-03-07T22:30:43Z

app/scripts/background.js

+    }
+
+    if (Object.keys(localData).length > 0) {
+      versionedData = localData


why do we do this
please add a comment

Comment added.

kumavis · 2018-03-07T22:32:19Z

app/scripts/background.js

  const migrator = new Migrator({ migrations })
  // read from disk
-  let versionedData = diskStore.getState() || migrator.generateInitialState(firstTimeState)
+  versionedData = diskStore.getState() || migrator.generateInitialState(firstTimeState)


maybe something like
versionedData = localData || diskStore.getState() || migrator.generateInitialState(firstTimeState)

New commit moves storage.local loading before this assignment so I can use a similar format to what you suggest.

kumavis

im going to take another pass over this, please wait

…i3076-UseStorageLocalInstead

made the cleanups i wanted

…i3076-UseStorageLocalInstead

danfinlay · 2018-03-21T23:56:27Z

Yes, they are migrated to the new storage model, and the old storage model is kept around, in case the new one fails.

The disk store has not been written to since MetaMask v4.3.0, as it was removed in MetaMask#3083. It was kept around so that anything written to disk prior to v4.3.0 could still be restored. It has been a year and a half since that release, so I think it's time to remove the disk store altogether. The consequences of losing locally stored data are small anyway - it's an inconvenience at worst.

The disk store has not been written to since MetaMask v4.3.0, as it was removed in #3083. It was kept around so that anything written to disk prior to v4.3.0 could still be restored. It has been a year and a half since that release, so I think it's time to remove the disk store altogether. The consequences of losing locally stored data are small anyway - it's an inconvenience at worst.

heyellieday and others added 11 commits December 23, 2017 08:23

Add ExtensionStore and add basic store instance syncing to main contr…

a76324f

…oller

handle situation where storage.sync is disabled in certain versions o…

7184db7

…f firefox

conditionally use extension store if supported or enabled

3c6a5b1

Add unlimitedStorage option to manifest

03c64ba

Modify @heyellieday's work to use storage.local to replace main storage

456dfdb

Merge branch 'UseStorageLocal' into i3076-UseStorageLocalInstead

7c4a9c3

Handle errors when getting and setting to localStore.

0164030

Check that extension.storage exists before attempting to call methods…

b7ae77f

… on it.

Finish tests for extension-store fetch and sync.

598390e

Merge pull request #3079 from MetaMask/i3076-UseStorageLocalInstead-d…

84dfb19

…anjm-1 Additions to the UseStorageLocalInstead branch

Bump changelog

61cf5e9

kumavis reviewed Jan 24, 2018

View reviewed changes

tmashuang reviewed Jan 24, 2018

View reviewed changes

danfinlay added 2 commits January 24, 2018 11:36

Corrected unlimitedStorage permission

dd80bd4

Remove extension-store since we aren't using it yet

f09d72f

Remove already handled TODO comment.

b281a52

kumavis approved these changes Jan 24, 2018

View reviewed changes

danfinlay added 4 commits January 24, 2018 13:05

Remove redundant async modifiers

cd5eaa4

Fix retrieval of object

76521cf

Remove local storage writes, add log

2f13790

Debounce storage to avoid crashing pump

ceebc6c

Increase storage debounce value

8ba64c6

danfinlay added 2 commits March 6, 2018 14:59

Merge branch 'master' into i3076-UseStorageLocalInstead

423f084

Merge branch 'master' into i3076-UseStorageLocalInstead

d0ba2d2

danfinlay added 3 commits March 7, 2018 13:50

Increase storage debounce to 2s

ab55c18

Update yarn.lock

8965e59

Some fixes for new storage strategy

d58e47e

- Don't persist undefined data - Write to new storage strategy without waiting for completion. - Continue writing to localStorage as fallback.

kumavis reviewed Mar 7, 2018

View reviewed changes

danfinlay mentioned this pull request Mar 7, 2018

Metamask Chrome all settings acounts gone #3467

Closed

danfinlay added 2 commits March 7, 2018 14:38

Add clarifying comment

b97ac51

Tighten up data loading code

64fcd27

kumavis previously requested changes Mar 7, 2018

View reviewed changes

kumavis added 3 commits March 8, 2018 14:10

background - storage - cleanup storage wiring

98efca0

gitignore - add yarn.lock

1712ab3

Merge branch 'master' of github.com:MetaMask/metamask-extension into …

455faec

…i3076-UseStorageLocalInstead

kumavis and others added 4 commits March 8, 2018 14:55

lint fix

a88e436

Merge branch 'master' of github.com:MetaMask/metamask-extension into …

e48934d

…i3076-UseStorageLocalInstead

Update CHANGELOG.md

28a41c2

Merge branch 'master' into i3076-UseStorageLocalInstead

dc5fbf5

kumavis approved these changes Mar 13, 2018

View reviewed changes

kumavis merged commit 5fbfb0b into master Mar 13, 2018

kumavis deleted the i3076-UseStorageLocalInstead branch March 13, 2018 22:48

danfinlay restored the i3076-UseStorageLocalInstead branch March 14, 2018 17:10

This was referenced Mar 21, 2018

Investigate possible causes of a password suddenly not working #3643

Closed

Lost my wallet after reopen my computer normally #3191

Closed

Gudahtt mentioned this pull request Sep 16, 2019

Remove the disk store #7170

Merged

whymarrh deleted the i3076-UseStorageLocalInstead branch November 19, 2019 14:59

Conversation

danfinlay commented Jan 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumavis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumavis commented Jan 24, 2018

Uh oh!

kumavis commented Jan 24, 2018

Uh oh!

danfinlay commented Jan 25, 2018

Uh oh!

heyellieday commented Jan 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumavis left a comment

Choose a reason for hiding this comment

Uh oh!

danfinlay commented Mar 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants