-
Notifications
You must be signed in to change notification settings - Fork 363
Enhancement: cache owned Safes in localStorage #3066
Changes from 2 commits
4270e44
bbd438d
be525a2
ddb62b4
e5a4535
424f9b4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,9 +30,6 @@ enum ErrorCodes { | |
_701 = '701: Failed to save a localStorage item', | ||
_702 = '702: Failed to remove a localStorage item', | ||
_703 = '703: Error migrating localStorage', | ||
_704 = '704: Failed to load a sessionStorage item', | ||
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. Why have you removed these? The 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. I know but there's little value in having two sets of error messages depending on which storage this is. Besides, we're not even tracking them. I can change 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. Alright, I would suggest generalising 701-703 then. |
||
_705 = '705: Failed to save a sessionStorage item', | ||
_706 = '706: Failed to remove a sessionStorage item', | ||
_800 = '800: Safe creation tx failed', | ||
_801 = '801: Failed to send a tx with a spending limit', | ||
_802 = '802: Error submitting a transaction, safeAddress not found', | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
import { logError, Errors } from 'src/logic/exceptions/CodedException' | ||
|
||
type BrowserStorage = typeof localStorage | typeof sessionStorage | ||
|
||
const DEFAULT_PREFIX = 'SAFE__' | ||
|
||
class Storage { | ||
private prefix: string | ||
private storage: BrowserStorage | ||
|
||
constructor(storage: BrowserStorage, prefix = DEFAULT_PREFIX) { | ||
this.prefix = prefix | ||
this.storage = storage | ||
} | ||
|
||
private prefixKey = (key: string): string => { | ||
return `${this.prefix}${key}` | ||
} | ||
|
||
public getItem = <T>(key: string): T | undefined => { | ||
const fullKey = this.prefixKey(key) | ||
let saved: string | null = null | ||
try { | ||
saved = this.storage.getItem(fullKey) | ||
} catch (err) { | ||
logError(Errors._700, `key ${key} – ${err.message}`) | ||
} | ||
|
||
let data = undefined | ||
if (saved) { | ||
try { | ||
data = JSON.parse(saved) | ||
} catch (err) { | ||
logError(Errors._700, `key ${key} – ${err.message}`) | ||
data = undefined | ||
} | ||
} | ||
|
||
return data as unknown as T | undefined | ||
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. Why not just return on line 32/35? You won't need to cast to both types, only line 32 to |
||
} | ||
|
||
public setItem = <T>(key: string, item: T): void => { | ||
const fullKey = this.prefixKey(key) | ||
try { | ||
this.storage.setItem(fullKey, JSON.stringify(item)) | ||
} catch (err) { | ||
logError(Errors._701, `key ${key} – ${err.message}`) | ||
} | ||
} | ||
|
||
public removeItem = (key: string): void => { | ||
const fullKey = this.prefixKey(key) | ||
try { | ||
this.storage.removeItem(fullKey) | ||
} catch (err) { | ||
logError(Errors._702, `key ${key} – ${err.message}`) | ||
} | ||
} | ||
} | ||
|
||
export default Storage |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
import local from '../local' | ||
|
||
describe('local storage', () => { | ||
const { getItem, setItem } = local | ||
|
||
beforeAll(() => { | ||
window.localStorage.clear() | ||
}) | ||
|
||
afterEach(() => { | ||
window.localStorage.clear() | ||
}) | ||
|
||
describe('getItem', () => { | ||
it('returns a parsed value', () => { | ||
const stringifiedValue = JSON.stringify({ test: 'value' }) | ||
window.localStorage.setItem('SAFE__test', stringifiedValue) | ||
|
||
expect(getItem('test')).toStrictEqual({ test: 'value' }) | ||
}) | ||
it("returns undefined the key doesn't exist", () => { | ||
expect(getItem('notAKey')).toBe(undefined) | ||
}) | ||
}) | ||
|
||
describe('setItem', () => { | ||
it('saves a stringified value', () => { | ||
setItem('test', true) | ||
expect(getItem('test')).toBe(true) | ||
expect(window.localStorage.getItem('SAFE__test')).toBe('true') | ||
}) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,47 +1,35 @@ | ||
import { loadFromSessionStorage, removeFromSessionStorage, saveToSessionStorage } from '../session' | ||
|
||
describe('loadFromSessionStorage', () => { | ||
describe('session storage', () => { | ||
beforeEach(() => { | ||
window.sessionStorage.clear() | ||
}) | ||
it('returns a parsed value', () => { | ||
const stringifiedValue = JSON.stringify({ test: 'value' }) | ||
window.sessionStorage.setItem('test', stringifiedValue) | ||
|
||
expect(loadFromSessionStorage('test')).toStrictEqual({ test: 'value' }) | ||
}) | ||
it("returns undefined the key doesn't exist", () => { | ||
expect(loadFromSessionStorage('notAKey')).toBe(undefined) | ||
}) | ||
}) | ||
describe('loadFromSessionStorage', () => { | ||
it('returns a parsed value', () => { | ||
const stringifiedValue = JSON.stringify({ test: 'value' }) | ||
window.sessionStorage.setItem('SAFE__test', stringifiedValue) | ||
|
||
describe('saveToSessionStorage', () => { | ||
beforeEach(() => { | ||
window.sessionStorage.clear() | ||
expect(loadFromSessionStorage('test')).toStrictEqual({ test: 'value' }) | ||
}) | ||
it("returns undefined the key doesn't exist", () => { | ||
expect(loadFromSessionStorage('notAKey')).toBe(undefined) | ||
}) | ||
}) | ||
|
||
it('saves a stringified value', () => { | ||
saveToSessionStorage('test', true) | ||
describe('saveToSessionStorage', () => { | ||
it('saves a stringified value', () => { | ||
saveToSessionStorage('test', true) | ||
|
||
expect(window.sessionStorage?.test).toBe('true') | ||
}) | ||
}) | ||
|
||
describe('removeFromSessionStorage', () => { | ||
beforeEach(() => { | ||
window.sessionStorage.clear() | ||
expect(window.sessionStorage?.SAFE__test).toBe('true') | ||
}) | ||
}) | ||
|
||
it('removes the key', () => { | ||
Object.defineProperty(window, 'sessionStorage', { | ||
writable: true, | ||
value: { | ||
removeItem: jest.fn(), | ||
}, | ||
describe('removeFromSessionStorage', () => { | ||
it('removes the key', () => { | ||
window.sessionStorage.setItem('SAFE__test', '1') | ||
removeFromSessionStorage('test') | ||
expect(window.sessionStorage.getItem('SAFE__test')).toBe(null) | ||
}) | ||
|
||
removeFromSessionStorage('test') | ||
|
||
expect(window.sessionStorage.removeItem).toHaveBeenCalledWith('test') | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,54 +1,27 @@ | ||
import { ImmortalStorage, LocalStorageStore } from 'immortal-db' | ||
|
||
import { getNetworkName } from 'src/config' | ||
import { Errors, logError } from 'src/logic/exceptions/CodedException' | ||
import Storage from './Storage' | ||
|
||
const stores = [LocalStorageStore] | ||
export const storage = new ImmortalStorage(stores) | ||
export const storage = new Storage(window.localStorage, '') | ||
|
||
// We need this to update on run time depending on selected network name | ||
export const getStoragePrefix = (networkName = getNetworkName()): string => `v2_${networkName}` | ||
// We need this to update the key in runtime depending on selected network name | ||
export const getStoragePrefix = (networkName = getNetworkName()): string => `_immortal|v2_${networkName}__` | ||
|
||
export const loadFromStorage = async <T = unknown>( | ||
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. Need this still be async? 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. The code that uses it still expects a promise. 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. Do you not think it's worthwhile changing it? Or create a new issue to do that? 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. Yes, we should eventually un-async all that code. 👍 |
||
key: string, | ||
prefix = getStoragePrefix(), | ||
): Promise<T | undefined> => { | ||
try { | ||
const stringifiedValue = await storage.get(`${prefix}__${key}`) | ||
if (stringifiedValue === null || stringifiedValue === undefined) { | ||
return undefined | ||
} | ||
|
||
return JSON.parse(stringifiedValue) | ||
} catch (err) { | ||
logError(Errors._700, `key ${key} – ${err.message}`) | ||
return undefined | ||
} | ||
return storage.getItem(`${prefix}${key}`) | ||
} | ||
|
||
export const saveToStorage = async <T = unknown>(key: string, value: T): Promise<void> => { | ||
try { | ||
const stringifiedValue = JSON.stringify(value) | ||
await storage.set(`${getStoragePrefix()}__${key}`, stringifiedValue) | ||
} catch (err) { | ||
logError(Errors._701, `key ${key} – ${err.message}`) | ||
} | ||
storage.setItem<T>(`${getStoragePrefix()}${key}`, value) | ||
} | ||
|
||
// This function is only meant to be used in L2-UX migration to gather information from other networks | ||
export const saveMigratedKeyToStorage = async <T = unknown>(key: string, value: T): Promise<void> => { | ||
try { | ||
const stringifiedValue = JSON.stringify(value) | ||
await storage.set(key, stringifiedValue) | ||
} catch (err) { | ||
logError(Errors._703, `key ${key} – ${err.message}`) | ||
} | ||
export const removeFromStorage = async (key: string): Promise<void> => { | ||
storage.removeItem(`${getStoragePrefix()}${key}`) | ||
} | ||
|
||
export const removeFromStorage = async (key: string): Promise<void> => { | ||
try { | ||
await storage.remove(`${getStoragePrefix()}__${key}`) | ||
} catch (err) { | ||
logError(Errors._702, `key ${key} – ${err.message}`) | ||
} | ||
// This function is only meant to be used in L2-UX migration to gather information from other networks | ||
export const saveMigratedKeyToStorage = async <T = unknown>(key: string, value: T): Promise<void> => { | ||
storage.setItem(key, value) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import Storage from './Storage' | ||
|
||
const local = new Storage(window.localStorage) | ||
|
||
export default local |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,33 +1,11 @@ | ||
import { logError, Errors } from 'src/logic/exceptions/CodedException' | ||
import Storage from './Storage' | ||
|
||
export const loadFromSessionStorage = <T = unknown>(key: string): T | undefined => { | ||
try { | ||
const stringifiedValue = sessionStorage.getItem(key) | ||
const session = new Storage(window.sessionStorage) | ||
|
||
if (stringifiedValue === null || stringifiedValue === undefined) { | ||
return undefined | ||
} | ||
export default session | ||
|
||
return JSON.parse(stringifiedValue) | ||
} catch (err) { | ||
logError(Errors._704, `key ${key} – ${err.message}`) | ||
} | ||
} | ||
|
||
export const saveToSessionStorage = <T = unknown>(key: string, value: T): void => { | ||
try { | ||
const stringifiedValue = JSON.stringify(value) | ||
|
||
sessionStorage.setItem(key, stringifiedValue) | ||
} catch (err) { | ||
logError(Errors._705, `key ${key} – ${err.message}`) | ||
} | ||
} | ||
|
||
export const removeFromSessionStorage = (key: string): void => { | ||
try { | ||
sessionStorage.removeItem(key) | ||
} catch (err) { | ||
logError(Errors._706, `key ${key} – ${err.message}`) | ||
} | ||
} | ||
export const { | ||
getItem: loadFromSessionStorage, | ||
setItem: saveToSessionStorage, | ||
removeItem: removeFromSessionStorage, | ||
} = session |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
import { useState, useEffect } from 'react' | ||
import local from './local' | ||
|
||
const useStoredState = <T>(key: string): [T | undefined, React.Dispatch<React.SetStateAction<T>>] => { | ||
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. Maybe this should be name something like 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. "Local" state, to me, doesn't imply that it's stored in a permanent place. I would keep this name. |
||
const [cache, setCache] = useState<T>() | ||
|
||
useEffect(() => { | ||
const saved = local.getItem<T>(key) | ||
setCache(saved) | ||
}, [key, setCache]) | ||
|
||
useEffect(() => { | ||
local.setItem<T | undefined>(key, cache) | ||
}, [key, cache]) | ||
|
||
return [cache, setCache] | ||
} | ||
|
||
export default useStoredState |
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.
In the chains migration, I am trying to move all
networkId
references tochainId
. Do you think it's worthwhile including this here too?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.
Out of scope, I'm just removing the prefix here.