[Address Book v2] Fix AB v2 migration#2345
Conversation
|
CLA Assistant Lite All Contributors have signed the CLA. |
| if (safes) { | ||
| safes.forEach((safeProps) => { | ||
| dispatch(addOrUpdateSafe(buildSafe(safeProps))) | ||
| }) |
There was a problem hiding this comment.
Refactored this along the way.
ESLint Summary View Full Report
Report generated by eslint-plus-action |
| state.findIndex( | ||
| ({ address, chainId }) => chainId === addressBookEntry.chainId && sameAddress(address, addressBookEntry.address), | ||
| ) | ||
|
|
There was a problem hiding this comment.
A giant red blob following, but it's just a move to a different file.
There was a problem hiding this comment.
But I assume there are some changes after that file move, aren't there?
| localStorage.setItem(addressBookKey, JSON.stringify(addressBookToStore)) | ||
|
|
||
| // update stored safe | ||
| localStorage.setItem(safesKey, JSON.stringify(migratedSafes)) |
There was a problem hiding this comment.
@fernandomg as you see, I've added a redundant call to set the storage item directly, so that we don't run in any async issues with the saveSafes (on the following line).
There was a problem hiding this comment.
Is there a possibility for some race condition with immortaldb?
There was a problem hiding this comment.
@mikheevm this migration is executed on store import time, and ImmortalDB kicks in after that (in the main index.ts).
ImmortalDB's self-healing mechanism is quite... interesting, and if we want to be 100% on the safe side, I would disable the IndexedDB storage. And in future ultimately replace ImmortalDB with just localStorage for simplicity.
There was a problem hiding this comment.
@mikheevm do you mean immortalDB overwriting localStorage with the old data that's in indexedDB?
There was a problem hiding this comment.
I'll merge this PR now to unblock further development on the epic branch. Let's discuss what we will do with ImmortalDB in #2346.
| localStorage.setItem(newKey, JSON.stringify(migratedAddressBook)) | ||
|
|
||
| // Remove the old Address Book storage | ||
| localStorage.removeItem(storageKey) |
There was a problem hiding this comment.
This removes the old item only from the localStorage but ImmortalDB actually also saves to the IndexDB. It will remain there forever.
There was a problem hiding this comment.
Shouldn't we keep the removeFromStorage call from the previous version (https://github.com/gnosis/safe-react/pull/2345/files#diff-ac8dd1d3ddf3939b9aa134689ad6f0ba12d8dca2298af2729792f1efe0e941a0L268)?
There was a problem hiding this comment.
True, restoring 👍
fernandomg
left a comment
fernandomg
left a comment
There was a problem hiding this comment.
Nice! Just tested locally and it worked great.
Sorry for the abuse of the .entries/.fromEntries methods. I promise to use them only when necessary. 😬
4 participants
What it solves
Resolves #2315.
There was a bug that we didn't migrate the safe owners. It used to be an object with name and address, and now it's just an address string.
Because of this bug, ImmortalDB was writing an empty
{}into the SAFES storage.How this PR fixes it
It now maps owners to addresses.
How to test it
Create (or update) a localStorage key
_immortal|v2_RINKEBY__SAFESand paste the content of this key on production.Same with the
_immortal|v2_RINKEBY__ADDRESS_BOOK_STORAGE_KEYkey.After a page reload,
_immortal|v2_RINKEBY__SAFESshould be migrated, and_immortal|v2_RINKEBY__ADDRESS_BOOK_STORAGE_KEYcompletely removed.