-
Notifications
You must be signed in to change notification settings - Fork 207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor!: migrate from .afj to .credo #2161
refactor!: migrate from .afj to .credo #2161
Conversation
Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>
|
Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>
Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>
Signed-off-by: Sai Ranjit Tummalapalli <[email protected]>
cc: @TimoGlastra @genaris |
Thanks for this PR @sairanjit, I think it would be good to wait for a bit with releasing this, as it makes testing with 0.6 unstable versions easier (due to that a migration script can only be run once per version). We should probably look into multi-updates at some point |
* Migrate data from .afj to .credo if .afj exists. | ||
* Copy the contents from old directory (.afj) to new directory (.credo). | ||
*/ | ||
public async migrateWalletToCredoFolder(walletId: string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the file system should stay very generic. So instead of adding this method migrateWalletToCredoFolder
in the file system (which then needs to be implemented by every file system), I'd move it to the migration method. This can then call the needed methods exists
and copyDirectory
(i think this method can also copy files now? Should we create one generic copy
method that can do both directories (using recursive
property) and files?)
for await (const path of pathsToMigrate) { | ||
// Migrate if the old paths exist | ||
if (await this.exists(path.from)) { | ||
await this.copyDirectory(path.from, path.to) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we delete the old files afterwards? Or how do we clean up?
* Migrate data from .afj to .credo if .afj exists. | ||
* Copy the contents from old directory (.afj) to new directory (.credo). | ||
*/ | ||
public async migrateWalletToCredoFolder() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, this can be moved to migration logic so we don't have to duplicate
* Migrates the sqlite folder location from .afj to .credo in the storage directory in node and react native. | ||
* | ||
*/ | ||
export async function migrateToCredoFolder<Agent extends BaseAgent>(agent: Agent) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
due to the sensivity in doing migrations it's always good to test these migrations extensively. For all other migrations and version changes we have tests (per migration method, and for a version migration in general), can you add these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I'm not sure about is how this works:
- We initialize the agent and open the wallet (this will use the new paths, so a new wallet will be created)
- The wallet will be initialized with storage version 0.6
- No updates need to happen because we're on the new path
- we now have a new empty wallet
Or am i missing something here?
Based on the discussion in the Credo Contributors meeting on 30th January 2025 we can keep |
Summary
.afj
data folder to.credo
#1715Questions
.afj
folder or old wallet after migrating to.credo
folder ?