Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: eth_accounts / accountsChanged behavior when wallet is locked #405

Merged
merged 8 commits into from
Feb 3, 2025
Merged

fix: eth_accounts / accountsChanged behavior when wallet is locked #405

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b379314
fix: remove handleUnlockStateChanged
ffmcgee725 Jan 31, 2025
5894170
chore: update jest.config.js
ffmcgee725 Feb 3, 2025
4c4aa4b
lint
ffmcgee725 Feb 3, 2025
bb8edc4
test: update jest.config.js
ffmcgee725 Feb 3, 2025
692a7ae
Merge remote-tracking branch 'origin' into fix/locked-wallet-behaviou…
adonesky1 Feb 3, 2025
e109526
remove more instances of isUnlocked
adonesky1 Feb 3, 2025
7877537
update jest.config
adonesky1 Feb 3, 2025
60bb239
test: add test case for isUnlocked in MetaMaskInpageProvider.test.ts
ffmcgee725 Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions jest.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,10 @@ const baseConfig = {
// An object that configures minimum threshold enforcement for coverage results
coverageThreshold: {
global: {
branches: 68.23,
functions: 68.69,
lines: 68.68,
statements: 68.71,
branches: 69.63,
functions: 71.42,
lines: 70.27,
statements: 70.4,
},
},

Expand Down
39 changes: 1 addition & 38 deletions src/BaseProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ export type RequestArguments = {
export type BaseProviderState = {
accounts: null | string[];
isConnected: boolean;
isUnlocked: boolean;
initialized: boolean;
isPermanentlyDisconnected: boolean;
};
Expand All @@ -77,7 +76,6 @@ export abstract class BaseProvider extends SafeEventEmitter {
protected static _defaultState: BaseProviderState = {
accounts: null,
isConnected: false,
isUnlocked: false,
initialized: false,
isPermanentlyDisconnected: false,
};
Expand Down Expand Up @@ -129,7 +127,6 @@ export abstract class BaseProvider extends SafeEventEmitter {
this._handleConnect = this._handleConnect.bind(this);
this._handleChainChanged = this._handleChainChanged.bind(this);
this._handleDisconnect = this._handleDisconnect.bind(this);
this._handleUnlockStateChanged = this._handleUnlockStateChanged.bind(this);
this._rpcRequest = this._rpcRequest.bind(this);
this.request = this.request.bind(this);

Expand Down Expand Up @@ -236,7 +233,6 @@ export abstract class BaseProvider extends SafeEventEmitter {
* @param initialState - The provider's initial state.
* @param initialState.accounts - The user's accounts.
* @param initialState.chainId - The chain ID.
* @param initialState.isUnlocked - Whether the user has unlocked MetaMask.
* @param initialState.networkVersion - The network version.
* @param initialState.isConnected - Whether the network is available.
* @fires BaseProvider#_initialized - If `initialState` is defined.
Expand All @@ -245,7 +241,6 @@ export abstract class BaseProvider extends SafeEventEmitter {
protected _initializeState(initialState?: {
accounts: string[];
chainId: string;
isUnlocked: boolean;
networkVersion?: string;
isConnected?: boolean;
}) {
Expand All @@ -254,13 +249,11 @@ export abstract class BaseProvider extends SafeEventEmitter {
}

if (initialState) {
const { accounts, chainId, isUnlocked, networkVersion, isConnected } =
initialState;
const { accounts, chainId, networkVersion, isConnected } = initialState;

// EIP-1193 connect
this._handleConnect({ chainId, isConnected });
this._handleChainChanged({ chainId, networkVersion, isConnected });
this._handleUnlockStateChanged({ accounts, isUnlocked });
Copy link
Contributor

@jiexi jiexi Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not confident we initialize the value of isUnlocked to true without this

Copy link
Member Author

@ffmcgee725 ffmcgee725 Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meaning we should still get isUnlocked from deconstructing initial state

      const { accounts, chainId, isUnlocked, networkVersion } = initialState;

and set it in this function

this._state.isUnlocked = isUnlocked;

Or just default to true ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems it's not necessary to set this after df0acbf

this._handleAccountsChanged(accounts);
}

Expand Down Expand Up @@ -367,7 +360,6 @@ export abstract class BaseProvider extends SafeEventEmitter {
this.#chainId = null;
this._state.accounts = null;
this.#selectedAddress = null;
this._state.isUnlocked = false;
this._state.isPermanentlyDisconnected = true;
}

Expand Down Expand Up @@ -472,33 +464,4 @@ export abstract class BaseProvider extends SafeEventEmitter {
}
}
}

/**
* Upon receipt of a new isUnlocked state, sets relevant public state.
* Calls the accounts changed handler with the received accounts, or an empty
* array.
*
* Does nothing if the received value is equal to the existing value.
* There are no lock/unlock events.
*
* @param opts - Options bag.
* @param opts.accounts - The exposed accounts, if any.
* @param opts.isUnlocked - The latest isUnlocked value.
*/
protected _handleUnlockStateChanged({
accounts,
isUnlocked,
}: { accounts?: string[]; isUnlocked?: boolean } = {}) {
if (typeof isUnlocked !== 'boolean') {
this._log.error(
'MetaMask: Received invalid isUnlocked parameter. Please report this bug.',
);
return;
}

if (isUnlocked !== this._state.isUnlocked) {
this._state.isUnlocked = isUnlocked;
this._handleAccountsChanged(accounts ?? []);
}
}
}
24 changes: 21 additions & 3 deletions src/MetaMaskInpageProvider.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ async function getInitializedProvider({
initialState: {
accounts = [],
chainId = '0x0',
isUnlocked = true,
networkVersion = '0',
isConnected = true,
} = {},
Expand Down Expand Up @@ -81,7 +80,6 @@ async function getInitializedProvider({
result: {
accounts,
chainId,
isUnlocked,
networkVersion,
isConnected,
},
Expand Down Expand Up @@ -1114,7 +1112,6 @@ describe('MetaMaskInpageProvider: Miscellanea', () => {
return {
accounts: ['0xabc'],
chainId: '0x0',
isUnlocked: true,
networkVersion: '0',
isConnected: true,
};
Expand Down Expand Up @@ -1221,4 +1218,25 @@ describe('MetaMaskInpageProvider: Miscellanea', () => {
expect(provider.selectedAddress).toBe('0xdeadbeef');
});
});

describe('_getExperimentalApi', () => {
let provider: any | MetaMaskInpageProvider;

beforeEach(async () => {
provider = (
await getInitializedProvider({
initialState: {
accounts: ['0xdeadbeef'],
},
})
).provider;
});

describe('isUnlocked', () => {
it('should return negated value of `state.isPermanentlyDisconnected`', async () => {
const isUnlocked = await provider._getExperimentalApi().isUnlocked();
expect(isUnlocked).toBe(!provider._state.isPermanentlyDisconnected);
});
});
});
});
9 changes: 1 addition & 8 deletions src/MetaMaskInpageProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,14 +409,7 @@ export class MetaMaskInpageProvider extends AbstractStreamProvider {
*
* @returns Promise resolving to true if MetaMask is currently unlocked.
*/
isUnlocked: async () => {
if (!this._state.initialized) {
await new Promise<void>((resolve) => {
this.on('_initialized', () => resolve());
});
}
return this._state.isUnlocked;
},
isUnlocked: async () => !this._state.isPermanentlyDisconnected,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Would be nice to have a unit test for this, I don't see one. I know it wasn't covered previously (so it seems), but it looks like it would be easy to add. And coverage is decent for the rest of the file otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm finding it quite difficult to simulate the unrecoverable disconnection event to actually toggle the isPermanentlyDisconnected state... any ideas on how to achieve this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved in 60bb239

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adonesky1 seems not much has to be done since the state isPermanentlyDisconnected initializes to false on BaseProvider parent class, so we just assert the return is it's negated value


/**
* Make a batch RPC request.
Expand Down
6 changes: 0 additions & 6 deletions src/StreamProvider.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ describe('StreamProvider', () => {
const accounts = ['0xabc'];
const chainId = '0x1';
const networkVersion = '1';
const isUnlocked = true;
const isConnected = true;

const streamProvider = new StreamProvider(new MockConnectionStream());
Expand All @@ -49,7 +48,6 @@ describe('StreamProvider', () => {
return {
accounts,
chainId,
isUnlocked,
networkVersion,
isConnected,
};
Expand All @@ -71,7 +69,6 @@ describe('StreamProvider', () => {
const accounts = ['0xabc'];
const chainId = '0x1';
const networkVersion = '1';
const isUnlocked = true;
const isConnected = true;

const streamProvider = new StreamProvider(new MockConnectionStream());
Expand All @@ -80,7 +77,6 @@ describe('StreamProvider', () => {
return {
accounts,
chainId,
isUnlocked,
networkVersion,
isConnected,
};
Expand Down Expand Up @@ -411,7 +407,6 @@ describe('StreamProvider', () => {
return {
accounts: [],
chainId: '0x0',
isUnlocked: true,
networkVersion: '0',
isConnected: true,
};
Expand Down Expand Up @@ -450,7 +445,6 @@ describe('StreamProvider', () => {
return {
accounts: [],
chainId: '0x0',
isUnlocked: true,
networkVersion: '0',
isConnected: true,
};
Expand Down
2 changes: 0 additions & 2 deletions src/StreamProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,6 @@ export abstract class AbstractStreamProvider extends BaseProvider {
const { method, params } = payload;
if (method === 'metamask_accountsChanged') {
this._handleAccountsChanged(params);
} else if (method === 'metamask_unlockStateChanged') {
this._handleUnlockStateChanged(params);
} else if (method === 'metamask_chainChanged') {
this._handleChainChanged(params);
} else if (EMITTED_NOTIFICATIONS.includes(method)) {
Expand Down
2 changes: 0 additions & 2 deletions src/extension-provider/createExternalExtensionProvider.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ async function getInitializedProvider({
initialState: {
accounts = [],
chainId = '0x0',
isUnlocked = true,
networkVersion = '0',
isConnected = true,
} = {},
Expand Down Expand Up @@ -72,7 +71,6 @@ async function getInitializedProvider({
result: {
accounts,
chainId,
isUnlocked,
networkVersion,
isConnected,
},
Expand Down
Loading