metamask-extension icon indicating copy to clipboard operation
metamask-extension copied to clipboard

Additional incoming transactions support

Open tgmichel opened this issue 2 years ago • 5 comments

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:

  • BNB Smart Chain - mainnet, testnet - view
  • Optimism - mainnet, testnet - view
  • Polygon - mainnet, testnet - view
  • Avalanche - mainnet, testnet - view
  • Fantom - mainnet, testnet - view
  • Moonbeam - mainnet, testnet - view
  • Moonriver - view

tgmichel avatar Mar 28 '22 08:03 tgmichel

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.

github-actions[bot] avatar Mar 28 '22 08:03 github-actions[bot]

I have read the CLA Document and I hereby sign the CLA

tgmichel avatar Mar 28 '22 08:03 tgmichel

@PeterYinusa did you have a chance to look at this?

tgmichel avatar Apr 05 '22 12:04 tgmichel

@PeterYinusa is there any additional work to do on my side to get it reviewed? Thank you.

tgmichel avatar Apr 26 '22 09:04 tgmichel

@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 :)

brad-decker avatar Sep 20 '22 15:09 brad-decker

@brad-decker Thank you, very excited to finally get this in :) and sorry it took a while, just rebased.

tgmichel avatar Oct 14 '22 09:10 tgmichel

@brad-decker I can take a look

seaona avatar Nov 10 '22 12:11 seaona

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.

https://user-images.githubusercontent.com/54408225/201342105-29193b1b-1517-4abb-a0d4-858e9e1db83b.mp4

seaona avatar Nov 11 '22 12:11 seaona

Thats peculiar. @seaona lets create a ticket to investigate that and i'll get another set of eyes on this.

brad-decker avatar Nov 11 '22 14:11 brad-decker

@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 _onLatestBlock and updating incomingTxLastFetchedBlockByChainId LGTM. I didn't debug it but it might be worth looking at getSelectedAddress here:

this._onLatestBlock = async (newBlockNumberHex) => {
    const selectedAddress = this.preferencesController.getSelectedAddress();
    const newBlockNumberDec = parseInt(newBlockNumberHex, 16);
    await this._update(selectedAddress, newBlockNumberDec);
};

If the selectedAddress is not properly retrieved from preferencesController, the _update function will early return:

if (
    !Object.hasOwnProperty.call(ETHERSCAN_SUPPORTED_NETWORKS, chainId) ||
    !address
) {
    return;
}

I'm thinking maybe after refreshing, the selectedAddress is correctly picked up and works as expected. Anyway this is just a wild guess because if that would happen shouldn't be something network-specific (and apparently is only observed in Polygon Testnet)

tgmichel avatar Dec 09 '22 12:12 tgmichel

Hello @tgmichel Thanks for your PR ! Do you think it's possible to add also incoming transactions for localhost with ganache ? Then it will help when we test locally.

chloeYue avatar Dec 20 '22 14:12 chloeYue

add also incoming transactions for localhost with ganache

The current controller for incoming transactions leverages Etherscan Api, which is not available for Ganache afaik.

tgmichel avatar Dec 21 '22 08:12 tgmichel

@tgmichel @brad-decker @seaona @PeterYinusa

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

clydesdales avatar Jan 17 '23 05:01 clydesdales

@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 avatar Feb 08 '23 10:02 tgmichel

@tgmichel ready for merge, @pedronfigueiredo and I will get this merged once the rebase occurs

brad-decker avatar Feb 08 '23 14:02 brad-decker

@brad-decker done

tgmichel avatar Feb 08 '23 17:02 tgmichel

@pedronfigueiredo when you come online please review and merge :D

brad-decker avatar Feb 08 '23 20:02 brad-decker