chainid migration #2161
chainid migration #2161
Conversation
Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com>
app/store/migrations.js
Outdated
| // If provider is rpc, check if the current network has a valid chainId | ||
| const storedChainId = typeof provider.chainId === 'string' ? provider.chainId : ''; | ||
| const isDecimalString = /^[1-9]\d*$/u.test(storedChainId); | ||
| const isCustomRpcWithInvalidChainId = !isDecimalString || !isSafeChainId(parseInt(storedChainId, 10)); |
There was a problem hiding this comment.
This should probably be renamed to hasInvalidChainId since it's not only for custom networks.
app/store/migrations.js
Outdated
| // If provider is rpc, check if the current network has a valid chainId | ||
| const storedChainId = typeof provider.chainId === 'string' ? provider.chainId : ''; | ||
| const isDecimalString = /^[1-9]\d*$/u.test(storedChainId); | ||
| const isCustomRpcWithInvalidChainId = !isDecimalString || !isSafeChainId(parseInt(storedChainId, 10)); |
There was a problem hiding this comment.
I'm not sure that we should change migration 2 at this point. I think it introduces more uncertainty for no benefit, since migration 3 will take care of any issues with migration 2.
At least in the extension, we've never changed a migration that's run in production, AFAIK.
| @@ -47,7 +48,31 @@ export const migrations = { | |||
| }; | |||
| } | |||
| return state; | |||
| }, | |||
| 3: state => { | |||
| const provider = state.engine.backgroundState.NetworkController.provider; | |||
There was a problem hiding this comment.
is there ever a chance provider will be undefined? my guess is no, but maybe it'd be safer to use optional chaining for this property? eg:
state.engine.backgroundState.NetworkController?.provider
wdyt?
There was a problem hiding this comment.
I don't think it'll ever be anything other than an object, but investigating the matter did lead me to this discovery: #2164
There was a problem hiding this comment.
Adding optional chaining is obviously harmless so I'm fine with it either way!
|
My requested changes have been addressed, by I'll let a mobile dev approve, because there are changes unrelated to the migration that I don't understand. |
This reverts commit cdceb58.
…le into chainid-migration
* chainidmigration * Update app/store/migrations.js Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com> * migration2 * bump * logs * migrations * bump * circle * Revert "bump" This reverts commit cdceb58. Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com>
* chainidmigration * Update app/store/migrations.js Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com> * migration2 * bump * logs * migrations * bump * circle * Revert "bump" This reverts commit cdceb58. Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com>
Description
Write a short description of the changes included in this pull request.
Checklist
Issue
Resolves #???