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.

Sign up for GitHub

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

Jump to bottom

feat(wallet): partially persist redux stores #17631

Closed
wants to merge 1 commit into from
Closed

feat(wallet): partially persist redux stores #17631

wants to merge 1 commit into from

Conversation

josheleonard
Copy link
Contributor

@josheleonard josheleonard commented Mar 15, 2023 •
edited
Loading

Resolves brave/brave-browser#29101

Security review: https://github.com/brave/security/issues/1220

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Scenario 1 - Clear storage via wallet reset

  1. Create a wallet
  2. unlock the wallet
  3. open the wallet page
  4. refresh the Wallet Page
  5. open DevTools
  6. Verify that partial application state is saved to local-storage
  7. Press ctrl+r (or CMD+R on MacOS) to force-refresh the page
  8. Verify that the persisted local-storage state was cleared/reset
  9. Repeat steps 1-7, but test the Panel via brave://wallet-panel.top-chrome/ instead of the Page

Scenario 2 - Clear storage via hard-refresh

  1. Create a wallet
  2. unlock the wallet
  3. open the wallet page
  4. refresh the Wallet Page
  5. open DevTools
  6. Verify that partial application state is saved to local-storage
  7. open brave://settings/web3
  8. Click "Reset Brave Wallet"
  9. Type "Yes" and click OK
  10. Open the wallet page
  11. Verify that the persisted local-storage state was cleared/reset
  12. Repeat steps 2-11, but test the Panel via brave://wallet-panel.top-chrome/ instead of the Page

@josheleonard josheleonard added security dependencies Pull requests that update a dependency file labels Mar 15, 2023
@josheleonard josheleonard self-assigned this Mar 15, 2023
@github-actions github-actions bot added CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) CI/storybook-url Deploy storybook and provide a unique URL for each build feature/web3/wallet labels Mar 15, 2023
@josheleonard josheleonard force-pushed the feat-persist-wallet-redux-store branch from 6f04c43 to 7cf0dfb Compare March 15, 2023 22:18
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@josheleonard josheleonard mentioned this pull request Mar 17, 2023
Closed
kdenhartog
kdenhartog reviewed Mar 17, 2023
View reviewed changes
package.json Outdated Show resolved Hide resolved
components/brave_wallet_ui/page/wallet_page.tsx Outdated Show resolved Hide resolved
components/brave_wallet_ui/panel/reducers/panel_reducer.ts Outdated Show resolved Hide resolved
components/brave_wallet_ui/common/slices/wallet.slice.ts Outdated Show resolved Hide resolved
components/brave_wallet_ui/common/slices/api.slice.ts Outdated Show resolved Hide resolved
@josheleonard josheleonard force-pushed the feat-persist-wallet-redux-store branch from 7cf0dfb to 7feee26 Compare March 20, 2023 15:47
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

diracdeltas
diracdeltas reviewed Mar 20, 2023
View reviewed changes
package.json
@@ -379,6 +379,7 @@
"react-window": "^1.8.5",
"redux-act": "^1.8.0",
"redux-logger": "^3.0.6",
"redux-persist": "6.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

are there any alternative libraries that would work here which are more actively maintained?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the most actively maintained library that can persist subsets of state without additional dependencies.

https://github.com/react-stack/redux-storage#installation is also recommended in the redux docs, but that requires additional packages that haven't been touched in a while either (https://github.com/react-stack/redux-storage-engine-localstorage, https://github.com/react-stack/redux-storage-merger-simple, https://github.com/react-stack/redux-storage-decorator-filter, https://github.com/mathieudutour/redux-storage-decorator-migrate).

@josheleonard josheleonard force-pushed the feat-persist-wallet-redux-store branch from 4654b4d to d3c21d2 Compare March 21, 2023 15:51
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@josheleonard josheleonard force-pushed the feat-persist-wallet-redux-store branch 3 times, most recently from 38411dd to 562fc43 Compare March 21, 2023 18:04
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@josheleonard
Copy link
Contributor Author

Noting here that if we want to store the persisted data in a location other than local-storage, a custom interface from core can be utilized as long as it implements setItem getItem removeItem similar to the local-storage api.

@josheleonard josheleonard force-pushed the feat-persist-wallet-redux-store branch from 562fc43 to 318cdf5 Compare March 22, 2023 16:13
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@josheleonard josheleonard force-pushed the feat-persist-wallet-redux-store branch from 318cdf5 to b779486 Compare March 29, 2023 15:40
@socket-security
Copy link

socket-security bot commented Mar 29, 2023 •
edited
Loading

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@josheleonard josheleonard marked this pull request as ready for review March 29, 2023 16:29
@josheleonard josheleonard force-pushed the feat-persist-wallet-redux-store branch from 8d815da to 934a5b7 Compare June 7, 2023 20:21
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@josheleonard josheleonard force-pushed the feat-persist-wallet-redux-store branch from 934a5b7 to 7e5a99e Compare June 9, 2023 13:43
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@josheleonard josheleonard force-pushed the feat-persist-wallet-redux-store branch 2 times, most recently from 0c88789 to f9cb767 Compare June 13, 2023 14:51
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@josheleonard josheleonard force-pushed the feat-persist-wallet-redux-store branch 2 times, most recently from 2140feb to d13c1cb Compare June 14, 2023 14:18
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@josheleonard josheleonard force-pushed the feat-persist-wallet-redux-store branch from d13c1cb to 7bd6d6b Compare June 15, 2023 13:29
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

onyb
onyb reviewed Jun 16, 2023
View reviewed changes
components/brave_wallet_ui/page/wallet_page.tsx Outdated
const onPageRefreshInitiated = function (
event: KeyboardEvent
): void {
if ((event.ctrlKey || event.metaKey) && event.code === 'KeyR') {
Copy link
Member

Choose a reason for hiding this comment

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

Discussed internally too, but we probably want to purge for Shift + Cmd + R.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jamesmudgett , can we also get some designs to add a button somewhere to clear any persisted settings without resetting the entire wallet from settings?

@josheleonard josheleonard force-pushed the feat-persist-wallet-redux-store branch from 7bd6d6b to 7b1f55f Compare June 16, 2023 15:09
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

1 similar comment
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@josheleonard josheleonard force-pushed the feat-persist-wallet-redux-store branch from 7b1f55f to 8bcfab9 Compare June 20, 2023 16:29
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@josheleonard josheleonard force-pushed the feat-persist-wallet-redux-store branch 2 times, most recently from 5f35ce0 to ef07a5b Compare June 21, 2023 16:02
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@josheleonard josheleonard force-pushed the feat-persist-wallet-redux-store branch from ef07a5b to 8656afe Compare June 26, 2023 20:04
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@josheleonard josheleonard force-pushed the feat-persist-wallet-redux-store branch from 8656afe to 0910963 Compare July 3, 2023 21:45
@josheleonard josheleonard force-pushed the feat-persist-wallet-redux-store branch from 0910963 to 66996f7 Compare July 3, 2023 22:23
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

@josheleonard
Copy link
Contributor Author

Closing since partial persistence of the api-slice was not working.
Might re-attempt later without redux-persist and with a way for the api base cache to persist some of it's data to local-storage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@onyb onyb onyb left review comments

@diracdeltas diracdeltas diracdeltas approved these changes

@kdenhartog kdenhartog kdenhartog approved these changes

Assignees

@josheleonard josheleonard

Labels
CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) CI/storybook-url Deploy storybook and provide a unique URL for each build dependencies Pull requests that update a dependency file feature/web3/wallet security
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Persist a subset of the Wallet redux store state in localStorage
6 participants
@josheleonard @brave-builds @muliswilliam @kdenhartog @diracdeltas @onyb