fix(indexed-db): reset now clears db tables

[fczuardi]

Co-authored-by: @taboca
Co-authored-by: @mquasar

Describe the changes you have made in this PR

A clear and concise description of what you want to happen

Link this PR to an issue [optional]

Fix: #2435

Type of change

(Remove other not matching type)

• fix: Bug fix (non-breaking change which fixes an issue)
• feat: New feature (non-breaking change which adds functionality)
• feat!: Breaking change (fix or feature that would cause existing functionality to not work as expected)
• docs: Documentation update

Screenshots of the changes [optional]

Add screenshots to make your changes easier to understand. You can also add a video here.

How has this been tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Checklist

• My code follows the style guidelines of this project and performed a self-review of my own code
• New and existing tests pass locally with my changes
• I checked if I need to make corresponding changes to the documentation (and made those changes if needed)

[github-actions]

🚀 Thanks for the pull request!

Here are the current build files for testing:

• chrome.zip.zip
• firefox.xpi.zip
• opera.crx.zip

Download and unzip the file for your browser. Refer to the readme for detailed install instructions.

Don't forget: keep earning sats!

[reneaaron]

Thanks for the PR!

As @rolznz noted I think the browser.storage.clear() APIs would be a better fit here as they delete all data and do not need to be adjusted in case there are new tables added in future.

[mquasar]

Thanks for the PR!

As @rolznz noted I think the browser.storage.clear() APIs would be a better fit here as they delete all data and do not need to be adjusted in case there are new tables added in future.

We tried that but the data still 'lives on' in the IndexedDB, apparently. Does it make sense?

[taboca]

Thanks for the PR!

As @rolznz noted I think the browser.storage.clear() APIs would be a better fit here as they delete all data and do not need to be adjusted in case there are new tables added in future.

Thanks for supporting us @reneaaron and @rolznz

Although there is a situation that loading data ( such as payments ) comes from browser.storage API - which would indeed suggest that just deleting browser.storage is enough - yet based on local and limited tests, we have stumbled in a situation where IndexedDB data was persistent.

The part of the code that gives us that idea is the function loadFromStorage:

lightning-browser-extension/src/extension/background-script/db.ts

Line 94 in 5bddad2

async loadFromStorage() {

In addition, notice the following section example where the logic gives up the load sync from browser.storage API in the situation where IndexedDB data exists: https://github.com/getAlby/lightning-browser-extension/blob/5bddad27cd0f58dcb83ffbceb5f3cf1317bb517c/src/extension/background-script/db.ts#LL116C6-L129C12

With the above said it is correct that there is a situation where IndexedDB is not present, which indeed require us to also clear the data from browser.storage

lightning-browser-extension/src/extension/background-script/db.ts

Line 106 in 5bddad2

} else if (result.allowances && result.allowances.length > 0) {

Some other observations #2435 (comment)

With the above said, if the suggestion has to do with easy maintenance, and if we do the "browser.storage.clear()" then we would have to keep some consistency with wiping the IndexedDB database entirely too? Another idea is to try to do the maintenance from the loadFromStorage function in db.ts which is the code that gets kicked in app initialization time.

[rolznz]

Thanks for the PR!
As @rolznz noted I think the browser.storage.clear() APIs would be a better fit here as they delete all data and do not need to be adjusted in case there are new tables added in future.

Thanks for supporting us @reneaaron and @rolznz

Although there is a situation that loading data ( such as payments ) comes from browser.storage API - which would indeed suggest that just deleting browser.storage is enough - yet based on local and limited tests, we have stumbled in a situation where IndexedDB data was persistent.

The part of the code that gives us that idea is the function loadFromStorage:

lightning-browser-extension/src/extension/background-script/db.ts

Line 94 in 5bddad2

async loadFromStorage() {

In addition, notice the following section example where the logic gives up the load sync from browser.storage API in the situation where IndexedDB data exists: https://github.com/getAlby/lightning-browser-extension/blob/5bddad27cd0f58dcb83ffbceb5f3cf1317bb517c/src/extension/background-script/db.ts#LL116C6-L129C12

With the above said it is correct that there is a situation where IndexedDB is not present, which indeed require us to also clear the data from browser.storage

lightning-browser-extension/src/extension/background-script/db.ts

Line 106 in 5bddad2

} else if (result.allowances && result.allowances.length > 0) {

Some other observations #2435 (comment)

With the above said, if the suggestion has to do with easy maintenance, and if we do the "browser.storage.clear()" then we would have to keep some consistency with wiping the IndexedDB database entirely too? Another idea is to try to do the maintenance from the loadFromStorage function in db.ts which is the code that gets kicked in app initialization time.

@mquasar @taboca thanks for testing and looking deeper into the issue. I think the goal is to delete absolutely everything when resetting the extension. So any and all storage mechanisms (indexedDb, browser.storage, etc) we save data to should be completely cleared.

[taboca]

Thanks for the PR!
As @rolznz noted I think the browser.storage.clear() APIs would be a better fit here as they delete all data and do not need to be adjusted in case there are new tables added in future.

Thanks for supporting us @reneaaron and @rolznz
Although there is a situation that loading data ( such as payments ) comes from browser.storage API - which would indeed suggest that just deleting browser.storage is enough - yet based on local and limited tests, we have stumbled in a situation where IndexedDB data was persistent.
The part of the code that gives us that idea is the function loadFromStorage:

lightning-browser-extension/src/extension/background-script/db.ts

Line 94 in 5bddad2

async loadFromStorage() {

In addition, notice the following section example where the logic gives up the load sync from browser.storage API in the situation where IndexedDB data exists: https://github.com/getAlby/lightning-browser-extension/blob/5bddad27cd0f58dcb83ffbceb5f3cf1317bb517c/src/extension/background-script/db.ts#LL116C6-L129C12
With the above said it is correct that there is a situation where IndexedDB is not present, which indeed require us to also clear the data from browser.storage

lightning-browser-extension/src/extension/background-script/db.ts

Line 106 in 5bddad2

} else if (result.allowances && result.allowances.length > 0) {

Some other observations #2435 (comment)
With the above said, if the suggestion has to do with easy maintenance, and if we do the "browser.storage.clear()" then we would have to keep some consistency with wiping the IndexedDB database entirely too? Another idea is to try to do the maintenance from the loadFromStorage function in db.ts which is the code that gets kicked in app initialization time.

@mquasar @taboca thanks for testing and looking deeper into the issue. I think the goal is to delete absolutely everything when resetting the extension. So any and all storage mechanisms (indexedDb, browser.storage, etc) we save data to should be completely cleared.

Ok, I will check then the other aspects of DB that seem to be there, such as "LBE-AVAILABILITY-CHECK" and the ways to clear all the DBs.

[fczuardi]

Thanks for the PR!

As @rolznz noted I think the browser.storage.clear() APIs would be a better fit here as they delete all data and do not need to be adjusted in case there are new tables added in future.

We have just updated the patch with the suggestions. It's much simpler now :) Thanks @rolznz !!

[rolznz]

@fczuardi I made some more simplifications. Could you please review the changes?

[fczuardi]

@fczuardi I made some more simplifications. Could you please review the changes?

@rolznz from your 3 commits, the first 2 LGTM, the third is probably fine as well, I just left a comment asking if your version is equivalent because I couldn't see 2 attributes nullified.

[rolznz]

@reneaaron I think this PR is ready for review now. 💪

[bumi]

looks good to me. @reneaaron have a look and merge.

[reneaaron]

Thanks for the PR @fczuardi! 💪

tACK