Skip to content
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

Store remembered device, discovery, accounts, txs in db #713

Merged
merged 50 commits into from
Nov 6, 2019

Conversation

slowbackspace
Copy link
Contributor

@slowbackspace slowbackspace commented Oct 22, 2019

fix #745
part of #8 (web/electron covered)

  • bumped node to v12
  • fixed failing tests under node 12
  • enabled es2019 syntax
  • misc fixes

@slowbackspace slowbackspace changed the title Store remembered device, discovery, accounts in db Store remembered device, discovery, accounts, txs in db Oct 24, 2019
@slowbackspace slowbackspace marked this pull request as ready for review October 24, 2019 14:04
@slowbackspace slowbackspace added READY FOR REVIEW Developed pull request ready to be reviewed by another developer and removed in progress labels Nov 1, 2019
@@ -251,6 +289,26 @@ class CommonDB<TDBStructure> {
const items = await tx.store.getAll();
return items;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return tx.store.getAll() ?

Copy link
Contributor

@szymonlesisz szymonlesisz Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and the same in line 285 and 220

@@ -290,6 +328,26 @@ class CommonDB<TDBStructure> {
const items = await tx.store.getAll();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as native/index:
return tx.store.getAll() ?

newVersion: number | null,
transaction: any
) => void;
db: OnUpgradeProps<TDBStructure>['db'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't be cleaner to declare this function once?
Since you are using it here and then the same declaration in constructor?

case DISCOVERY.STOP:
case DISCOVERY.COMPLETE: {
// TODO: explore better way to update discovery only
const device = api.getState().devices.find(d => d.state === action.payload.deviceState);
Copy link
Contributor

@szymonlesisz szymonlesisz Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use getState().suite.device instead, discovery is designed to run only on currently selected device

Copy link
Contributor

@szymonlesisz szymonlesisz Nov 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait :) i don't get why you are saving device instead of discovery?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rememberDevice should be triggered from SUITE.REMEMBER_DEVICE, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • additionaly device should be updated after DEVICE.UPDATE as well, for example changing label/FW/pin/passphrase settings

@slowbackspace slowbackspace added in progress and removed READY FOR REVIEW Developed pull request ready to be reviewed by another developer labels Nov 5, 2019
@slowbackspace slowbackspace added READY FOR REVIEW Developed pull request ready to be reviewed by another developer and removed in progress labels Nov 5, 2019
@vladimirvolek vladimirvolek merged commit 1bc7122 into develop Nov 6, 2019
@vladimirvolek vladimirvolek deleted the remember-device-storage branch November 7, 2019 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
READY FOR REVIEW Developed pull request ready to be reviewed by another developer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unit tests failing with Node.js v11+
3 participants