Skip to content

Commit

Permalink
fix: Account for persistence flag when setting initial state in `Comp…
Browse files Browse the repository at this point in the history
…osableObservableStore` (#26280)

## **Description**

Fixes a bug where `ComposableObservableStore` would allow non-persistent
state properties in its `initialState`. This in turn would cause state
properties flagged as non-persistent to be persisted until a controller
state change.

This PR addresses this by taking the `persist` flag into account when
deriving the initial state for each controller. If the controller does
not support `metadata` or persistence is disabled, everything should
continue to work as-is.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26280?quickstart=1)
  • Loading branch information
FrederikBolding authored Aug 1, 2024
1 parent 3f09c80 commit fcf474c
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 6 deletions.
7 changes: 6 additions & 1 deletion app/scripts/lib/ComposableObservableStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,12 @@ export default class ComposableObservableStore extends ObservableStore {
);
}

initialState[key] = store.state ?? store.getState?.();
const initialStoreState = store.state ?? store.getState?.();

initialState[key] =
this.persist && config[key].metadata
? getPersistentState(initialStoreState, config[key].metadata)
: initialStoreState;
}
this.updateState(initialState);
}
Expand Down
42 changes: 37 additions & 5 deletions app/scripts/lib/ComposableObservableStore.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,12 @@ class OldExampleController extends BaseControllerV1 {
class ExampleController extends BaseController {
static defaultState = {
bar: 'bar',
baz: 'baz',
};

static metadata = {
bar: { persist: true, anonymous: true },
baz: { persist: false, anonymous: true },
};

constructor({ messenger }) {
Expand All @@ -41,8 +43,8 @@ class ExampleController extends BaseController {
}

updateBar(contents) {
this.update(() => {
return { bar: contents };
this.update((state) => {
state.bar = contents;
});
}
}
Expand Down Expand Up @@ -94,7 +96,9 @@ describe('ComposableObservableStore', () => {
const store = new ComposableObservableStore({ controllerMessenger });
store.updateStructure({ Example: exampleController });
exampleController.updateBar('state');
expect(store.getState()).toStrictEqual({ Example: { bar: 'state' } });
expect(store.getState()).toStrictEqual({
Example: { bar: 'state', baz: 'baz' },
});
});

it('should update structure with all three types of stores', () => {
Expand All @@ -114,7 +118,7 @@ describe('ComposableObservableStore', () => {
exampleController.updateBar('state');
oldExampleController.updateBaz('state');
expect(store.getState()).toStrictEqual({
Example: { bar: 'state' },
Example: { bar: 'state', baz: 'baz' },
OldExample: { baz: 'state' },
Store: 'state',
});
Expand All @@ -139,7 +143,7 @@ describe('ComposableObservableStore', () => {
});

expect(store.getState()).toStrictEqual({
Example: { bar: 'state' },
Example: { bar: 'state', baz: 'baz' },
OldExample: { baz: 'state' },
Store: 'state',
});
Expand All @@ -160,6 +164,34 @@ describe('ComposableObservableStore', () => {
});
});

it('should strip non-persisted state from initial state with all three types of stores', () => {
const controllerMessenger = new ControllerMessenger();
const exampleStore = new ObservableStore();
const exampleController = new ExampleController({
messenger: controllerMessenger,
});
const oldExampleController = new OldExampleController();
exampleStore.putState('state');
exampleController.updateBar('state');
oldExampleController.updateBaz('state');
const store = new ComposableObservableStore({
controllerMessenger,
persist: true,
});

store.updateStructure({
Example: exampleController,
OldExample: oldExampleController,
Store: exampleStore,
});

expect(store.getState()).toStrictEqual({
Example: { bar: 'state' },
OldExample: { baz: 'state' },
Store: 'state',
});
});

it('should return flattened state', () => {
const controllerMessenger = new ControllerMessenger();
const fooStore = new ObservableStore({ foo: 'foo' });
Expand Down

0 comments on commit fcf474c

Please sign in to comment.