Skip to content

Commit

Permalink
Update controllers that rely on provider to listen to `NetworkControl…
Browse files Browse the repository at this point in the history
…ler:networkDidChange` instead of `NetworkController:stateChange` (#3610)

## Explanation

Listening to `NetworkController:stateChange` to catch network changes
doesn't guarantee that the provider proxy has also been updated to point
to the new network. `NetworkController:networkDidChange` does however.
This PR updates several controllers that rely on the provider proxy to
listen to `NetworkController:networkDidChange` rather than
`NetworkController:stateChange`. GasFeeController and
AssetsContractController are already listening to
`NetworkController:networkDidChange` on the extension side despite their
constructor param hook names still being `onNetworkStateChange` (which
this PR also renames to be correct)

## References

* See MetaMask/MetaMask-planning#1713

## Changelog

### `@metamask/assets-controllers`

- **BREAKING**: `TokensController` constructor params
`onNetworkStateChange` replaced with `onNetworkDidChange`
- **BREAKING**: `AssetsContractController` constructor params
`onNetworkStateChange` replaced with `onNetworkDidChange`

### `@metamask/ens-controller`

- **BREAKING**: `EnsController` constructor params
`onNetworkStateChange` replaced with `onNetworkDidChange`

### `@metamask/gas-fee-controller`

- **BREAKING**: `GasFeeController` will subscribe to
`NetworkController:networkDidChange` now instead of
`NetworkController:stateChange` using the passed in `messenger` param
when `onNetworkDidChange` is not provided
- **CHANGED**: `GasFeeController` constructor params optional
`onNetworkStateChange` replaced with optional `onNetworkDidChange`


## Checklist

- [ ] I've updated the test suite for new or updated code as appropriate
- [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [ ] I've highlighted breaking changes using the "BREAKING" category
above as appropriate

---------

Co-authored-by: Alex Donesky <adonesky@gmail.com>
  • Loading branch information
jiexi and adonesky1 authored Dec 6, 2023
1 parent f7f88c4 commit 64d6166
Show file tree
Hide file tree
Showing 12 changed files with 94 additions and 98 deletions.
58 changes: 29 additions & 29 deletions packages/assets-controllers/src/AssetsContractController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ async function setupAssetContractControllers() {
const assetsContract = new AssetsContractController({
chainId: ChainId.mainnet,
onPreferencesStateChange: (listener) => preferences.subscribe(listener),
onNetworkStateChange: (listener) =>
messenger.subscribe('NetworkController:stateChange', listener),
onNetworkDidChange: (listener) =>
messenger.subscribe('NetworkController:networkDidChange', listener),
getNetworkClientById: (networkClientId: NetworkClientId) =>
({
...network.getNetworkClientById(networkClientId),
Expand Down Expand Up @@ -129,7 +129,7 @@ describe('AssetsContractController', () => {
ipfsGateway: IPFS_DEFAULT_GATEWAY_URL,
provider: undefined,
});
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should update the ipfsGateWay config value when this value is changed in the preferences controller', async () => {
Expand All @@ -148,15 +148,15 @@ describe('AssetsContractController', () => {
provider: undefined,
});

messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should throw when provider property is accessed', async () => {
const { assetsContract, messenger } = await setupAssetContractControllers();
expect(() => console.log(assetsContract.provider)).toThrow(
'Property only used for setting',
);
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should throw missing provider error when getting ERC-20 token balance when missing provider', async () => {
Expand All @@ -168,7 +168,7 @@ describe('AssetsContractController', () => {
TEST_ACCOUNT_PUBLIC_ADDRESS,
),
).rejects.toThrow(MISSING_PROVIDER_ERROR);
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should throw missing provider error when getting ERC-20 token decimal when missing provider', async () => {
Expand All @@ -177,7 +177,7 @@ describe('AssetsContractController', () => {
await expect(
assetsContract.getERC20TokenDecimals(ERC20_UNI_ADDRESS),
).rejects.toThrow(MISSING_PROVIDER_ERROR);
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should get balance of ERC-20 token contract correctly', async () => {
Expand Down Expand Up @@ -231,7 +231,7 @@ describe('AssetsContractController', () => {
);
expect(UNIBalance.toString(16)).not.toBe('0');
expect(UNINoBalance.toString(16)).toBe('0');
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should get ERC-721 NFT tokenId correctly', async () => {
Expand Down Expand Up @@ -265,7 +265,7 @@ describe('AssetsContractController', () => {
0,
);
expect(tokenId).not.toBe(0);
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should throw missing provider error when getting ERC-721 token standard and details when missing provider', async () => {
Expand All @@ -277,7 +277,7 @@ describe('AssetsContractController', () => {
TEST_ACCOUNT_PUBLIC_ADDRESS,
),
).rejects.toThrow(MISSING_PROVIDER_ERROR);
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should throw contract standard error when getting ERC-20 token standard and details when provided with invalid ERC-20 address', async () => {
Expand All @@ -291,7 +291,7 @@ describe('AssetsContractController', () => {
TEST_ACCOUNT_PUBLIC_ADDRESS,
),
).rejects.toThrow(error);
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should get ERC-721 token standard and details', async () => {
Expand Down Expand Up @@ -356,7 +356,7 @@ describe('AssetsContractController', () => {
TEST_ACCOUNT_PUBLIC_ADDRESS,
);
expect(standardAndDetails.standard).toBe('ERC721');
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should get ERC-1155 token standard and details', async () => {
Expand Down Expand Up @@ -441,7 +441,7 @@ describe('AssetsContractController', () => {
expect(standardAndDetails.name).toBe('Apeiron Godiverse Collection');
expect(standardAndDetails.symbol).toBe('APEGC');

messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should get ERC-20 token standard and details', async () => {
Expand Down Expand Up @@ -538,7 +538,7 @@ describe('AssetsContractController', () => {
TEST_ACCOUNT_PUBLIC_ADDRESS,
);
expect(standardAndDetails.standard).toBe('ERC20');
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should get ERC-721 NFT tokenURI correctly', async () => {
Expand Down Expand Up @@ -587,7 +587,7 @@ describe('AssetsContractController', () => {
'0',
);
expect(tokenId).toBe('https://api.godsunchained.com/card/0');
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should throw an error when address given is not an ERC-721 NFT', async () => {
Expand Down Expand Up @@ -623,7 +623,7 @@ describe('AssetsContractController', () => {

const error = 'Contract does not support ERC721 metadata interface.';
await expect(result).rejects.toThrow(error);
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should get ERC-721 NFT name', async () => {
Expand Down Expand Up @@ -653,7 +653,7 @@ describe('AssetsContractController', () => {
});
const name = await assetsContract.getERC721AssetName(ERC721_GODS_ADDRESS);
expect(name).toBe('Gods Unchained');
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should get ERC-721 NFT symbol', async () => {
Expand Down Expand Up @@ -685,15 +685,15 @@ describe('AssetsContractController', () => {
ERC721_GODS_ADDRESS,
);
expect(symbol).toBe('GODS');
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should throw missing provider error when getting ERC-721 NFT symbol when missing provider', async () => {
const { assetsContract, messenger } = await setupAssetContractControllers();
await expect(
assetsContract.getERC721AssetSymbol(ERC721_GODS_ADDRESS),
).rejects.toThrow(MISSING_PROVIDER_ERROR);
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should get ERC-20 token decimals', async () => {
Expand Down Expand Up @@ -725,7 +725,7 @@ describe('AssetsContractController', () => {
ERC20_SAI_ADDRESS,
);
expect(Number(decimals)).toBe(18);
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should get ERC-20 token name', async () => {
Expand Down Expand Up @@ -757,7 +757,7 @@ describe('AssetsContractController', () => {
const name = await assetsContract.getERC20TokenName(ERC20_DAI_ADDRESS);

expect(name).toBe('Dai Stablecoin');
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should get ERC-721 NFT ownership', async () => {
Expand Down Expand Up @@ -790,15 +790,15 @@ describe('AssetsContractController', () => {
'148332',
);
expect(tokenId).not.toBe('');
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should throw missing provider error when getting ERC-721 NFT ownership', async () => {
const { assetsContract, messenger } = await setupAssetContractControllers();
await expect(
assetsContract.getERC721OwnerOf(ERC721_GODS_ADDRESS, '148332'),
).rejects.toThrow(MISSING_PROVIDER_ERROR);
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should get balance of ERC-20 token in a single call on network with token detection support', async () => {
Expand Down Expand Up @@ -831,7 +831,7 @@ describe('AssetsContractController', () => {
[ERC20_SAI_ADDRESS],
);
expect(balances[ERC20_SAI_ADDRESS]).toBeDefined();
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should not have balance in a single call after switching to network without token detection support', async () => {
Expand Down Expand Up @@ -908,7 +908,7 @@ describe('AssetsContractController', () => {
[ERC20_SAI_ADDRESS],
);
expect(noBalances).toStrictEqual({});
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should throw missing provider error when transferring single ERC-1155 when missing provider', async () => {
Expand All @@ -923,7 +923,7 @@ describe('AssetsContractController', () => {
'1',
),
).rejects.toThrow(MISSING_PROVIDER_ERROR);
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should get the balance of a ERC-1155 NFT for a given address', async () => {
Expand Down Expand Up @@ -957,7 +957,7 @@ describe('AssetsContractController', () => {
ERC1155_ID,
);
expect(Number(balance)).toBeGreaterThan(0);
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should throw missing provider error when getting the balance of a ERC-1155 NFT when missing provider', async () => {
Expand All @@ -969,7 +969,7 @@ describe('AssetsContractController', () => {
ERC1155_ID,
),
).rejects.toThrow(MISSING_PROVIDER_ERROR);
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});

it('should get the URI of a ERC-1155 NFT', async () => {
Expand Down Expand Up @@ -1003,6 +1003,6 @@ describe('AssetsContractController', () => {
ERC1155_ID,
);
expect(uri.toLowerCase()).toStrictEqual(expectedUri);
messenger.clearEventSubscriptions('NetworkController:stateChange');
messenger.clearEventSubscriptions('NetworkController:networkDidChange');
});
});
8 changes: 4 additions & 4 deletions packages/assets-controllers/src/AssetsContractController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ export class AssetsContractController extends BaseControllerV1<
* @param options - The controller options.
* @param options.chainId - The chain ID of the current network.
* @param options.onPreferencesStateChange - Allows subscribing to preference controller state changes.
* @param options.onNetworkStateChange - Allows subscribing to network controller state changes.
* @param options.onNetworkDidChange - Allows subscribing to network controller networkDidChange events.
* @param options.getNetworkClientById - Gets the network client with the given id from the NetworkController.
* @param config - Initial options used to configure this controller.
* @param state - Initial state to set on this controller.
Expand All @@ -107,14 +107,14 @@ export class AssetsContractController extends BaseControllerV1<
{
chainId: initialChainId,
onPreferencesStateChange,
onNetworkStateChange,
onNetworkDidChange,
getNetworkClientById,
}: {
chainId: Hex;
onPreferencesStateChange: (
listener: (preferencesState: PreferencesState) => void,
) => void;
onNetworkStateChange: (
onNetworkDidChange: (
listener: (networkState: NetworkState) => void,
) => void;
getNetworkClientById: NetworkController['getNetworkClientById'];
Expand All @@ -135,7 +135,7 @@ export class AssetsContractController extends BaseControllerV1<
this.configure({ ipfsGateway });
});

onNetworkStateChange((networkState) => {
onNetworkDidChange((networkState) => {
if (this.config.chainId !== networkState.providerConfig.chainId) {
this.configure({
chainId: networkState.providerConfig.chainId,
Expand Down
10 changes: 5 additions & 5 deletions packages/assets-controllers/src/NftController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,9 +151,9 @@ function setupController({
) => Promise<BN>;
} = {}) {
const preferences = new PreferencesController();
const onNetworkStateChangeListeners: ((state: NetworkState) => void)[] = [];
const onNetworkDidChangeListeners: ((state: NetworkState) => void)[] = [];
const changeNetwork = (providerConfig: ProviderConfig) => {
onNetworkStateChangeListeners.forEach((listener) => {
onNetworkDidChangeListeners.forEach((listener) => {
listener({
...defaultNetworkState,
providerConfig,
Expand Down Expand Up @@ -202,8 +202,8 @@ function setupController({
const assetsContract = new AssetsContractController({
chainId: ChainId.mainnet,
onPreferencesStateChange: (listener) => preferences.subscribe(listener),
onNetworkStateChange: (listener) =>
onNetworkStateChangeListeners.push(listener),
onNetworkDidChange: (listener) =>
onNetworkDidChangeListeners.push(listener),
getNetworkClientById: getNetworkClientByIdSpy,
});

Expand All @@ -222,7 +222,7 @@ function setupController({
chainId: ChainId.mainnet,
onPreferencesStateChange: (listener) => preferences.subscribe(listener),
onNetworkStateChange: (listener) =>
onNetworkStateChangeListeners.push(listener),
onNetworkDidChangeListeners.push(listener),
getERC721AssetName:
getERC721AssetNameStub ??
assetsContract.getERC721AssetName.bind(assetsContract),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ describe('NftDetectionController', () => {
let assetsContract: AssetsContractController;
let clock: sinon.SinonFakeTimers;
const networkStateChangeNoop = jest.fn();
const networkDidChangeNoop = jest.fn();
const getOpenSeaApiKeyStub = jest.fn();

const messenger = new ControllerMessenger<
Expand All @@ -40,7 +41,7 @@ describe('NftDetectionController', () => {
assetsContract = new AssetsContractController({
chainId: ChainId.mainnet,
onPreferencesStateChange: (listener) => preferences.subscribe(listener),
onNetworkStateChange: networkStateChangeNoop,
onNetworkDidChange: networkDidChangeNoop,
getNetworkClientById: jest.fn(),
});
const getNetworkClientById = jest.fn().mockImplementation(() => {
Expand Down
Loading

0 comments on commit 64d6166

Please sign in to comment.