-
Notifications
You must be signed in to change notification settings - Fork 5k
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
refactor: Refactor extension storage classes #24016
Open
brad-decker
wants to merge
16
commits into
main
Choose a base branch
from
feat/refactor-state-classes
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+817
−301
Open
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
6fa5a22
refactor storage system
brad-decker 44bdfbb
Capture sentry error if there is an empty/corrupt vault, and handle m…
danjm 1c805ca
Ensure log.debug call in case fetch in init in ReadOnlyNetworkStore h…
danjm b8f75d6
Ensure that in ReadOnlyNetworkStore waits for initializing to complete
danjm ce2079c
Lint fix
danjm 7a3373d
Ensure that first time state is returned if state has no data
danjm a5d0eec
Update app/scripts/lib/Stores/ExtensionStore.ts
danjm 8048bd6
Update test description
danjm eb46312
Use const for DEFAULT_INITIAL_STATE in ExtensionStore.test.ts
danjm e1c3f7d
Improve naming and usage of versionedData variable in background.js
danjm 82d2650
lint fix
danjm 0bbbb9e
Capture exception with sentry in both error cases ExtensionStore.get
danjm f0c377f
Lint fix
danjm 1ca2982
Add app/scripts/lib/Stores/ReadOnlyNetworkStore.test.ts
danjm 16df6c5
Delete files that are no longer used
danjm e2d023f
Use async/await for get/set in ExtensionStore.ts
danjm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,167 @@ | ||
import type Migrator from '../migrator'; | ||
import firstTimeState from '../../first-time-state'; | ||
import { generateWalletState } from '../../fixtures/generate-wallet-state'; | ||
|
||
/** | ||
* This type is a temporary type that is used to represent the state tree of | ||
* MetaMask. This type is used in the BaseStore class and its extending classes | ||
* and should ultimately be replaced by the fully typed State Tree once that is | ||
* available for consumption. We should likely optimize the state tree by | ||
* storing the individual controllers in their own keys in the state tree. This | ||
* would allow for partial updates at the controller state level, without | ||
* modifying the entire data key. | ||
*/ | ||
export type IntermediaryStateType = Record<string, unknown>; | ||
|
||
/** | ||
* This type represents the 'meta' key on the state object. This key is used to | ||
* store the current version of the state tree as set in the various migrations | ||
* ran by the migrator. This key is used to determine if the state tree should | ||
* be updated when the extension is loaded, by comparing the version to the | ||
* target versions of the migrations. | ||
*/ | ||
export type MetaData = { version: number }; | ||
|
||
/** | ||
* This type represents the structure of the storage object that is saved in | ||
* extension storage. This object has two keys, 'data' and 'meta'. The 'data' | ||
* key is the entire state tree of MetaMask and the meta key contains an object | ||
* with a single key 'version' that is the current version of the state tree. | ||
*/ | ||
export type MetaMaskStorageStructure = { | ||
data?: IntermediaryStateType; | ||
meta?: MetaData; | ||
}; | ||
|
||
/** | ||
* When loading state from storage, if the state is not available, then the | ||
* extension storage api, at least in the case of chrome, returns an empty | ||
* object. This type represents that empty object to be used in error handling | ||
* and state initialization. | ||
*/ | ||
export type EmptyState = Omit<MetaMaskStorageStructure, 'data' | 'meta'>; | ||
|
||
/** | ||
* The BaseStore class is an Abstract Class meant to be extended by other classes | ||
* that implement the methods and properties marked as abstract. There are a | ||
* few properties and methods that are not abstract and are implemented here to | ||
* be consumed by the extending classes. At the time of writing this class | ||
* there are only two extending classes: ReadOnlyNetworkStore and | ||
* ExtensionStore. Both of these extending classes are the result of | ||
* refactoring the previous storage implementation to TypeScript while | ||
* consolidating some logic related to storage that was external to the | ||
* implementation of those storage systems. ReadOnlyNetworkStore is a class | ||
* that is used while in an End To End or other Test environment where the full | ||
* chrome storage API may not be available. ExtensionStore is the class that is | ||
* used when the full chrome storage API is available. While Chrome is the | ||
* target of this documentation, Firefox also has a mostly identical storage | ||
* API that is used interchangeably. | ||
* | ||
* The classes that extend this system take on the responsibilities listed here | ||
* 1. Retrieve the current state from the underlying storage system. If that | ||
* state is unavailable, then the storage system should return a default state | ||
* in the case that this is the first time the extension has been installed. If | ||
* the state is not available due to some form of possible corruption, using | ||
* the best methods available to detect such things, then a backup of the vault | ||
* should be inserted into a state tree that otherwise resembles a first time | ||
* installation. If the backup of the vault is unavailable, then a default | ||
* state tree should be used. In any case we should provide clear and concise | ||
* communication to the user about what happened and their best recourse for | ||
* handling the situation if the extension cannot gracefully recover. | ||
* | ||
* 2. Set the current state to the underlying storage system. This should be | ||
* implemented in such a way that the current metadata is stored in a separate | ||
* key that is tracked by the storage system. This metadata should *not* be a | ||
* input to the set method. If the underlying storage system allows for partial | ||
* state objects it should be sufficient to pass the data key, which is the | ||
* full MetaMask state tree. If not, then the metadata should be supplied by | ||
* the storage system itself. | ||
* | ||
* 3. Provide a method for generating a first time state tree. This method is | ||
* implemented as a part of this Abstract class and should not be overwritten | ||
* unless future work requires specific implementations for different storage | ||
* systems. This method should return a state tree that is the default state | ||
* tree for a new install. | ||
*/ | ||
export abstract class BaseStore { | ||
/** | ||
* isSupported is a boolean that is set to true if the underlying storage | ||
* system is supported by the current browser and implementation. | ||
*/ | ||
abstract isSupported: boolean; | ||
|
||
/** | ||
* dataPersistenceFailing is a boolean that is set to true if the storage | ||
* system attempts to write state and the write operation fails. This is only | ||
* used as a way of deduplicating error reports sent to sentry as it is | ||
* likely that multiple writes will fail concurrently. | ||
*/ | ||
abstract dataPersistenceFailing: boolean; | ||
|
||
/** | ||
* mostRecentRetrievedState is a property that holds the most recent state | ||
* successfully retrieved from memory. Due to the nature of async read | ||
* operations it is beneficial to have a near real-time snapshot of the state | ||
* for sending data to sentry as well as other developer tooling. | ||
*/ | ||
abstract mostRecentRetrievedState: MetaMaskStorageStructure | null; | ||
|
||
/** | ||
* metadata is a property that holds the current metadata object. This object | ||
* includes a single key which is 'version' and contains the current version | ||
* number of the state tree. This is only incremented via the migrator and in | ||
* a well functioning (typical) install should match the latest migration's | ||
* version number. | ||
*/ | ||
#metadata?: { version: number }; | ||
|
||
/** | ||
* migrator is a property that holds the migrator instance that is used to | ||
* migrate state from one shape to another. This migrator is used to create | ||
* the first time state tree. | ||
*/ | ||
abstract migrator: Migrator; | ||
|
||
/** | ||
* Sets the current metadata. The set method that is implemented in storage | ||
* classes only requires an object that is set on the 'data' key. The | ||
* metadata key of this class is set on the 'meta' key of the underlying | ||
* storage implementation (e.g. chrome.storage.local). | ||
*/ | ||
set metadata(metadata: { version: number }) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should consider how to ensure:
|
||
this.#metadata = metadata; | ||
} | ||
|
||
/** | ||
* Gets the current metadata object and returns it. The underlying key is | ||
* private and implemented in the BaseStore class so that the extending class | ||
* can access it through this getter. | ||
*/ | ||
get metadata(): { version: number } | undefined { | ||
return this.#metadata; | ||
} | ||
|
||
/** | ||
* Generates the first time state tree. This method is used to generate the | ||
* MetaMask state tree in its initial shape using the migrator. | ||
* | ||
* @returns state - The first time state tree generated by the migrator | ||
*/ | ||
async generateFirstTimeState() { | ||
let _firstTimeState = { ...firstTimeState }; | ||
if (process.env.WITH_STATE) { | ||
const stateOverrides = await generateWalletState(); | ||
_firstTimeState = { ..._firstTimeState, ...stateOverrides }; | ||
} | ||
|
||
return this.migrator.generateInitialState( | ||
_firstTimeState, | ||
) as Required<MetaMaskStorageStructure>; | ||
} | ||
|
||
abstract set(state: IntermediaryStateType): Promise<void>; | ||
|
||
abstract get(): Promise<MetaMaskStorageStructure>; | ||
|
||
abstract isFirstTimeInstall(): Promise<boolean>; | ||
} |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this check for
!versionedData.data
, or an alternative to it, present in the newExtensionStore.ts
? I can't find it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed by 70e6191