-
Notifications
You must be signed in to change notification settings - Fork 5k
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
fix: selector getKnownMethodData should return empty object if user has opted out for using 4Byte Resolution #27203
Conversation
…ted out for using 4Byte Resolution
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. |
b47f50b
@@ -34,7 +34,7 @@ describe('useFourByte', () => { | |||
expect(result.current.params).toEqual([]); | |||
}); | |||
|
|||
it('returns undefined if resolution is turned off', () => { | |||
it('returns rmpty object if resolution is turned off', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, typo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
ui/selectors/selectors.js
Outdated
@@ -1262,7 +1262,7 @@ export function getKnownMethodData(state, data) { | |||
const fourBytePrefix = prefixedData.slice(0, 10); | |||
const { knownMethodData, use4ByteResolution } = state.metamask; | |||
// If 4byte setting is off, we do not want to return the knownMethodData | |||
return use4ByteResolution ? knownMethodData?.[fourBytePrefix] : undefined; | |||
return use4ByteResolution ? knownMethodData?.[fourBytePrefix] : {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate this is the quicker fix for resolution, but for audit sake it's less explicit since the caller has to check the individual properties to determine if there is actual method data present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. It would be nice to create a follow-up refactor to handle when no methodData is found with null or undefined instead of {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing to have in mind though is that currently you can have an empty object in the state. In the state file shared by CS in the bug thread, you can see entries in the KnownMethodData like this:
"0xf305d719": {},
This means that even if use4ByteResolution
is true, and there's an entry for the fourBytePrefix
we might still get an empty object, and the caller still will have to check individual properties to determine if there's actual data present.
The only thing that I would flag in the above fix, is that it is still possible to get undefined
if there's no entry for the fourBytePrefix
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch @cryptotavares , I changed it to
return use4ByteResolution ? knownMethodData?.[fourBytePrefix] ?? {} : {};
Builds ready [b47f50b]
Page Load Metrics (1713 ± 126 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #27203 +/- ##
========================================
Coverage 70.00% 70.01%
========================================
Files 1445 1445
Lines 50195 50194 -1
Branches 14041 14041
========================================
Hits 35139 35139
+ Misses 15056 15055 -1 ☔ View full report in Codecov by Sentry. |
ui/selectors/selectors.js
Outdated
@@ -1262,7 +1262,7 @@ export function getKnownMethodData(state, data) { | |||
const fourBytePrefix = prefixedData.slice(0, 10); | |||
const { knownMethodData, use4ByteResolution } = state.metamask; | |||
// If 4byte setting is off, we do not want to return the knownMethodData | |||
return use4ByteResolution ? knownMethodData?.[fourBytePrefix] : undefined; | |||
return use4ByteResolution ? knownMethodData?.[fourBytePrefix] : {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for consistency, at the start of the method we could now return {} instead of null as well.
if (!data) {
return null;
}
const { name } = null;
// VM334:1 Uncaught TypeError: Cannot destructure property 'name' of 'null' as it is null.
at <anonymous>:1:9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@digiwand since this is hotfix, I will prefer to keep scope minimum. More detailed changes can go in a followup PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting to update the !data return value too in case this may also cause similar destructing issues
Quality Gate passedIssues Measures |
Builds ready [4450a71]
Page Load Metrics (1746 ± 110 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Description
If user has toggled off Decode smart contracts setting he is not able to approve ERC20. This is regression introduced recently.
Related issues
Fixes: #27188
Manual testing steps
Screenshots/Recordings
Screen.Recording.2024-09-17.at.2.20.17.PM.mov
Pre-merge author checklist
Pre-merge reviewer checklist