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

chore: metadata refactoring 4 #9560

Merged
merged 19 commits into from
Oct 19, 2023
Merged

chore: metadata refactoring 4 #9560

merged 19 commits into from
Oct 19, 2023

Conversation

mroz22
Copy link
Contributor

@mroz22 mroz22 commented Sep 28, 2023

In this PR I am separating code from #9130 which is not features. so basically everything from #9130 except for confirm-less labeling and migration was moved to this PR.

While working on #9130 I noticed couple of problems which might have been around for long time or might have been introduced in one of the previous requests on the were merged along the way to the confirm-less labeling feature (#9108, #9218, #9262, #9270)

commits:

  • 853ee53
    • labels have not been stored anymore into indexxed db for some time now and are instead loaded when provider becomes available. This commit fixes bug when labels were loaded only for selected device.
    • added e2e test to validate this
    • fixed paths of files in mocked providers in e2e tests.
  • 7cc08f0
    • unifies unique id for labelable entities. Previously there was account.key and device.state and it was necessary to use in operator frequently. Now we are using everywhere entity.key abstraction narrowing defined in a newly added selector.
  • 051e504
    • removes unused constants and export
  • 9d88de1
    • moves code from metadataActions getLabelableEntities which was actually a selector to metadataReducer.
  • 9f91aef
    • micro change. remove provider by clientId instead of type. type would work as well for now but we want to be ready for future.
  • 298fca7
    • removes field device.metadata.status ('disabled' || 'cancelled' || 'enabled'). This is mostyl types simplification. Previously, metadata[encryptionVersion]... was bound with metadata.status.enabled and it was needed to use in operator in my opinion superfluously.
    • cancelled status is replaced by newly added metadata.error which is basically metadata.failedMigration field introduced in metadata encryption v2 (confirm-less labeling) #9130. When error is set, suite does not try to init metadata automatically.
  • 2fc852c
    • updates unit tests, there were some antipatterns, for example mocking happend inside single "tests/its" meaning that running some of the tests in isolation did not work
  • f59a696
    • just use selector in SettingsGeneral component.
  • cb4e79d
    • in metadataActions we need to be careful about for which device (passphrase) selected action runs. Previously, we only worked with selected device but it was not right. It was actually possible to have one device selected and trigger metadata init action for another device when trying to label another wallet in wallet modal selection.
  • 4f1951f
    • revision of return types in metadataActions. in general, the more atomic the action is, the less sideefects it should have. example: encryptAndSaveMetadata action and fetchMetadata actions should never handle their errors and only return results to their caller. The reason is that caller knows whether these actions are run in batch and is in better position to handle errors. In other words - if error is handled in an atomic action we might fire dozens of notifications in case of error which we obviously don't want to do.
  • e82a6ad
    • removing device.status 'cancelled' actually changes behaviour of app. we need to make a note that user cancelled labeling on device so that we know that in the next run we don't want to trigger this dialogue again. Also this field will be reused in metadata encryption v2 (confirm-less labeling) #9130 where I named it metadata.failedMigration but I believe that naming in this PR metadata.error is actually better.
    • more about it in e2e test added in later commit 8c5029c
    • this commit does not make that field persitent yet, it is added in 8c5029c
  • 073e40e
  • b86a5e0
    • e2e test to make sure that newly added account has keys added too (this is handled in metadata middleware)
  • a830893
    • revision of selectors
  • 8c5029c
    • addition to e82a6ad making metadata.error, while not leaking info about number of used passphrases. This information follows fate of device (remember/forget)
    • I believe that decoupling this data (device.status => metadat.error) is benefitial, but our current implementation does not have any good way how to work with relational data unfortunately :( we should investigate something like https://redux-orm.github.io/redux-orm/basics/reducers.
    • this commit also adds test that describe how suite and (un)remembered devices behave in relation to labeling
  • d7f4cd5

Screamshots

Trying to enable labeling for other than currently selected device should work

image

When there are more saved wallets, all should load their labels after app reload. not only the selected one.

image

@mroz22 mroz22 changed the title [wip] chore: metadata refactoring 5 [wip] chore: metadata refactoring 4 Sep 28, 2023
@mroz22 mroz22 force-pushed the metadata-fixes branch 10 times, most recently from 1ebab0e to 8c5029c Compare October 1, 2023 19:47
@mroz22 mroz22 changed the title [wip] chore: metadata refactoring 4 chore: metadata refactoring 4 Oct 2, 2023
@mroz22 mroz22 marked this pull request as ready for review October 2, 2023 09:47
@mroz22 mroz22 force-pushed the metadata-fixes branch 2 times, most recently from 94f0f29 to 64c0a3c Compare October 9, 2023 09:13
@mroz22 mroz22 force-pushed the metadata-fixes branch 2 times, most recently from c87651f to ce866ba Compare October 11, 2023 10:58
@@ -213,6 +214,8 @@ export const rememberDevice =
if (!(await db.isAccessible())) return;
if (!device || !device.features || !device.state) return;
if (!remember) {
// eslint-disable-next-line @typescript-eslint/no-use-before-define
dispatch(forgetDeviceMetadataError(device));
Copy link
Member

Choose a reason for hiding this comment

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

nitpick - why not just move it up or down? 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sometimes it is not possible. not sure how about this case. here I guess motivation might have been not making too many changes for review. I can try to move it as it is reviewed now.

Copy link
Member

@matejkriz matejkriz left a comment

Choose a reason for hiding this comment

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

I'm sorry for the nitpicks, feel free to close them directly or even ignore them. In general it looks great and works so far very smooth 🎉

Copy link
Member

@matejkriz matejkriz left a comment

Choose a reason for hiding this comment

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

🎉 Let's rebase it

@mroz22 mroz22 merged commit 6aad115 into develop Oct 19, 2023
31 of 32 checks passed
@mroz22 mroz22 deleted the metadata-fixes branch October 19, 2023 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants