Enhancement: cache owned Safes in localStorage #3066

katspaugh · 2021-11-26T10:18:54Z

What it solves

Owned safes on other networks are gone when you refresh the page.

How this PR fixes it

Keeps them in localStorage.

I've added a wrapper around localStorage that does that. Plus a hook.

How to test it

Open the sidebar, switch between networks.
Refresh the page
Owned safes on all networks should be still in the sidebar

github-actions · 2021-11-26T10:19:11Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2021-11-26T10:22:47Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	3	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

coveralls · 2021-11-26T10:23:41Z

Pull Request Test Coverage Report for Build 1526576110

33 of 50 (66.0%) changed or added relevant lines in 8 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.01%) to 32.751%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/utils/storage/index.ts	6	8	75.0%
src/utils/storage/Storage.ts	22	25	88.0%
src/logic/safe/hooks/useOwnerSafes.ts	0	4	0.0%
src/utils/storage/useStoredState.ts	0	8	0.0%

Totals
Change from base Build 1526428491:	-0.01%
Covered Lines:	3110
Relevant Lines:	8463

💛 - Coveralls

iamacook

Looking good! I think there is some duplicate logic of what we already have though.

iamacook · 2021-11-26T10:22:56Z

src/utils/storage/__tests__/local.ts

@@ -0,0 +1,31 @@
+import { getItem, setItem } from '../local'


I'm surprised this passes. Normally I have to mock the localStorage.

Somehow it's already mocked.

iamacook · 2021-11-26T10:23:13Z

src/utils/storage/local.ts

@@ -0,0 +1,20 @@
+const PREFIX = 'SAFE__'
+
+const prefixKey = (key: string): string => `${PREFIX}${key}`


We already have a function for getting the prefix?

Can you point me to it?

getStoragePrefix() at src/utils/storage/index.ts

That one is for immortal.

Do we want to follow a versioning convention though, just in case we make any drastic changes?

We'll have to live with two keys for now: the legacy immortal ones and the new SAFE ones.
The first ones, we'll need to eventually migrate (not a high prio I think).

As for versioning, We can always slap a version on top of the new key, e.g. SAFE_v2. Once there's a need.

iamacook · 2021-11-26T10:23:31Z

src/utils/storage/local.ts

+
+const prefixKey = (key: string): string => `${PREFIX}${key}`
+
+export const getItem = <T>(key: string): T | undefined => {


Why are you not using the functions we already have for storage interaction?

Which ones?

loadFromStorage() and saveToStorage() at src/utils/storage/index.ts

I created some for the sessionStorage at src/utils/storage/session.ts that don't rely on ImmortalDB. They might be of help.

I'll merge session and local, they share 99% of the code.

github-actions · 2021-11-26T10:33:07Z

Deployment links

🟠 Safe Rinkeby	⚪ Safe Mainnet	🟣 Safe Polygon	🟡 Safe BSC	⚫ Safe Arbitrum	🟢 Safe xDai

github-actions · 2021-11-26T13:39:18Z

E2E Tests Failed
Check the results here: https://github.com/gnosis/safe-react-e2e-tests/actions/runs/1526621355

Failed tests:

❌ Address book Address book
❌ Safe Apps List Safe Apps List
❌ Read-only transaction creation and review Create and review a Send Funds transaction

iamacook

Looking good. Just a few things

iamacook · 2021-11-29T09:22:21Z

src/config/index.ts

@@ -43,7 +43,7 @@ const isNetworkId = (id: unknown): id is ETHEREUM_NETWORK => {
  return Object.values(ETHEREUM_NETWORK).some((network) => network === id)
 }

-export const NETWORK_ID_KEY = 'SAFE__networkId'
+export const NETWORK_ID_KEY = 'networkId'


In the chains migration, I am trying to move all networkId references to chainId. Do you think it's worthwhile including this here too?

Out of scope, I'm just removing the prefix here.

iamacook · 2021-11-29T09:23:02Z

src/logic/exceptions/registry.ts

@@ -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',


Why have you removed these? The /load and /open routes use the sessionStorage to retrieve the current chainId.

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 localStorage -> storage.

Alright, I would suggest generalising 701-703 then.

iamacook · 2021-11-29T09:26:36Z

src/utils/storage/Storage.ts

+      }
+    }
+
+    return data as unknown as T | undefined


Why not just return on line 32/35? You won't need to cast to both types, only line 32 to T.

iamacook · 2021-11-29T09:27:12Z

src/utils/storage/index.ts

-// 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>(


Need this still be async?

The code that uses it still expects a promise.

Do you not think it's worthwhile changing it? Or create a new issue to do that?

Yes, we should eventually un-async all that code. 👍
Created #3073

iamacook · 2021-11-29T09:28:14Z

src/utils/storage/useStoredState.ts

+import { useState, useEffect } from 'react'
+import local from './local'
+
+const useStoredState = <T>(key: string): [T | undefined, React.Dispatch<React.SetStateAction<T>>] => {


Maybe this should be name something like useLocalState just to be clear that it isn't involving the sessionStorage.

"Local" state, to me, doesn't imply that it's stored in a permanent place. I would keep this name.

francovenica · 2021-12-02T01:16:55Z

Looks good to me.
Still a mystery were you actually save them, I've checked the local storage and I was expecting to find the almost 700 safes I have with one of my owners in rinkeby :D

Checked:
The owned safes are remembered while switching networks. The list remains there is accessible and they don't have to load every time you switch networks
They have to be refreshed if you switch owners ( you have to visit every network again) which is ok.
If you disconnect the "owner safes" dropdown disappears which is correct as well.

katspaugh · 2021-12-02T08:53:15Z

@francovenica thanks! They are stored under SAFE__ownedSafes.

katspaugh requested a review from iamacook November 26, 2021 10:18

katspaugh added the Enhancement ✨ Minor Improvement / changes to existing functionality label Nov 26, 2021

Enhancement: cache owned Safes in localStorage

4270e44

iamacook reviewed Nov 26, 2021

View reviewed changes

katspaugh force-pushed the cache-owned branch from a55e877 to 4270e44 Compare November 26, 2021 10:25

Rm ImmortalDB, unify session and local storages

bbd438d

iamacook reviewed Nov 29, 2021

View reviewed changes

katspaugh mentioned this pull request Nov 29, 2021

Get rid of loadFromStorage and saveToStorage #3073

Closed

Code style; generalize error messages

be525a2

iamacook approved these changes Nov 29, 2021

View reviewed changes

katspaugh added 3 commits November 29, 2021 10:53

Merge branch 'dev' into cache-owned

ddb62b4

Merge branch 'dev' into cache-owned

e5a4535

Merge branch 'dev' of github.com:gnosis/safe-react into cache-owned

424f9b4

katspaugh merged commit 134c787 into dev Dec 2, 2021

katspaugh deleted the cache-owned branch December 2, 2021 08:53

github-actions bot locked and limited conversation to collaborators Dec 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement: cache owned Safes in localStorage #3066

Enhancement: cache owned Safes in localStorage #3066

katspaugh commented Nov 26, 2021

github-actions bot commented Nov 26, 2021

github-actions bot commented Nov 26, 2021 •

edited

Loading

coveralls commented Nov 26, 2021 •

edited

Loading

iamacook left a comment

iamacook Nov 26, 2021 •

edited

Loading

katspaugh Nov 29, 2021

iamacook Nov 26, 2021

katspaugh Nov 26, 2021

iamacook Nov 26, 2021

katspaugh Nov 26, 2021

iamacook Nov 26, 2021

katspaugh Nov 26, 2021

iamacook Nov 26, 2021

katspaugh Nov 26, 2021

iamacook Nov 26, 2021

iamacook Nov 26, 2021

katspaugh Nov 26, 2021

github-actions bot commented Nov 26, 2021

github-actions bot commented Nov 26, 2021 •

edited

Loading

iamacook left a comment

iamacook Nov 29, 2021

katspaugh Nov 29, 2021

iamacook Nov 29, 2021

katspaugh Nov 29, 2021 •

edited

Loading

iamacook Nov 29, 2021

iamacook Nov 29, 2021

iamacook Nov 29, 2021

katspaugh Nov 29, 2021

iamacook Nov 29, 2021

katspaugh Nov 29, 2021

iamacook Nov 29, 2021

katspaugh Nov 29, 2021

francovenica commented Dec 2, 2021

katspaugh commented Dec 2, 2021

		@@ -0,0 +1,20 @@
		const PREFIX = 'SAFE__'

		const prefixKey = (key: string): string => `${PREFIX}${key}`


		const prefixKey = (key: string): string => `${PREFIX}${key}`

		export const getItem = <T>(key: string): T \| undefined => {

Enhancement: cache owned Safes in localStorage #3066

Enhancement: cache owned Safes in localStorage #3066

Conversation

katspaugh commented Nov 26, 2021

What it solves

How this PR fixes it

How to test it

github-actions bot commented Nov 26, 2021

github-actions bot commented Nov 26, 2021 • edited Loading

ESLint Summary View Full Report

coveralls commented Nov 26, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1526576110

💛 - Coveralls

iamacook left a comment

Choose a reason for hiding this comment

iamacook Nov 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 26, 2021

Deployment links

github-actions bot commented Nov 26, 2021 • edited Loading

iamacook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katspaugh Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francovenica commented Dec 2, 2021

katspaugh commented Dec 2, 2021

github-actions bot commented Nov 26, 2021 •

edited

Loading

coveralls commented Nov 26, 2021 •

edited

Loading

iamacook Nov 26, 2021 •

edited

Loading

github-actions bot commented Nov 26, 2021 •

edited

Loading

katspaugh Nov 29, 2021 •

edited

Loading