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

Add toggle to enable/disable multi account balances fetching #6026

Merged
merged 36 commits into from
Jun 2, 2023

Conversation

Fatxx
Copy link
Contributor

@Fatxx Fatxx commented Mar 23, 2023

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description

1. What is the reason for the change?

As explained in #5321, users have to be able to choose if they want to query for their accounts balance in batch or one account at a time.

2. What is the improvement/solution?

  • add a new setting section with a switch in Security & privacy settings to select between batch balance request or single selected account balance request. Use the provided description.
  • use batch mode by default (default value to true, switch selected)
  • save setting value in state and update UI from state
  • add translation strings
  • add MetaMetricsEvents
    • example events sent:
       INFO  TRACK event saved {"anonymousId": "0x0000000000000000", "event": "Batch account balance requests", "properties": {"batch": "ON"}, "type": "track", "userId": "0x38335acdbca7be5d1a7b10290e37a086d69705aa49f5a03dfd1e3db8ebc91ca8"}
       INFO  TRACK event saved {"anonymousId": "0x0000000000000000", "event": "Batch account balance requests", "properties": {"batch": "OFF"}, "type": "track", "userId": "0x38335acdbca7be5d1a7b10290e37a086d69705aa49f5a03dfd1e3db8ebc91ca8"}
      
  • patch controllers to query only the selected account balance or batch query all accounts according to setting in state
  • add unit test

Screenshots/Recordings

If applicable, add screenshots and/or recordings to visualize the before and after of your change

Wallet Screen Selector: https://recordit.co/1jIXqGlCAg
Dapp Account Selector: https://recordit.co/OxepxVgHa0

image

Issue

Progresses #5321

Testing

See QA builds in Bitrise build #6686

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@NicolasMassart
Copy link
Contributor

Find my changes in a branch you can merge in this PR if it looks fine for you: fix/add-opt-out-toggle-for-batching-balances...nico/5321_add-opt-out-toggle-for-batching-balances

@Fatxx Fatxx linked an issue Mar 29, 2023 that may be closed by this pull request
10 tasks
@Fatxx Fatxx marked this pull request as ready for review March 29, 2023 17:58
@Fatxx Fatxx requested a review from a team as a code owner March 29, 2023 17:58
@Fatxx Fatxx added needs-qa Any New Features that needs a full manual QA prior to being added to a release. needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Mar 29, 2023
@Cal-L Cal-L added the needs-product Needs product manager to look into it label Mar 30, 2023
@cortisiko cortisiko removed the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Mar 30, 2023
@Cal-L Cal-L dismissed stale reviews from NicolasMassart and themself via 8ef7f89 May 15, 2023 18:13
@cortisiko cortisiko added QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels May 15, 2023
Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

@Fatxx sending this back your way:

  1. Translations are missing.

  2. I cannot see analytic events:

  • Locally when I enable and disable “batch account balance requests”
  • In a preproduction version of the app: I had an iOS version of the QA app > I went ahead and toggled “batch account balance requests” to off, but when I searched mix panel the analytics did not appear.

What wasn't tested:
upgrading the app from a previous version to a version containing this PR. Can we kick off a release build for this branch?

@cortisiko cortisiko added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed QA in Progress QA has started on the feature. labels May 15, 2023
@socket-security
Copy link

socket-security bot commented May 16, 2023

No dependency changes detected. Learn more about Socket for GitHub ↗︎


👍 No dependency changes detected in pull request

Pull request alert summary
Issue Status
Critical CVE ✅ 0 issues
CVE ✅ 0 issues
Mild CVE ✅ 0 issues
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script confusion ✅ 0 issues
Bin script shell injection ✅ 0 issues
Filesystem access ✅ 0 issues
Network access ✅ 0 issues
Shell access ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
GitHub dependency ✅ 0 issues
No bug tracker ✅ 0 issues
No contributors or author data ✅ 0 issues
No README ✅ 0 issues
Deprecated ✅ 0 issues
New author ✅ 0 issues
Unstable ownership ✅ 0 issues
Non-existent author ✅ 0 issues
Unmaintained ✅ 0 issues
Unpublished package ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues
AI detected security risk ✅ 0 issues
AI warning ✅ 0 issues

# Conflicts:
#	app/components/UI/AccountInfoCard/__snapshots__/index.test.tsx.snap
#	app/components/UI/AccountInfoCard/index.test.tsx
#	app/components/UI/AnimatedTransactionModal/index.test.tsx
#	locales/languages/en.json
#	wdio/screen-objects/testIDs/Components/AccountListComponent.testIds.js
#	wdio/step-definitions/import-wallet-via-private-key.steps.js
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

Left a small comment

@NicolasMassart
Copy link
Contributor

@Fatxx sending this back your way:

  1. Translations are missing.
  2. I cannot see analytic events:
  • Locally when I enable and disable “batch account balance requests”
  • In a preproduction version of the app: I had an iOS version of the QA app > I went ahead and toggled “batch account balance requests” to off, but when I searched mix panel the analytics did not appear.

What wasn't tested: upgrading the app from a previous version to a version containing this PR. Can we kick off a release build for this branch?

#5321 (comment)

@NicolasMassart NicolasMassart self-assigned this Jun 1, 2023
@NicolasMassart NicolasMassart added QA in Progress QA has started on the feature. and removed QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed labels Jun 1, 2023
Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarcloud
Copy link

sonarcloud bot commented Jun 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

50.0% 50.0% Coverage
0.0% 0.0% Duplication

@NicolasMassart NicolasMassart dismissed cortisiko’s stale review June 2, 2023 11:25

We decided not to handle the requested change in this PR

@NicolasMassart NicolasMassart merged commit 4f2ab89 into main Jun 2, 2023
11 checks passed
@NicolasMassart NicolasMassart deleted the fix/add-opt-out-toggle-for-batching-balances branch June 2, 2023 11:27
@github-actions github-actions bot locked and limited conversation to collaborators Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Priority - High Task with high priority QA in Progress QA has started on the feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add toggle to opt-out of batching balance requests for multiple accounts
5 participants