Skip to content

Commit

Permalink
Initialize asset controllers with the current network (#1361)
Browse files Browse the repository at this point in the history
* Initialize asset controllers with the current network

Certain controllers in `@metamask/assets-controllers` would default to
assuming the current selected network was Ethereum Mainnet, or they'd
default with an empty string as the `chainId` (causing errors if any
methods were called). Instead they now all get initialized with the
current chain ID, passed in as a new constructor parameter.

Affected controllers are:
* `AssetsContractController`
* `NftController`
* `NftDetectionController`
* `TokenRatesController`
* `TokensController`

Relates to #1209

* Fix more tests
  • Loading branch information
Gudahtt authored and MajorLift committed Oct 11, 2023
1 parent b49edf4 commit 7e04cb1
Show file tree
Hide file tree
Showing 12 changed files with 57 additions and 14 deletions.
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import HttpProvider from 'ethjs-provider-http';
import {
ChainId,
IPFS_DEFAULT_GATEWAY_URL,
NetworkType,
} from '@metamask/controller-utils';
Expand Down Expand Up @@ -45,6 +46,7 @@ const setupControllers = () => {
});
const preferences = new PreferencesController();
const assetsContract = new AssetsContractController({
chainId: ChainId.mainnet,
onPreferencesStateChange: (listener) => preferences.subscribe(listener),
onNetworkStateChange: (listener) =>
messenger.subscribe('NetworkController:stateChange', listener),
Expand Down
5 changes: 4 additions & 1 deletion packages/assets-controllers/src/AssetsContractController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,16 +83,19 @@ export class AssetsContractController extends BaseController<
* Creates a AssetsContractController instance.
*
* @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 config - Initial options used to configure this controller.
* @param state - Initial state to set on this controller.
*/
constructor(
{
chainId: initialChainId,
onPreferencesStateChange,
onNetworkStateChange,
}: {
chainId: string;
onPreferencesStateChange: (
listener: (preferencesState: PreferencesState) => void,
) => void;
Expand All @@ -107,7 +110,7 @@ export class AssetsContractController extends BaseController<
this.defaultConfig = {
provider: undefined,
ipfsGateway: IPFS_DEFAULT_GATEWAY_URL,
chainId: SupportedTokenDetectionNetworks.mainnet,
chainId: initialChainId,
};
this.initialize();

Expand Down
2 changes: 2 additions & 0 deletions packages/assets-controllers/src/NftController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,13 +89,15 @@ function setupController({
};

const assetsContract = new AssetsContractController({
chainId: ChainId.mainnet,
onPreferencesStateChange: (listener) => preferences.subscribe(listener),
onNetworkStateChange: (listener) =>
onNetworkStateChangeListeners.push(listener),
});
const onNftAddedSpy = includeOnNftAdded ? jest.fn() : undefined;

const nftController = new NftController({
chainId: ChainId.mainnet,
onPreferencesStateChange: (listener) => preferences.subscribe(listener),
onNetworkStateChange: (listener) =>
onNetworkStateChangeListeners.push(listener),
Expand Down
5 changes: 4 additions & 1 deletion packages/assets-controllers/src/NftController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -857,6 +857,7 @@ export class NftController extends BaseController<NftConfig, NftState> {
* Creates an NftController instance.
*
* @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.getERC721AssetName - Gets the name of the asset at the given address.
Expand All @@ -872,6 +873,7 @@ export class NftController extends BaseController<NftConfig, NftState> {
*/
constructor(
{
chainId: initialChainId,
onPreferencesStateChange,
onNetworkStateChange,
getERC721AssetName,
Expand All @@ -882,6 +884,7 @@ export class NftController extends BaseController<NftConfig, NftState> {
getERC1155TokenURI,
onNftAdded,
}: {
chainId: string;
onPreferencesStateChange: (
listener: (preferencesState: PreferencesState) => void,
) => void;
Expand All @@ -908,7 +911,7 @@ export class NftController extends BaseController<NftConfig, NftState> {
super(config, state);
this.defaultConfig = {
selectedAddress: '',
chainId: '',
chainId: initialChainId,
ipfsGateway: IPFS_DEFAULT_GATEWAY_URL,
openSeaEnabled: false,
useIPFSSubdomains: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,13 @@ describe('NftDetectionController', () => {
beforeEach(async () => {
preferences = new PreferencesController();
assetsContract = new AssetsContractController({
chainId: ChainId.mainnet,
onPreferencesStateChange: (listener) => preferences.subscribe(listener),
onNetworkStateChange: networkStateChangeNoop,
});

nftController = new NftController({
chainId: ChainId.mainnet,
onPreferencesStateChange: (listener) => preferences.subscribe(listener),
onNetworkStateChange: networkStateChangeNoop,
getERC721AssetName:
Expand All @@ -47,6 +49,7 @@ describe('NftDetectionController', () => {
});

nftDetection = new NftDetectionController({
chainId: ChainId.mainnet,
onNftsStateChange: (listener) => nftController.subscribe(listener),
onPreferencesStateChange: (listener) => preferences.subscribe(listener),
onNetworkStateChange: networkStateChangeNoop,
Expand All @@ -55,7 +58,7 @@ describe('NftDetectionController', () => {
getNftState: () => nftController.state,
});

nftController.configure({ chainId: '1', selectedAddress: '0x1' });
nftController.configure({ selectedAddress: '0x1' });
preferences.setOpenSeaEnabled(true);
preferences.setUseNftDetection(true);

Expand Down Expand Up @@ -212,6 +215,7 @@ describe('NftDetectionController', () => {
);
const nftsDetectionController = new NftDetectionController(
{
chainId: ChainId.mainnet,
onNftsStateChange: (listener) => nftController.subscribe(listener),
onPreferencesStateChange: (listener) =>
preferences.subscribe(listener),
Expand Down Expand Up @@ -247,6 +251,7 @@ describe('NftDetectionController', () => {
);
new NftDetectionController(
{
chainId: ChainId.goerli,
onNftsStateChange: (listener) => nftController.subscribe(listener),
onPreferencesStateChange: (listener) =>
preferences.subscribe(listener),
Expand Down
9 changes: 6 additions & 3 deletions packages/assets-controllers/src/NftDetectionController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ export interface ApiNftCreator {
*/
export interface NftDetectionConfig extends BaseConfig {
interval: number;
chainId: `0x${string}` | `${number}` | number;
chainId: string;
selectedAddress: string;
}

Expand Down Expand Up @@ -204,6 +204,7 @@ export class NftDetectionController extends BaseController<
* Creates an NftDetectionController instance.
*
* @param options - The controller options.
* @param options.chainId - The chain ID of the current network.
* @param options.onNftsStateChange - Allows subscribing to assets controller state changes.
* @param options.onPreferencesStateChange - Allows subscribing to preferences controller state changes.
* @param options.onNetworkStateChange - Allows subscribing to network controller state changes.
Expand All @@ -215,12 +216,14 @@ export class NftDetectionController extends BaseController<
*/
constructor(
{
chainId: initialChainId,
onPreferencesStateChange,
onNetworkStateChange,
getOpenSeaApiKey,
addNft,
getNftState,
}: {
chainId: string;
onNftsStateChange: (listener: (nftsState: NftState) => void) => void;
onPreferencesStateChange: (
listener: (preferencesState: PreferencesState) => void,
Expand All @@ -238,7 +241,7 @@ export class NftDetectionController extends BaseController<
super(config, state);
this.defaultConfig = {
interval: DEFAULT_INTERVAL,
chainId: '1',
chainId: initialChainId,
selectedAddress: '',
disabled: true,
};
Expand Down Expand Up @@ -266,7 +269,7 @@ export class NftDetectionController extends BaseController<

onNetworkStateChange(({ providerConfig }) => {
this.configure({
chainId: providerConfig.chainId as NftDetectionConfig['chainId'],
chainId: providerConfig.chainId,
});
});
this.getOpenSeaApiKey = getOpenSeaApiKey;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ describe('TokenBalancesController', () => {
it('should update all balances', async () => {
const { messenger, preferences } = setupControllers();
const assets = new TokensController({
chainId: '1',
onPreferencesStateChange: (listener) => preferences.subscribe(listener),
onNetworkStateChange: (listener) =>
messenger.subscribe('NetworkController:stateChange', listener),
Expand Down Expand Up @@ -178,6 +179,7 @@ describe('TokenBalancesController', () => {
it('should handle `getERC20BalanceOf` error case', async () => {
const { messenger, preferences } = setupControllers();
const assets = new TokensController({
chainId: '1',
onPreferencesStateChange: (listener) => preferences.subscribe(listener),
onNetworkStateChange: (listener) =>
messenger.subscribe('NetworkController:stateChange', listener),
Expand Down Expand Up @@ -226,11 +228,13 @@ describe('TokenBalancesController', () => {
it('should subscribe to new sibling assets controllers', async () => {
const { messenger, preferences } = setupControllers();
const assetsContract = new AssetsContractController({
chainId: '1',
onPreferencesStateChange: (listener) => preferences.subscribe(listener),
onNetworkStateChange: (listener) =>
messenger.subscribe('NetworkController:stateChange', listener),
});
const tokensController = new TokensController({
chainId: '1',
onPreferencesStateChange: (listener) => preferences.subscribe(listener),
onNetworkStateChange: (listener) =>
messenger.subscribe('NetworkController:stateChange', listener),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ describe('TokenDetectionController', () => {
.callsFake(() => null);

tokensController = new TokensController({
chainId: '1',
onPreferencesStateChange: (listener) => preferences.subscribe(listener),
onNetworkStateChange: (listener) =>
onNetworkStateChangeListeners.push(listener),
Expand Down
24 changes: 19 additions & 5 deletions packages/assets-controllers/src/TokenRatesController.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ describe('TokenRatesController', () => {

it('should set default state', () => {
const controller = new TokenRatesController({
chainId: '1',
onTokensStateChange: sinon.stub(),
onCurrencyRateStateChange: sinon.stub(),
onNetworkStateChange: sinon.stub(),
Expand All @@ -129,6 +130,7 @@ describe('TokenRatesController', () => {

it('should initialize with the default config', () => {
const controller = new TokenRatesController({
chainId: '1',
onTokensStateChange: sinon.stub(),
onCurrencyRateStateChange: sinon.stub(),
onNetworkStateChange: sinon.stub(),
Expand All @@ -137,14 +139,15 @@ describe('TokenRatesController', () => {
disabled: false,
interval: 180000,
nativeCurrency: 'eth',
chainId: '',
chainId: '1',
tokens: [],
threshold: 21600000,
});
});

it('should throw when tokens property is accessed', () => {
const controller = new TokenRatesController({
chainId: '1',
onTokensStateChange: sinon.stub(),
onCurrencyRateStateChange: sinon.stub(),
onNetworkStateChange: sinon.stub(),
Expand All @@ -160,6 +163,7 @@ describe('TokenRatesController', () => {
const times = 5;
new TokenRatesController(
{
chainId: '1',
onTokensStateChange: jest.fn(),
onCurrencyRateStateChange: jest.fn(),
onNetworkStateChange: jest.fn(),
Expand All @@ -182,6 +186,7 @@ describe('TokenRatesController', () => {
it('should not update rates if disabled', async () => {
const controller = new TokenRatesController(
{
chainId: '1',
onTokensStateChange: sinon.stub(),
onCurrencyRateStateChange: sinon.stub(),
onNetworkStateChange: sinon.stub(),
Expand All @@ -200,6 +205,7 @@ describe('TokenRatesController', () => {
const mock = sinon.stub(global, 'clearTimeout');
const controller = new TokenRatesController(
{
chainId: '1',
onTokensStateChange: sinon.stub(),
onCurrencyRateStateChange: sinon.stub(),
onNetworkStateChange: sinon.stub(),
Expand All @@ -223,19 +229,21 @@ describe('TokenRatesController', () => {
});
const preferences = new PreferencesController();
const tokensController = new TokensController({
chainId: '1',
onPreferencesStateChange: (listener) => preferences.subscribe(listener),
onNetworkStateChange: (listener) =>
messenger.subscribe('NetworkController:stateChange', listener),
messenger: undefined as unknown as TokensControllerMessenger,
});
const controller = new TokenRatesController(
{
chainId: '1',
onTokensStateChange: (listener) => tokensController.subscribe(listener),
onCurrencyRateStateChange: sinon.stub(),
onNetworkStateChange: (listener) =>
messenger.subscribe('NetworkController:stateChange', listener),
},
{ interval: 10, chainId: '1' },
{ interval: 10 },
);
const address = '0x89d24A6b4CcB1B6fAA2625fE562bDD9a23260359';
expect(controller.state.contractExchangeRates).toStrictEqual({});
Expand All @@ -257,6 +265,7 @@ describe('TokenRatesController', () => {
it('should handle balance not found in API', async () => {
const controller = new TokenRatesController(
{
chainId: '1',
onTokensStateChange: sinon.stub(),
onCurrencyRateStateChange: sinon.stub(),
onNetworkStateChange: sinon.stub(),
Expand Down Expand Up @@ -285,6 +294,7 @@ describe('TokenRatesController', () => {
const onNetworkStateChange = sinon.stub();
const controller = new TokenRatesController(
{
chainId: '1',
onTokensStateChange,
onCurrencyRateStateChange,
onNetworkStateChange,
Expand All @@ -311,6 +321,7 @@ describe('TokenRatesController', () => {
const onNetworkStateChange = sinon.stub();
const controller = new TokenRatesController(
{
chainId: '1',
onTokensStateChange,
onCurrencyRateStateChange,
onNetworkStateChange,
Expand Down Expand Up @@ -360,13 +371,14 @@ describe('TokenRatesController', () => {
const onNetworkStateChange = sinon.stub();
const controller = new TokenRatesController(
{
chainId: '137',
onTokensStateChange,
onCurrencyRateStateChange: sinon.stub(),
onNetworkStateChange,
},
{ interval: 10 },
);
await controller.configure({ chainId: '137', nativeCurrency: 'MATIC' });
await controller.configure({ nativeCurrency: 'MATIC' });

// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
await tokenStateChangeListener!({
Expand Down Expand Up @@ -422,14 +434,15 @@ describe('TokenRatesController', () => {

const controller = new TokenRatesController(
{
chainId: '1',
onTokensStateChange,
onNetworkStateChange,
onCurrencyRateStateChange: sinon.stub(),
},
{ interval: 10 },
);

await controller.configure({ chainId: '1', nativeCurrency: 'ETH' });
await controller.configure({ nativeCurrency: 'ETH' });

// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
await tokenStateChangeListener!({
Expand Down Expand Up @@ -494,11 +507,12 @@ describe('TokenRatesController', () => {

const controller = new TokenRatesController(
{
chainId: '1',
onTokensStateChange,
onNetworkStateChange: sinon.stub(),
onCurrencyRateStateChange: sinon.stub(),
},
{ interval: 10, chainId: '1', nativeCurrency: 'ETH' },
{ interval: 10, nativeCurrency: 'ETH' },
);

expect(controller.state.contractExchangeRates).toStrictEqual({});
Expand Down
Loading

0 comments on commit 7e04cb1

Please sign in to comment.