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

adonesky1 · 2021-12-20T22:17:41Z

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 · 2021-12-28T17:08:01Z

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

src/assets/CollectibleStandards/ERC1155/ERC1155Standard.ts

src/assets/CollectibleStandards/ERC721/ERC721Standard.ts

mcmire

A couple of cleanup things and a question.

src/assets/AssetsContractController.ts

src/assets/Standards/ERC20Standard.ts

wachunei · 2022-01-04T15:22:13Z

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

wachunei · 2022-01-04T18:13:25Z

src/assets/CollectiblesController.ts

+      getERC721AssetName,
+      getERC721AssetSymbol,
+      getERC721TokenURI,
+      getERC721OwnerOf,
+      getERC1155BalanceOf,
+      getERC1155TokenURI,


so much better 👌

src/assets/Standards/CollectibleStandards/ERC721/ERC721Standard.test.ts

adonesky1 · 2022-01-05T19:22:33Z

@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.

src/assets/Standards/CollectibleStandards/ERC1155/ERC1155Standard.test.ts

src/assets/Standards/CollectibleStandards/ERC721/ERC721Standard.test.ts

src/assets/Standards/ERC20Standard.test.ts

gantunesr · 2022-01-07T17:17:59Z

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.

src/assets/AssetsContractController.ts

src/assets/CollectiblesController.ts

src/assets/Standards/CollectibleStandards/ERC1155/ERC1155Standard.ts

mcmire · 2022-01-07T17:23:58Z

src/assets/Standards/CollectibleStandards/ERC1155/ERC1155Standard.ts

+  private web3: any;
+
+  constructor(web3?: any) {
+    this.web3 = web3;


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.

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

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.

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

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.

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.

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

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

Looks good, thank you!

src/assets/Standards/CollectibleStandards/ERC721/ERC721Standard.ts

src/assets/Standards/ERC20Standard.ts

gantunesr · 2022-01-07T18:31:09Z

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 · 2022-01-08T02:24:56Z

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 · 2022-01-08T03:01:55Z

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

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

gantunesr · 2022-01-08T03:22:08Z

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

Yes

…55/20 method names (#667) * add getTokenStandard method * standardize method name patterns * improve typing

adonesky1 force-pushed the create-getTokenStandard-method branch 3 times, most recently from 5ac4d99 to 93fecae Compare December 24, 2021 23:39

adonesky1 marked this pull request as ready for review December 26, 2021 20:04

adonesky1 requested a review from a team as a code owner December 26, 2021 20:04

adonesky1 assigned wachunei, Gudahtt and gantunesr Dec 26, 2021

adonesky1 requested review from gantunesr, Gudahtt and wachunei December 27, 2021 19:15

gantunesr reviewed Jan 3, 2022

View reviewed changes

src/assets/CollectibleStandards/ERC1155/ERC1155Standard.ts Outdated Show resolved Hide resolved

gantunesr reviewed Jan 3, 2022

View reviewed changes

src/assets/CollectibleStandards/ERC721/ERC721Standard.ts Outdated Show resolved Hide resolved

adonesky1 force-pushed the create-getTokenStandard-method branch 2 times, most recently from 417171d to 8fd61c5 Compare January 3, 2022 19:47

mcmire reviewed Jan 3, 2022

View reviewed changes

src/assets/AssetsContractController.ts Outdated Show resolved Hide resolved

src/assets/Standards/ERC20Standard.ts Outdated Show resolved Hide resolved

src/assets/Standards/ERC20Standard.ts Outdated Show resolved Hide resolved

adonesky1 force-pushed the create-getTokenStandard-method branch 2 times, most recently from febe905 to e3f606f Compare January 3, 2022 23:20

adonesky1 force-pushed the create-getTokenStandard-method branch 2 times, most recently from 5ed0370 to 6f2e400 Compare January 4, 2022 16:41

adonesky1 requested review from gantunesr and mcmire January 4, 2022 17:09

adonesky1 changed the title ~~Create get token standard method~~ Create get token standard method / Standardize ERC721/1155/20 method names Jan 4, 2022

wachunei previously approved these changes Jan 4, 2022

View reviewed changes

adonesky1 mentioned this pull request Jan 4, 2022

Feat/collectibles send flow MetaMask/metamask-extension#13048

Merged

adonesky1 added 3 commits January 5, 2022 09:33

add getTokenStandard method

03265df

enhancing method

b561328

more wip

08442c3

adonesky1 force-pushed the create-getTokenStandard-method branch from 76e1b9c to f5ae2a9 Compare January 5, 2022 19:20

adonesky1 force-pushed the create-getTokenStandard-method branch 2 times, most recently from f1825ad to 9591559 Compare January 5, 2022 19:34

mcmire reviewed Jan 5, 2022

View reviewed changes

src/assets/Standards/CollectibleStandards/ERC1155/ERC1155Standard.test.ts Show resolved Hide resolved

src/assets/Standards/CollectibleStandards/ERC721/ERC721Standard.test.ts Show resolved Hide resolved

src/assets/Standards/ERC20Standard.test.ts Show resolved Hide resolved

mock network requests for new tests

3e48605

adonesky1 force-pushed the create-getTokenStandard-method branch from 9591559 to 3e48605 Compare January 5, 2022 21:07

adonesky1 requested review from mcmire and wachunei January 5, 2022 21:09

Merge branch 'main' into create-getTokenStandard-method

5fac471

adonesky1 changed the title ~~Create get token standard method / Standardize ERC721/1155/20 method names~~ [BREAKING] - Create get token standard method / Standardize ERC721/1155/20 method names Jan 7, 2022

mcmire requested changes Jan 7, 2022

View reviewed changes

mcmire mentioned this pull request Jan 7, 2022

Apply as const to objects? MetaMask/metamask-eth-abis#5

Closed

addressing feedback

8effcaf

adonesky1 force-pushed the create-getTokenStandard-method branch from 8fe5ccc to 086ecdf Compare January 7, 2022 19:29

improve typing

c9abc81

adonesky1 force-pushed the create-getTokenStandard-method branch from 086ecdf to c9abc81 Compare January 7, 2022 19:32

adonesky1 requested a review from mcmire January 7, 2022 19:55

mcmire approved these changes Jan 7, 2022

View reviewed changes

gantunesr approved these changes Jan 7, 2022

View reviewed changes

adonesky1 merged commit c38f798 into main Jan 10, 2022

adonesky1 deleted the create-getTokenStandard-method branch January 10, 2022 16:19

adonesky1 mentioned this pull request Jan 10, 2022

25.0.0 #673

Merged

MajorLift pushed a commit that referenced this pull request Oct 11, 2023

[BREAKING] - Create get token standard method / Standardize ERC721/11…

3b92511

…55/20 method names (#667) * add getTokenStandard method * standardize method name patterns * improve typing

MajorLift pushed a commit that referenced this pull request Oct 11, 2023

[BREAKING] - Create get token standard method / Standardize ERC721/11…

eb612a5

…55/20 method names (#667) * add getTokenStandard method * standardize method name patterns * improve typing

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

adonesky1 commented Dec 20, 2021 •

edited

Loading

wachunei commented Dec 28, 2021

mcmire left a comment

wachunei commented Jan 4, 2022

wachunei Jan 4, 2022

adonesky1 commented Jan 5, 2022

mcmire left a comment

gantunesr commented Jan 7, 2022

mcmire left a comment

mcmire Jan 7, 2022

gantunesr Jan 7, 2022

mcmire Jan 7, 2022

mcmire Jan 7, 2022

gantunesr Jan 7, 2022

mcmire Jan 7, 2022

adonesky1 Jan 7, 2022 •

edited

Loading

adonesky1 Jan 7, 2022

mcmire Jan 7, 2022

gantunesr commented Jan 7, 2022

mcmire left a comment •

edited

Loading

gantunesr left a comment

gantunesr commented Jan 8, 2022

adonesky1 commented Jan 8, 2022

gantunesr commented Jan 8, 2022

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

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

Conversation

adonesky1 commented Dec 20, 2021 • edited Loading

wachunei commented Dec 28, 2021

mcmire left a comment

Choose a reason for hiding this comment

wachunei commented Jan 4, 2022

Choose a reason for hiding this comment

adonesky1 commented Jan 5, 2022

mcmire left a comment

Choose a reason for hiding this comment

gantunesr commented Jan 7, 2022

mcmire left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adonesky1 Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gantunesr commented Jan 7, 2022

mcmire left a comment • edited Loading

Choose a reason for hiding this comment

gantunesr left a comment

Choose a reason for hiding this comment

gantunesr commented Jan 8, 2022

adonesky1 commented Jan 8, 2022

gantunesr commented Jan 8, 2022

adonesky1 commented Dec 20, 2021 •

edited

Loading

adonesky1 Jan 7, 2022 •

edited

Loading

mcmire left a comment •

edited

Loading