[BREAKING] - Create get token standard method / Standardize ERC721/1155/20 method names

[adonesky1]

• CHANGED:

  • BREAKING Renames many methods on the AssetsContractController to include the contract type they are used by in the name. This change is entirely to enhance legibility and maintainability as more contract types are supported.

• ADDED:

  • Adds a single method getTokenStandardAndDetails to determine whether the input contract confirms to particular known token standard (ERC20, ERC721 or ERC1155) and return the detected standard along with some key values/details about that the contract that pertain to that standard. These additional details may include, in the case of an ERC721, for example, details about a specific tokenId within that contract, or, in the case of ERC20, the balance of the current user in the contract.

[wachunei]

Random ideas:

• Instead of waterfalling, could we use Promise.allSettled
• Could we also turn those promises into utils, private methods or members of the standard itself

[mcmire]

A couple of cleanup things and a question.

[wachunei]

Another idea for the future: make the standard class extend a base standard class that has web3 in the constructor arguments

[wachunei]

so much better 👌

[adonesky1]

@wachunei @mcmire rebased and added this commit: f5ae2a9, which mocks all network requests for test files that were added/touched. Take another look when you have a chance.

[mcmire]

Looks better, just a few suggestions.

[gantunesr]

Can you add "[BREAKING]" to the title of the PR so it's easier to identify?

[mcmire]

Did another pass and there were a few things I noticed.

[mcmire]

Can we use a type for this instead of any? And why is web3 optional? It seems that everything in this class relies on web3 being provided.

[gantunesr]

I agree with you @mcmire, but I think for this we have to upgrade the web3 dependence, which could be done in another PR. But I'm not against this if @adonesky1 wants to upgrade the module in this PR

[mcmire]

Oh, does web3 provide types now? I agree that we probably wouldn't want to upgrade it in this PR, but I was thinking in the meantime we could give this property a type on the spot based on how we end up using it in this class. For instance:

type ERC1155ABI = ...;
type Address = `0x${string}`

type Contract = {
  at: Address
}

type Web3 = {
  eth: {
    contract: (abi: ERC1155ABI): Contract
  }
}

That said, I don't believe that abiERC1155 in @metamask/metamask-eth-abis has the right type on it. My feeling is that all of the objects that that module exports should have as const on them. Right now it's possible to pass an ABI that has the same shape but isn't exactly the ERC-1155 ABI. Maybe we could have a generic ABI type to get around this, but then we'd be repeating work that is probably elsewhere. So maybe this is fine for now.

[mcmire]

Can this be logged somewhere for tech debt purposes, though?

[gantunesr]

Thats a good point, I think that could work in the meantime for this PR. For future development we should upgrade this module and use the types provided by web3 or start using the ethers dependence. I remember that extension changed from web3 to ethers, would be easier if we all use the same.

[mcmire]

Yeah, I am definitely a fan of using ethers. It feels a lot more maintained at this point and is fully typed, and I think we can avoid the pain that we've been experiencing with web3. Of course that's a larger change but that'd be my recommendation for new code going forward.

[adonesky1]

I've made updates here: 8effcaf and here c9abc81, that I believe should address these concerns

[adonesky1]

@mcmire @gantunesr take a look and let me know what you think

[mcmire]

Looks good, thank you!

[gantunesr]

LGTM. I'll wait for the other comments to be resolved before approving.

[mcmire]

This all looks good to me! Thanks for enduring all of the feedback 😅

[gantunesr]

LGTM

[gantunesr]

Hey @adonesky1, I just noticed that you added the method symbol from the ERC20 contract, in the metamask-eth-abis module there are tests to ensure that no one removes the methods we're using from the ABIs. We have to add the test for symbol, I can do it next week.

[adonesky1]

We have to add the test for symbol, I can do it next week.

You mean to the metamask-eth-abis repo right?

[gantunesr]

You mean to the metamask-eth-abis repo right?

Yes