doc: migration best practices #70
Conversation
|
I'm currently with a doubt on this migration file Since we need to address a lot of changes with different parts of the state, I wonder if it would be a good practice to split and write multiple versions instead of one. What could be the rule? When a different controller migration create a different version? This is because when applying the migrations it will run across every condition (that we check if the state currently exists) and if not it will throw an exception, and this will break the tests, creating the need to have the old state and new state of different test cases that are not strictly testing the change you want, but at the same time doesn't make it inaccurate This my point of view it would not break production anyway on the case any of this states didn't exist because we capture the exception and that shouldn't break the app from work (we are not throwing an error just creating one), let me know your thoughts! |
| return state; | ||
| ``` | ||
|
|
||
| - Keeping Migrations Idempotent: Make sure your migrations are idempotent, meaning they can be run multiple times without changing the result beyond the initial application. This makes migrations safer and easier to test. |
There was a problem hiding this comment.
I don't think we've ever written migrations this way. Could you elaborate on why we'd do this? Currently we store the number of the last migration run in state, to ensure migrations are never run twice.
| ``` | ||
|
|
||
| - Keeping Migrations Idempotent: Make sure your migrations are idempotent, meaning they can be run multiple times without changing the result beyond the initial application. This makes migrations safer and easier to test. | ||
| - Removing Old Migrations: Over time, you may accumulate many migrations. At some point, it may be safe to remove old migrations, especially if you know that all users have migrated to a newer version of the state. Be cautious with this, as removing a migration could break the ability to upgrade for any users who are still on old versions of the state. |
There was a problem hiding this comment.
This seems a bit contradictory to me. This suggests that it "may be safe" to remove old migrations, while acknowledging that it could break for some users.
There is no way for us to know whether any users have dormant wallet instances, so this is never a completely safe thing to do.
There was a problem hiding this comment.
Will we ever start to end of life application versions? This is done on the mobile side when we force upgrade.
This may not apply now but could apply in the future. We can remove this section but it is something we should keep in mind.
There was a problem hiding this comment.
Hmm, so maybe we should clearly define these terms a bit more. I interpreted "safe" as meaning "won't break anything", which is different from this notion of which versions are "end-of-life" (i.e. no longer supported/considered).
For old version support, there are two separate cases to consider. First is which versions we support being actively used, and the second is which versions we support updating from. For active use, support is limited to just the versions that have working API integrations. I guess that's what your team is using the forced updates for, and that's likely what the extension team would do as well if they had this feature.
But at the moment we support upgrading from any version, so that a user can update and things be in working order no matter the version that was last used. Dropping migration support for older versions would be a new thing, to my knowledge.
There was a problem hiding this comment.
We can certainly consider dropping migration support, but we don't really have a way to know how many affected wallets are out there. Any affected wallets would be dormant until the user wanted to update them, so we'd have no way to collect that data.
We should have a plan for how customer success handles these cases, if we are going to do something like that. We could have improved in-app support for that case as well, e.g. detect old versions and allow exporting the SRP and any keys (after the password is validated of course).
But it might be easier to leave them in-place for now, than it would be to make this a nice experience that wouldn't overly burden our CS team.
docs/mirgrations.md
Outdated
| ## Best Practices | ||
| - When to Add a Migration: You should add a migration whenever you make a change to the shape of your state that is not backwards-compatible. This includes adding or removing properties, changing the type of a property, or moving properties around within the state tree (e.g. breaking controllers changes, etc...). | ||
| - Detecting Errors in a Migration: The best way to detect errors in a migration is through thorough testing. Write tests that take various shapes of old state and ensure that they are correctly transformed into the new state shape. Also, use TypeScript or another type system to help catch type errors. | ||
| - Handling Migration Errors: If a migration fails, you should have a strategy in place to handle the error. This could be as simple as logging the error and continuing with the default state, or it could involve more complex error recovery logic. In any case, it's important to ensure that your app can still function in some way even if a migration fails. |
There was a problem hiding this comment.
For extension migrations, the strategy we've been using for handling errors is:
- Abort the migration
- Log an error and submit an error to Sentry
- Continue running next migrations
This should leave the application in a broken state in some cases, but hopefully also in a recoverable state (if we ship a patch with new migrations to fix any state problems we see).
On a related point, we also have the practice of logging to the console if we skip a migration for any reason (e.g. maybe the state being migrated doesn't exist in the users state). This is useful for debugging.
But I'd strongly recommend against simply logging an error and moving on when a legitimate error is encountered. We need the sentry report to have visibility into the error, to be able to fix it.
I'd also hesitate to state "ensure that your app can still function in some way even if a migration fails." as a goal. There are plenty of migration errors we might encounter that would leave the application in an unusable state, which I don't think we can do anything about at the migration layer. Potentially we could make application initialization more robust so that it deals with invalid state more gracefully, but I write migrations to be recoverable first and foremost. Not to provide valid state given any invalid input, that's not achievable.
|
|
||
| ## Best Practices | ||
| - When to Add a Migration: You should add a migration whenever you make a change to the shape of your state that is not backwards-compatible. This includes adding or removing properties, changing the type of a property, or moving properties around within the state tree (e.g. breaking controllers changes, etc...). | ||
| - Detecting Errors in a Migration: The best way to detect errors in a migration is through thorough testing. Write tests that take various shapes of old state and ensure that they are correctly transformed into the new state shape. Also, use TypeScript or another type system to help catch type errors. |
There was a problem hiding this comment.
I'd recommend making zero assumptions about the incoming state shape, i.e. going well beyond just testing old state versus new. The users state may be corrupted, so the input state may be invalid in unexpected ways. In those cases, it would be best to try to leave the user's state unaltered, so that we have some hope of recovering the data with a later migration or with the vault decryption tool.
remove should Co-authored-by: Nico MASSART <[email protected]>
remove should Co-authored-by: Nico MASSART <[email protected]>
Remove should and add must Co-authored-by: Nico MASSART <[email protected]>
|
Replaced in favor of #96 |
This PR attempts to put together best practices documentation for how MetaMask contributors should think of migrations.