-
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
Additional incoming transactions support #14219
Conversation
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. |
I have read the CLA Document and I hereby sign the CLA |
@PeterYinusa did you have a chance to look at this? |
@PeterYinusa is there any additional work to do on my side to get it reviewed? Thank you. |
@tgmichel sorry we didn't review this yet, looks like a pretty amazing contribution that I'm sure our users would love. Could you do me a favor and rebase this? One area that has changed dramatically is the network.js constants file which is now typescript so you'll have to introduce some types to your changes. Let me know if you need assistance and tag me for review when you're ready :) |
# Conflicts: # app/scripts/controllers/incoming-transactions.js # app/scripts/controllers/incoming-transactions.test.js # shared/constants/network.js
@brad-decker Thank you, very excited to finally get this in :) and sorry it took a while, just rebased. |
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.
Awesome work, we'll get more eyes on this next week for an additional approval.
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.
This LGTM, @seaona / @PeterYinusa I've tested this and it looks good locally, but i might miss some edge cases. Would either of you take a look from the QA side? @jpuri and/or @segun if you have a chance to do a code review that'd be great.
@brad-decker I can take a look |
From QA looks good! @brad-decker @tgmichel ! I've only noticed one small behaviour on Polygon Testnet. Bascially, until I don't refresh the browser page I don't see any tx coming through. Not sure why I can only observe this on Polygon Testnet. Any ideas? Just curious. I would still approve the PR from QA side, since after refreshing, the tx appear. mumbai.mp4 |
Thats peculiar. @seaona lets create a ticket to investigate that and i'll get another set of eyes on this. |
@brad-decker the issue @seaona mentioned doesn't seem to originate in this PR, is there something I can do to help you unlock it? The logic for triggering
If the
I'm thinking maybe after refreshing, the |
Hello @tgmichel Thanks for your PR ! |
The current controller for incoming transactions leverages Etherscan Api, which is not available for Ganache afaik. |
@tgmichel Can i add a bounty to get this expedited? When will incoming transactions be merged? The community has been waiting far too long for such an essential feature to be added |
@brad-decker sorry to come back at you again regarding this PR, please let me know when is ready to be merged so I can rebase once. Thank you! |
@tgmichel ready for merge, @pedronfigueiredo and I will get this merged once the rebase occurs |
# Conflicts: # app/scripts/controllers/incoming-transactions.js
14d3b8c
@brad-decker done |
@pedronfigueiredo when you come online please review and merge :D |
Currently only the built-in Infura networks - and not EVM compatible networks where the official Etherscan API is available - support displaying incoming transactions.
This PR proposes introducing additional support for them: