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

4byte fallback #6551

Merged
merged 7 commits into from
Jun 18, 2019
Merged

4byte fallback #6551

merged 7 commits into from
Jun 18, 2019

Conversation

whymarrh
Copy link
Contributor

@whymarrh whymarrh commented May 1, 2019

This PR re-introduces the 4byte fallback for getMethodData, originally introduced in #6435.

We'll want to improve how this behaves before landing it on develop again, namely:

Note: this feature branch will see the above changes made before it is marked as ready for review.

This PR is ready for review.

@whymarrh whymarrh requested review from danjm and tmashuang May 1, 2019 16:41
@metamaskbot
Copy link
Collaborator

Builds ready [5118055]: chrome, firefox, edge, opera

@metamaskbot
Copy link
Collaborator

Builds ready [3adf956]: chrome, firefox, edge, opera

@metamaskbot
Copy link
Collaborator

Builds ready [891fbb7]: chrome, firefox, edge, opera

@metamaskbot
Copy link
Collaborator

Builds ready [da599c9]: chrome, firefox, edge, opera

@danjm danjm added this to the UI Sprint 11 [April 29] milestone May 8, 2019
@danjm danjm force-pushed the 4byte-fallback branch from da599c9 to ea67ca9 Compare May 8, 2019 19:38
@danjm
Copy link
Contributor

danjm commented May 8, 2019

Just rebased onto latest develop

@metamaskbot
Copy link
Collaborator

Builds ready [ea67ca9]: chrome, firefox, edge, opera

@bdresser
Copy link
Contributor

bdresser commented May 9, 2019

Even with caching, this will send a lot of traffic to 4byte - @danfinlay can you check with the maintainer?

Chatted with Infura and they are not keen to host a version of the API simply for bandwidth reasons

@danjm
Copy link
Contributor

danjm commented May 22, 2019

Rebased onto developer

@metamaskbot
Copy link
Collaborator

Builds ready [a3475ee]: chrome, firefox, edge, opera

@whymarrh whymarrh marked this pull request as ready for review May 28, 2019 15:32
@danjm
Copy link
Contributor

danjm commented May 28, 2019

  • confirm that number of requests to 4byte will now be equal to the sum of each users total unique contract method confirmations (i.e. much less than the total number of transactions a day)

@metamaskbot
Copy link
Collaborator

Builds ready [cfa916a]: chrome, firefox, edge, opera

danjm and others added 6 commits June 17, 2019 08:45
* Remove async call from getTransactionActionKey()

* Stop blocking confirm screen rendering on method data loading, and base screen route on transactionCategory

* Remove use of withMethodData, fix use of knownMethodData, in relation to transaction-list-item.component

* Load data contract method data progressively, making it non-blocking; requires simplifying conf-tx-base lifecycle logic.

* Allow editing of gas price while loading on the confirm screen.

* Fix transactionAction component and its unit tests.

* Fix confirm transaction components for cases of route transitions within metamask.

* Only call toString on id if truthy in getNavigateTxData()
@danjm
Copy link
Contributor

danjm commented Jun 17, 2019

Rebased onto develop

@metamaskbot
Copy link
Collaborator

Builds ready [919b784]: chrome, firefox, edge, opera

@metamaskbot
Copy link
Collaborator

Builds ready [91be4a9]: chrome, firefox, edge, opera

@whymarrh
Copy link
Contributor Author

From @danjm above, emphasis mine:[1]

Confirm that number of requests to 4byte will now be equal to the sum of each users total unique contract method confirmations (i.e. much less than the total number of transactions a day)

Version 6.4.0

6.4.0 was the first version we shipped the 4byte fallback in. It made two lookup requests for each transaction in the transaction list and two lookup requests on the confirm screen. A network waterfall for 4 txs in the list:

You can notice that the two requests are dispatched at the same time (i.e., they're not staggered).

It's also worth re-affirming that the old implementation handled failures for both requests—both requests failing individually and requests failing together:

With this PR

This PR adjusts how we dispatch the requests to reduce the requests made to 4byte. We now no longer make two requests per tx in the tx list. In fact, we now need no requests to render the tx list and make only two requests for each tx confirmation.

Copy link
Contributor Author

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

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

I feel confident that this makes significantly fewer requests than the previous implementation

@chikeichan @danjm can one of y'all give this a ✅

@danjm danjm merged commit 748801f into develop Jun 18, 2019
@whymarrh whymarrh deleted the 4byte-fallback branch June 18, 2019 20:03
Gudahtt added a commit that referenced this pull request Jul 22, 2020
This higher-order-component has been unused since #6551
Gudahtt added a commit that referenced this pull request Jul 22, 2020
This higher-order-component has been unused since #6551
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.

4 participants