Add detection params (userAddress, chainId) and remove duplicate source of truth #636

wachunei · 2021-11-22T13:16:01Z

Description

REMOVED
- [BREAKING] Both collectibles and collectibleContracts are removed from CollectiblesController state.
FIXED:
- [BREAKING] Change auto detection from boolean to an object containing address and chainId that was used to autodetect.

Checklist

Tests are included if applicable
Any added code is fully documented

Issue

Related MetaMask/metamask-mobile#3335

gantunesr · 2021-11-26T13:42:29Z

src/assets/CollectibleDetectionController.ts

@@ -353,10 +365,7 @@ export class CollectibleDetectionController extends BaseController<
          }

          /* istanbul ignore else */
-          if (
-            !ignored &&
-            requestedSelectedAddress === this.config.selectedAddress


Interesting, so this condition was only added on the CollectibleDetectionController and not in the CollectiblesController

gantunesr

Left some small comments

src/assets/CollectiblesController.ts

adonesky1 · 2021-11-26T19:47:08Z

src/assets/CollectibleDetectionController.test.ts

    });
+    collectiblesController.configure({ selectedAddress });


I know these are unit tests so we don't need to test the integration points with other Controllers but I generally like to use the upstream method where the piece of state we're subscribed to actually changes (in this case it would be preferences.setSelectedAddress(selectedAddress)). I think it makes our network of assumptions safer but I'm open to being convinced otherwise! Thoughts @Gudahtt @wachunei @gantunesr

If we wanted to test this robustly we'd do both. I'd rather us migrate to using the new base controller than waste our time doing that though, because we don't really want configure to be public API like this.

Following your suggestion would be better at least in that it'd be more compatible with BaseControllerV2 when we do migrate.

adonesky1 · 2021-11-26T19:53:43Z

src/assets/CollectibleDetectionController.test.ts

    expect(collectiblesController.state.ignoredCollectibles).toHaveLength(0);
    collectiblesController.removeAndIgnoreCollectible(
-      '0x1d963688fe2209a98db35c67a041524822cf04ff',
+      '0x1d963688FE2209A98dB35C67A041524822Cf04ff',


just curious why was the checksum change here necessary?

Not really, I did it without noticing 😅

src/assets/CollectibleDetectionController.test.ts

adonesky1 · 2021-11-26T20:05:59Z

src/assets/CollectibleDetectionController.ts

@@ -131,6 +132,7 @@ export interface ApiCollectibleCollection {
 export interface CollectibleDetectionConfig extends BaseConfig {
  interval: number;
  networkType: NetworkType;
+  chainId: `0x${string}` | `${number}` | number;


Whoa! I didn't know you could do string interpolated types! So cool

adonesky1 · 2021-11-26T20:09:44Z

src/assets/CollectibleDetectionController.ts

      return;
    }

    await safelyExecute(async () => {
-      const apiCollectibles = await this.getOwnerCollectibles();
+      const apiCollectibles = await this.getOwnerCollectibles(selectedAddress);


I'm not understanding what we achieve by getting selectedAddress in detectCollectibles and passing it in to getOwnerCollectibles rather than just pulling selectedAddress off config in getOwnerCollectibles?

We just want to avoid grabbing it in different places to make sure it does not change along the execution of the procedure

adonesky1

I think in general I don't like the pattern of replacing undefined with an empty array because it maintains the default value of the collectibles and collectibleContracts subset state values. I think it ends up causing more confusion. Open to be convinced otherwise though!

src/assets/CollectibleDetectionController.test.ts

src/assets/CollectiblesController.test.ts

adonesky1 · 2021-11-26T20:43:59Z

src/assets/CollectiblesController.test.ts

+        collectiblesController.state.allCollectibleContracts[selectedAddress][
+          chainId
+        ],
+      ).toHaveLength(0);


Should we remove the key rather than just remove the item from the array when there are no more left for the address/chainId combo?

I agree that this would be nicer, so that the behaviour after adding and removing a collectible matches the initial behaviour. Leaves us with less cases to test, and it's simpler for anyone using this state. I don't want to block on it though.

src/assets/CollectiblesController.ts

Co-authored-by: Alex Donesky <adonesky@gmail.com>

wachunei · 2021-11-30T12:43:56Z

I updated the expect arguments to be assertive (removed optional chaining when we expect a value, removed default []) and the expectations to match this change.

About this:

Should we remove the key rather than just remove the item from the array when there are no more left for the address/chainId combo?

I think it makes sense to prune empty nodes, is it something we do usually? I'd defer decision to you guys that are more familiar with controllers conventions.

adonesky1

LGTM

Gudahtt · 2021-11-30T20:34:25Z

So these "detection params", what purpose do these serve? Is this so that the chainID/address at time of detection is preserved, so that the NFT is assigned correctly if the user switches accounts/networks while the detection was still being processed? I didn't see the problem this was meant to solve explained anywhere.

wachunei · 2021-11-30T20:36:52Z

Yes, exactly that

src/assets/CollectiblesController.ts

Gudahtt · 2021-11-30T21:03:49Z

src/assets/CollectibleDetectionController.ts

-            );
+            await this.addCollectible(address, token_id, collectibleMetadata, {
+              userAddress: selectedAddress,
+              chainId: chainId as string,


Nit: We should not need to cast here. If it's possible for this to be something other than a string, we need to deal with that here. If it's not, then the type is wrong and we should fix that instead.

The chain ID normalization above is responsible for this. It is guaranteed to be a string here but the normalization is done in a way TypeScript's inference is having trouble with. I've just prototyped a solution that I can prepare as a follow up PR later.

Basically it's to move the normalization to a utility function, and adjust the conditions to help TypeScript's inference along a bit better, and add a "throw" case for invalid chain IDs.

I see, this type and normalization is also present in the GasFeeController, so we can use that util there.

Gudahtt · 2021-11-30T21:19:52Z

src/assets/CollectibleDetectionController.ts

-      this.configure({ networkType: provider.type });
+      this.configure({
+        networkType: provider.type,
+        chainId: provider.chainId as CollectibleDetectionConfig['chainId'],


Nit: Ideally we'd parse this here to guarantee that the chain ID is the expected type, rather than cast. Or propagate this new chain ID type into the network controller as well. I guess we can do that later.

src/assets/CollectibleDetectionController.ts

Gudahtt · 2021-11-30T21:25:44Z

src/assets/CollectibleDetectionController.ts

@@ -228,6 +229,7 @@ export class CollectibleDetectionController extends BaseController<
    this.defaultConfig = {
      interval: DEFAULT_INTERVAL,
      networkType: MAINNET,
+      chainId: '1',


Nit: I don't object to doing this for now, since the default networkType is MAINNET already, so this is a pre-existing issue. But we shouldn't be setting a default network at all, we can get the true current network from the network controller by passing in a getNetworkState function to the constructor.

This could lead to bugs where something runs initially before the first network state update, and uses the wrong network.

Gudahtt · 2021-11-30T21:40:34Z

Overall looks good! I'm very glad to see that duplicate state removed. It would have been better to do this in two separate PRs, but what's done is done. I can take another look when conflicts are resolved.

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

wachunei · 2021-11-30T21:50:48Z

@Gudahtt Conflicts are resolved 👌

Gudahtt

LGTM!

…ce of truth (#636) Co-authored-by: Alex Donesky <adonesky@gmail.com> Co-authored-by: Mark Stacey <markjstacey@gmail.com>

wachunei added 2 commits November 22, 2021 10:13

Add detection params (address, chainId) to collectibles autodetection

ab84296

Remove duplicated source of truth for collectibles

4a13494

wachunei changed the title ~~Add detection params (address, chainId) to collectibles autodetection~~ Add detection params (address, chainId) and remove duplicate source of truth Nov 24, 2021

Merge branch 'main' into fix/collectibles-autodetect

31ea37c

wachunei marked this pull request as ready for review November 25, 2021 15:35

wachunei requested a review from a team as a code owner November 25, 2021 15:35

wachunei mentioned this pull request Nov 25, 2021

Fix collectibles autodetection MetaMask/metamask-mobile#3458

Merged

4 tasks

gantunesr reviewed Nov 26, 2021

View reviewed changes

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

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

Remove autodetected property from DetectionParams

5ebef38

adonesky1 reviewed Nov 26, 2021

View reviewed changes

src/assets/CollectibleDetectionController.test.ts Outdated Show resolved Hide resolved

adonesky1 reviewed Nov 26, 2021

View reviewed changes

src/assets/CollectibleDetectionController.test.ts Show resolved Hide resolved

adonesky1 reviewed Nov 26, 2021

View reviewed changes

src/assets/CollectibleDetectionController.test.ts Outdated Show resolved Hide resolved

adonesky1 reviewed Nov 26, 2021

View reviewed changes

adonesky1 requested changes Nov 26, 2021

View reviewed changes

wachunei and others added 4 commits November 30, 2021 09:10

Update src/assets/CollectiblesController.ts detection param description

b3c5130

Co-authored-by: Alex Donesky <adonesky@gmail.com>

Update src/assets/CollectiblesController.ts detection param description

059a9e5

Co-authored-by: Alex Donesky <adonesky@gmail.com>

Update src/assets/CollectiblesController.ts detection param description

598f1ff

Co-authored-by: Alex Donesky <adonesky@gmail.com>

Refactor test, remove default value for collectibles expect argument

e12a3dc

Rename address to userAddress in detection parameters

a80f003

wachunei requested a review from adonesky1 November 30, 2021 17:44

wachunei added a commit that referenced this pull request Nov 30, 2021

Upgrade to commit a80f003 from #636

e2877ab

adonesky1 previously approved these changes Nov 30, 2021

View reviewed changes

Gudahtt reviewed Nov 30, 2021

View reviewed changes

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

Change detection param description

d7f56d0

wachunei dismissed adonesky1’s stale review via d7f56d0 November 30, 2021 21:21

Gudahtt reviewed Nov 30, 2021

View reviewed changes

Update src/assets/CollectibleDetectionController.ts

667ecce

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

mobularay assigned wachunei Nov 30, 2021

Merge branch 'main' into fix/collectibles-autodetect

aa631be

wachunei added a commit that referenced this pull request Nov 30, 2021

Upgrade to commit aa631be from #636

e61f514

Gudahtt approved these changes Nov 30, 2021

View reviewed changes

wachunei changed the title ~~Add detection params (address, chainId) and remove duplicate source of truth~~ Add detection params (userAddress, chainId) and remove duplicate source of truth Nov 30, 2021

wachunei merged commit 7754dd7 into main Nov 30, 2021

wachunei deleted the fix/collectibles-autodetect branch November 30, 2021 22:21

This was referenced Nov 30, 2021

21.0.0 #647

Merged

Feat/collectibles display MetaMask/metamask-extension#12873

Merged

gantunesr mentioned this pull request Dec 3, 2021

NFT Caching issue MetaMask/metamask-mobile#3335

Closed

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

Add detection params (userAddress, chainId) and remove duplicate sour…

3f81fa2

…ce of truth (#636) Co-authored-by: Alex Donesky <adonesky@gmail.com> Co-authored-by: Mark Stacey <markjstacey@gmail.com>

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

Add detection params (userAddress, chainId) and remove duplicate sour…

5721530

…ce of truth (#636) Co-authored-by: Alex Donesky <adonesky@gmail.com> Co-authored-by: Mark Stacey <markjstacey@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add detection params (userAddress, chainId) and remove duplicate source of truth #636

Add detection params (userAddress, chainId) and remove duplicate source of truth #636

wachunei commented Nov 22, 2021 •

edited

Loading

gantunesr Nov 26, 2021 •

edited

Loading

gantunesr left a comment

adonesky1 Nov 26, 2021

Gudahtt Nov 30, 2021

adonesky1 Nov 26, 2021

wachunei Nov 26, 2021

adonesky1 Nov 26, 2021 •

edited

Loading

adonesky1 Nov 26, 2021

wachunei Nov 26, 2021

adonesky1 left a comment

adonesky1 Nov 26, 2021

Gudahtt Nov 30, 2021

wachunei commented Nov 30, 2021

adonesky1 left a comment

Gudahtt commented Nov 30, 2021

wachunei commented Nov 30, 2021

Gudahtt Nov 30, 2021

Gudahtt Nov 30, 2021

Gudahtt Nov 30, 2021

wachunei Nov 30, 2021

Gudahtt Nov 30, 2021

Gudahtt Nov 30, 2021

Gudahtt commented Nov 30, 2021

wachunei commented Nov 30, 2021

Gudahtt left a comment

Add detection params (userAddress, chainId) and remove duplicate source of truth #636

Add detection params (userAddress, chainId) and remove duplicate source of truth #636

Conversation

wachunei commented Nov 22, 2021 • edited Loading

gantunesr Nov 26, 2021 • edited Loading

Choose a reason for hiding this comment

gantunesr 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

adonesky1 Nov 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adonesky1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wachunei commented Nov 30, 2021

adonesky1 left a comment

Choose a reason for hiding this comment

Gudahtt commented Nov 30, 2021

wachunei commented Nov 30, 2021

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

Gudahtt commented Nov 30, 2021

wachunei commented Nov 30, 2021

Gudahtt left a comment

Choose a reason for hiding this comment

wachunei commented Nov 22, 2021 •

edited

Loading

gantunesr Nov 26, 2021 •

edited

Loading

adonesky1 Nov 26, 2021 •

edited

Loading