fix: safeguard utill/address functions for undefined arguments

[jpcloureiro]

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 the appropiate QA label when dev review is completed
   • needs-qa: PR requires manual QA.
   • No QA/E2E only: PR does not require any manual QA effort. Prior to merging, ensure that you have successful end-to-end test runs in Bitrise.
   • Spot check on release build: PR does not require feature QA but needs non-automated verification. In the description section, provide test scenarios. Add screenshots, and or recordings of what was tested.
5. Add QA Passed label when QA has signed off (Only required if the PR was labeled with needs-qa)
6. Add your team's label, i.e. label starting with team- (or external-contributor label if your not a MetaMask employee)

Description

This PR aims at preventing the appearance of the ErrorBoundary screen on a specific user flow that seems to be the most affected by this bug

The user flow we're touching is the navigation to the Transaction screen while the wallet has a non-empty list of transactions to be displayed.

`TypeError: undefined is not an object(evaluating e.toLowerCase)`  (Under the error boundary screen "ActivityView"). 

is the error we've been seeing the most.

This fix only implements an access check on a couple of util functions inside app/util/address that are being used in the Transaction screen components

Issue

fixes #6771

Checklist

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

[github-actions]

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.

[codecov-commenter]

Codecov Report

Patch coverage: 36.36% and project coverage change: +0.06% 🎉

Comparison is base (df4b055) 33.08% compared to head (b0d2e6c) 33.15%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7056      +/-   ##
==========================================
+ Coverage   33.08%   33.15%   +0.06%     
==========================================
  Files        1005     1005              
  Lines       32649    32654       +5     
  Branches     8395     8398       +3     
==========================================
+ Hits        10803    10827      +24     
+ Misses      21846    21827      -19     

Files Changed	Coverage Δ
...mponents/Views/EditAccountName/EditAccountName.tsx	80.00% <0.00%> (-4.22%)	⬇️
app/util/address/index.js	52.17% <100.00%> (+12.39%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tommasini]

The getAddressAccountType is used only for analytics purposes, just want to do the reference that the property account_type will be false when the address is undefined on the following events:

• Sign Request Started
• Sign Request Completed
• Sign Request Cancelled
• Send Transaction Started
• Account Renamed
• Contract Address Copied
• Connect Request Started

Do you confirm @jpcloureiro ?

[tommasini]

LGTM!

[jpcloureiro]

The getAddressAccountType is used only for analytics purposes, just want to do the reference that the property account_type will be false when the address is undefined on the following events:

• Sign Request Started
• Sign Request Completed
• Sign Request Cancelled
• Send Transaction Started
• Account Renamed
• Contract Address Copied
• Connect Request Started

Do you confirm @jpcloureiro ?

Good catch.

That function shouldn't return a boolean.

I'll make the function throw an error if all calls happen to be wrapped inside a try / catch block

[jpcloureiro]

getAddressAccountType throws an error when address is undefined in 24308d6

[NicolasMassart]

Q: not sure why you don't cascade the checks in the functions.

Right now, each address check function has to do similar checks (defined, length, valid hex, regex,...).

Why not just one that checks the definition, then the format?
In the end, all of the functions that use an address seems to require a fully valid hex ethereum address. So I feel that one "isValidEthAddress" that does a definition check and a regex match would be enough to be called from all other places.
It would also reduce the number of places where you do checks and make code stronger and easier to understand to me.

[jpcloureiro]

Q: not sure why you don't cascade the checks in the functions.

Right now, each address check function has to do similar checks (defined, length, valid hex, regex,...).

Why not just one that checks the definition, then the format? In the end, all of the functions that use an address seems to require a fully valid hex ethereum address. So I feel that one "isValidEthAddress" that does a definition check and a regex match would be enough to be called from all other places. It would also reduce the number of places where you do checks and make code stronger and easier to understand to me.

We should be using isValidAddress from @ethereumjs/util to validate the selectedAddress value.

This validation should occur at a root level rather than in the multiple places where it is being used.
We should also be controlling more user flows based on the validity of selectedAddress value.

With that in mind, I prefer to leave those changes outside this PR for the sake of keeping the scope close to the issue

[NicolasMassart]

Looks good to me.

[tommasini]

LGTM!

[MarioAslau]

Looks good!

[sethkfman]

E2E Testing: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/2226c24f-d843-414d-8ca2-b1fd09575d08

[Cal-L]

Left some comments - Do we know the root of why selectedAddress would be undefined?

[hesterbruikman]

Test recording as shared on Slack Sep 4

[jpcloureiro]

Left some comments - Do we know the root of why selectedAddress would be undefined?

Bad redux hydration is one of them

[Cal-L]

Left some comments - Do we know the root of why selectedAddress would be undefined?

Bad redux hydration is one of them

Is there a solution for fixing the root cause (fixing the bad rehydration condition)? Currently, the patch still results in the app showing potentially incorrect states since selectedAddress can still be undefined.

[jpcloureiro]

Hey, I've updated the PR description to provide more context around this approach.

The motivation for this PR relies on preventing a specific error that has been reported multiple times. All of them appear to occur on the transaction screen.

The approach is to allow specific functions to not throw errors when their arguments are undefined rather than preventing the selectedAddress from becoming undefined.

Efforts on preventing selectedAddress from becoming undefined could be tracked on this issue

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

0.0% Coverage
0.0% Duplication

[cortisiko]

I do not see anything out of the ordinary to report here. ✅