-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add import/export dexie db workflow to web client #740
Conversation
records.map(recursivelyTransformForImport) | ||
); | ||
|
||
await table.bulkPut(transformedRecords); |
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.
Here we are upserting all the exported entries in the db. This means that the data before importing will also be present. If we don't do some checks/extra logic before upserting there are some problems that might arise. Here are some that I can think of right now:
- An imported account already existed in the db and the imported state is older. The latest account state would be overwritten and if the account was private it might not be recoverable.
- The imported notes could be from the past/future relative to the client. In each case, the imported note needs different handling (for context here's the current import note logic).
- I think the
stateSync
table in the web store will also be overwritten, basically moving the client to the past/future. This might also cause issues with already tracked notes/accounts as the sync request will depend on the current block of the client.
Basically it's not as straightforward to introduce new data to an existing client.
I'm not sure what is the intended use for this db import/export but it might be better we could change it so that the old db is deleted before inserting. Then, these problems would disapear. It would just be a way of copying and pasting your client state to another computer. The clear downside here is that the old data will be lost. It depends on the intended use, if we want this to be for new empty clients then it's ok, if not then we will need to add some additional logic to deal with the importing of new elements.
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.
If the idea is to use it for new clients then maybe the "import" process could be moved to a constructor, so we remove the possibility of a user deleting their existing data.
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.
Yes makes sense. The intended use is as you described, to simply move data from one computer to another or to for example backup a snapshot state, and restore from that state (destructively) if for example your local machine failed in some way. It's not designed to be "cleanly" applied on top of an existing db, at least not for its current use-case. The way we intend to allow chrome wallet users to access this ultimately is only when creating a wallet for the first time, offering the opportunity from a clean slate, to import from a file.
Totally understand what you're saying wrt providing a way for the users to shoot themselves in the foot. I can rename functions to make things clearer i.e. overwriteDb
, forceImportDb
. This plus some better documentation as @igamigo mentioned I think would cover safety of usage for majority of cases.
I don't think there's a straightforward way to make it a constructor option as you're presenting it, though I understand your thought process. Ultimately an sdk user can instantiate as many WebClient
miden web client connections as they'd like. It utilizes the wasm to access the dexie db, but it can spin up as many independent connections as it wants as each client instantiates its own unique connection to the main, single, dexie db in the browser. I suppose we could track which access is the "first" and in that way enforce it, but that doesn't feel clean to me.
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.
Instead of upserting data, could it somehow override the existing database?
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.
Ultimately an sdk user can instantiate as many WebClient miden web client connections as they'd like. It utilizes the wasm to access the dexie db, but it can spin up as many independent connections as it wants as each client instantiates its own unique connection to the main, single, dexie db in the browser.
Is the dexie db shared between separate WebClient
instances?
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.
In the import js call we could have a pre-process step and remove any existing data from all of the tables. Perhaps wrap the whole thing in an atomic txn call in case the bulkPuts fail. Is that in line what you're thinking?
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.
Is the dexie db shared between separate WebClient instances?
@tomyrd Yes, that's mostly correct. Presuming they're part of the same browser session profile (i used session here, incorrectly... it persists across sessions) . If you went incognito for example, it will separate out the dbs even if using chrome for both.
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.
Yes, that's mostly correct. Presuming they're part of the same browser session. If you went incognito for example, it will separate out the dbs even if using chrome for both.
Oh, I didn't know this. So if we clear de db to import in one client it will delete the data for other separate instances?
On a separate note: How do separate clients deal with the syncState
table where it's only one entry that gets updated? They also share it? Like a sync in one client will sync the others?
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.
It will delete the data in the Miden dexie db for that browser profile to be precise. All WebClient
instances point to the same Miden dexie db. Which leads to the answer for the second question which is all operations work with the singular Miden dexie db (which has its single syncState
table) in the browser profile.
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.
In practice, in the wallet we're developing for example, you only have one instance of the client that's in charge of running the syncState
operation. Other client instances (or the same client instance depending on the design) can then perform operations independent of that one.
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.
Thanks! Code looks good, but I agree with @tomyrd in that there could be unwanted consequences so depending on the usecase, a slightly more specific approach could be used in order to avoid 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.
Thanks!
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.
👍🏼
528ecb5
to
11a2ab4
Compare
Adds the ability to dump a user's entire dexie db into js. This can then be passed back and forth in a file for the user to re-import later.
Future additions can include specifying the tables / specific information pertaining to the user.
Attempted to use libraries like
dexie-export-import
to avoid writing the recursive traversal as seen, but something about the encoding of the blob that it outputted was not working with the serialization layers being passed from js to wasm to js.