revert: deprecating more networks (#23695)#24369
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
5add3d1 to
e1d0760
Compare
03b9de3 to
af2eab9
Compare
Builds ready [af2eab9]
Page Load Metrics (341 ± 406 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #24369 +/- ##
===========================================
- Coverage 67.36% 67.29% -0.07%
===========================================
Files 1282 1282
Lines 50011 49991 -20
Branches 12983 12971 -12
===========================================
- Hits 33685 33638 -47
- Misses 16326 16353 +27 ☔ View full report in Codecov by Sentry. |
app/scripts/migrations/117.ts
Outdated
There was a problem hiding this comment.
Shouldn't the revert PR only revert the 115.ts file which was responsible to migrate users ti linea sepolia?
I think 117.ts and 116.ts logic should still stay and maybe be renamed if we are removing 115.ts?
There was a problem hiding this comment.
In this PR, we address changes to our migration files. Specifically, we've removed the migration file named 115.ts. During the period since its creation, two additional migration files, 116.ts and 117.ts, were introduced. To maintain a sequential order and fill the gap left by the deletion of 115.ts, we've renumbered these files: 116.ts has been renamed to 115.ts, and 117.ts has been renamed to 116.ts. This adjustment ensures our migration files are sequentially organized without any missing numbers in the sequence.
There was a problem hiding this comment.
Renaming migration files does not seem a good idea as it might lead to migrations being skipped for anyone that is between 115 and 117 (inclusive). For example this should prevent anyone already on version 117 to skip the future 117 migration file when it is added again.
There was a problem hiding this comment.
hey @cryptotavares ,
I agree with your points, but it's important to note a crucial detail: the migration file in question has never been deployed to a production environment, nor has it been run at all.
There was a problem hiding this comment.
@salimtb it has been run in develop, meaning that everyone's local env might be affected. Example: if I was to add an 117 migration right now, I would not be able to trigger it locally without modifying the extension persisted state.
(output from window.stateHooks.getCleanAppState().then(console.log) in the browser console)
It has also been included in the 11.16 release cut. We can cherry pick a fix into 11.16 release candidate.. but I suspect that anyone that is already QAing that branch might face similar issues, if we end up having to cherry pick a commit to add another migration (future 117).
In my opinion, it would be cleaner and simpler to just add another migration (lets say 118) that would revert 115. But I don't know how we have approach this in the past. @Gudahtt or @danjm, how have we dealt with this in the past?
There was a problem hiding this comment.
eeemm ok , thnx for the clarification @cryptotavares , i'll add another file ( 118.ts ) like you just suggested.
will ping you in a few moment once it's done.
thnx for your comment and clarification
There was a problem hiding this comment.
@cryptotavares update done , pls check again
There was a problem hiding this comment.
@cryptotavares I agree that it is preferable to avoid removing already committed migration files when we can. At the same time, I don't like having migration code that does nothing.
We faced a similar issue about a year ago, and because the changes were not yet on master, we decided to renumber the migration files https://consensys.slack.com/archives/CTQAGKY5V/p1677784034578949
There was a problem hiding this comment.
I didn't see this thread before, but Salim flagged it for me after I opened this #24666
There was a problem hiding this comment.
Meanwhile, this was not as big of an issue a year ago:
It has also been included in the 11.16 release cut. We can cherry pick a fix into 11.16 release candidate.. but I suspect that anyone that is already QAing that branch might face similar issues, if we end up having to cherry pick a commit to add another migration (future 117).
There were many fewer people building release branches then... I think I still lean in favour of deleting migration files if they are not needed and have not yet been merged to master.
Builds ready [5b08f80]
Page Load Metrics (983 ± 610 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
ca78b52 to
17ac937
Compare
17ac937 to
50aae8e
Compare
| ...state, | ||
| NetworkController: networkControllerState, | ||
| }; | ||
| return state; |
There was a problem hiding this comment.
I see that basically we wanted to keep the 115 migration but we are not making any state modifications, then in future it will be reverted back
|
LGTM |
| const testNetworks = { | ||
| [CHAIN_IDS.GOERLI]: true, | ||
| [CHAIN_IDS.SEPOLIA]: true, | ||
| [CHAIN_IDS.LINEA_GOERLI]: true, |
There was a problem hiding this comment.
I think we still need to get rid of goerli.
There was a problem hiding this comment.
yes, this revert PR is only temporary, and it is to make sure the deprecation only happens in v11.17
Builds ready [50aae8e]
Page Load Metrics (1226 ± 706 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
This reverts commit 257e9f9.
Description
Related issues
Fixes:
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist