BREAKING: Fetch and return token image as part of `getDetails` calls on ERC721Standard and ERC1155Standard #702

adonesky1 · 2022-03-02T21:17:17Z

ADDED:
- BREAKING Adds functionality to fetch and return token images as part of getDetails calls on ERC721Standard and ERC1155Standard
  - This change is breaking because it requires that the AssetsContractController (on which the ERC721Standard and ERC1155Standard are instantiated) be passed a listener for onPreferencesStateChange from the PreferencesController so that it can use the user's preferred IPFSGateway to fetch any images hosted on IPFS. Consumers will have to pass onPreferencesStateChange in an options object (first arg) to the AssetsContractController constructor when initializing.

Resolves #689

wachunei

LGTM

mcmire

Three questions:

Are the new tests for getDetails actually hitting the IPFS gateway? I see you were trying to use Nock to mock some kind of API, perhaps it would be worth doing this for the IPFS gateway? This raises the question of "how should we handle external APIs in tests?" again.
Perhaps tying into 1, is it possible to test the case in which getDetails returns a non-nullish value for image in the expected set of data?
How are we testing the new change to AssetsContractController where onPreferencesStateChange can update the configuration object?

adonesky1 · 2022-03-03T17:22:18Z

Three questions:

All good callouts. Will add/modify tests to address these concerns.

adonesky1 · 2022-03-03T19:45:36Z

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

+      standard: 'ERC721',
+      symbol: 'BAYC',
+      tokenURI:
+        'https://bafybeihpjhkeuiq3k6nqa3fkgeigeri7iebtrsuyuey5y6vy36n345xmbi.ipfs.cloudflare-ipfs.com/3',


tokenURI hosted on IPFS cc @mcmire @wachunei

adonesky1 · 2022-03-03T19:46:01Z

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

+          image: 'https://card.godsunchained.com/?id=1329&q=4',
+        };
+      });
+
    const expectedResult = {
      name: 'Gods Unchained',
      standard: 'ERC721',
      symbol: 'GODS',
      tokenURI: 'https://api.godsunchained.com/card/4',


tokenURI not hosted on IPFS cc @mcmire @wachunei

adonesky1 · 2022-03-03T19:47:06Z

src/assets/AssetsContractController.test.ts

+  it('should update the ipfsGateWay config value when this value is changed in the preferences controller', () => {
+    expect(assetsContract.config).toStrictEqual({
+      ipfsGateway: IPFS_DEFAULT_GATEWAY_URL,
+      provider: undefined,
+    });
+
+    preferences.setIpfsGateway('newIPFSGateWay');
+    expect(assetsContract.config).toStrictEqual({
+      ipfsGateway: 'newIPFSGateWay',


test new preferences listener updates to controller config values cc @mcmire

adonesky1 · 2022-03-03T19:48:20Z

@mcmire I accidentally squashed commits but I've addressed your feedback here: cec4496

@wachunei could you take another look too when you have a chance 😄

adonesky1 · 2022-03-03T20:51:46Z

Thanks for updating these tests. For these types of tests where you've modified how the controller is instantiated so that you now pass onPreferencesStateChange, does this have any effect on the tests?

For all except the test I added on AssetsContractController.test.ts you could pass

onPreferencesStateChange: (listener) => {
    // do nothing
  }

cc @mcmire
I think we write many unit tests for these controllers where the passed in listeners are not actively used by the test but are required by the constructor.

…lls on ERC721Standard and ERC1155Standard (MetaMask#702) * Fetch and return token image as part of getDetails call * adapt tests

…lls on ERC721Standard and ERC1155Standard (#702) * Fetch and return token image as part of getDetails call * adapt tests

adonesky1 force-pushed the return-token-image-from-getDetails branch from 386f39f to 529638f Compare March 2, 2022 21:24

Fetch and return token image as part of getDetails call

6dff5c6

adonesky1 force-pushed the return-token-image-from-getDetails branch 2 times, most recently from 90c459e to fb4fa72 Compare March 2, 2022 23:17

adonesky1 changed the title ~~BREAKING: Fetch and return token image as part of getDetails` calls on ERC721Standard and ERC1155Standard~~ **BREAKING**: Fetch and return token image as part of getDetails calls on ERC721Standard and ERC1155Standard Mar 2, 2022

adonesky1 requested review from gantunesr and wachunei March 2, 2022 23:18

adonesky1 marked this pull request as ready for review March 2, 2022 23:23

adonesky1 requested a review from a team as a code owner March 2, 2022 23:23

wachunei previously approved these changes Mar 3, 2022

View reviewed changes

mcmire requested changes Mar 3, 2022

View reviewed changes

adonesky1 dismissed wachunei’s stale review via ad4599c March 3, 2022 19:41

adonesky1 force-pushed the return-token-image-from-getDetails branch from fb4fa72 to ad4599c Compare March 3, 2022 19:41

adapt tests

cec4496

adonesky1 force-pushed the return-token-image-from-getDetails branch from ad4599c to cec4496 Compare March 3, 2022 19:44

adonesky1 commented Mar 3, 2022

View reviewed changes

adonesky1 requested review from mcmire and wachunei March 3, 2022 20:36

mcmire approved these changes Mar 3, 2022

View reviewed changes

Merge branch 'main' into return-token-image-from-getDetails

4bbf4a0

wachunei approved these changes Mar 3, 2022

View reviewed changes

adonesky1 merged commit 613c5b9 into main Mar 3, 2022

adonesky1 deleted the return-token-image-from-getDetails branch March 3, 2022 21:22

adonesky1 mentioned this pull request Mar 8, 2022

26.0.0 #711

Merged

gantunesr mentioned this pull request Mar 10, 2022

Upgrade controllers v26.0.0 and Swaps controller to v6.6.0 MetaMask/metamask-mobile#3878

Merged

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

**BREAKING**: Fetch and return token image as part of getDetails ca…

8ac3dc8

…lls on ERC721Standard and ERC1155Standard (#702) * Fetch and return token image as part of getDetails call * adapt tests

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

**BREAKING**: Fetch and return token image as part of getDetails ca…

59d6c9b

…lls on ERC721Standard and ERC1155Standard (#702) * Fetch and return token image as part of getDetails call * adapt tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BREAKING: Fetch and return token image as part of `getDetails` calls on ERC721Standard and ERC1155Standard #702

BREAKING: Fetch and return token image as part of `getDetails` calls on ERC721Standard and ERC1155Standard #702

adonesky1 commented Mar 2, 2022 •

edited

Loading

wachunei left a comment

mcmire left a comment •

edited

Loading

adonesky1 commented Mar 3, 2022

adonesky1 Mar 3, 2022

adonesky1 Mar 3, 2022

adonesky1 Mar 3, 2022

adonesky1 commented Mar 3, 2022

adonesky1 commented Mar 3, 2022 •

edited

Loading

**BREAKING**: Fetch and return token image as part of getDetails calls on ERC721Standard and ERC1155Standard #702

**BREAKING**: Fetch and return token image as part of getDetails calls on ERC721Standard and ERC1155Standard #702

Conversation

adonesky1 commented Mar 2, 2022 • edited Loading

wachunei left a comment

Choose a reason for hiding this comment

mcmire left a comment • edited Loading

Choose a reason for hiding this comment

adonesky1 commented Mar 3, 2022

adonesky1 Mar 3, 2022

Choose a reason for hiding this comment

adonesky1 Mar 3, 2022

Choose a reason for hiding this comment

adonesky1 Mar 3, 2022

Choose a reason for hiding this comment

adonesky1 commented Mar 3, 2022

adonesky1 commented Mar 3, 2022 • edited Loading

BREAKING: Fetch and return token image as part of `getDetails` calls on ERC721Standard and ERC1155Standard #702

BREAKING: Fetch and return token image as part of `getDetails` calls on ERC721Standard and ERC1155Standard #702

adonesky1 commented Mar 2, 2022 •

edited

Loading

mcmire left a comment •

edited

Loading

adonesky1 commented Mar 3, 2022 •

edited

Loading