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

Check four byte prefix object is empty #8836

Merged
merged 1 commit into from
Jun 23, 2020
Merged

Conversation

tmashuang
Copy link
Contributor

@tmashuang tmashuang commented Jun 19, 2020

Fixes #8835

In cases where the initial registry failed to load, and the sig is set to {} on this line:

this proceeds to set the four byte method to {} in knownMethodData.

Additionally check if the method four byte prefix object is empty to proceed call getMethodDataAsync again.

I could only reproduce by intentionally failing the method registry lookup and found this solution. I could not find an instance where the registry consistently failed to lookup even on slow/throttled/high latency networks.

…s empty

Fixes #8835

In cases where the registry failed to load, and the sig is set to `{}` on this line: https://github.com/MetaMask/metamask-extension/blob/e85b162651e887d79bfd15469289abc2c6753cbc/ui/app/helpers/utils/transactions.util.js#L78 this proceeds to set the method prefix to `{}` in knownMethodData.

Additionally check if the method prefix object is empty to proceed call getMethodDataAsync again.

I could only reproduce by intentionally failing the method registry lookup and found this solution. I could not find an instance where the registry consistently failed to lookup even on slow/throttled/high latency networks.
@tmashuang tmashuang requested a review from a team as a code owner June 19, 2020 04:20
@metamaskbot
Copy link
Collaborator

Builds ready [05ecda8]
Page Load Metrics (716 ± 43 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint3494592412
domContentLoaded3988367149043
load4008387169043
domInteractive3988367149043

@tmashuang tmashuang changed the title Check knownMethodData[fourBytePrefix] object is empty Check four byte prefix object is empty Jun 22, 2020
Copy link
Contributor

@brad-decker brad-decker left a comment

Choose a reason for hiding this comment

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

This change makes sense to me, but I don't have a lot of context with this API so I'm not going to mark approved. LGTM though!

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Interesting. So the blank entry gets cached and referred to later, preventing additional lookup attempts.

This would all probably be a lot simpler if that {} was never returned from getMethodDataAsync, but we can clean that up later. Changing that would be a bit more difficult.

LGTM!

@tmashuang tmashuang merged commit 4354e9e into develop Jun 23, 2020
@tmashuang tmashuang deleted the contract-method-check branch June 23, 2020 04:30
Gudahtt added a commit that referenced this pull request Jun 23, 2020
* origin/develop:
  Fix signing method bugs (#8833)
  replace icons with Checkbox component (#8830)
  Use [email protected] (#8845)
  Use [email protected] (#8844)
  Call getMethodDataAsync when knownMethodData[fourBytePrefix] object is empty (#8836)
  Update connected status popover content (#8834)
  Use @metamask/[email protected] (#8832)
  ParseInt nextworkNextNonce correction (#8827)
  Fix first time onboarding popup position (#8829)
  fix overflowing contract names and origins (#8823)
  Hide 'Expand view' button in fullscreen (#8826)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metamask fails to parse function signatures, shows contract interaction
4 participants