Remove deprecated properties, networkChanged event, and offline send() net_version support #306
Conversation
…dress. Remove send net_version. Remove networkChanged event
src/MetaMaskInpageProvider.ts
Outdated
| // Deprecated Properties | ||
| // Private Properties | ||
| //==================== | ||
|
|
||
| get chainId(): string | null { | ||
| if (!this._sentWarnings.chainId) { | ||
| this._log.warn(messages.warnings.chainIdDeprecation); | ||
| this._sentWarnings.chainId = true; | ||
| } | ||
| return super.chainId; | ||
| } | ||
|
|
||
| get networkVersion(): string | null { | ||
| if (!this._sentWarnings.networkVersion) { | ||
| this._log.warn(messages.warnings.networkVersionDeprecation); | ||
| this._sentWarnings.networkVersion = true; | ||
| } | ||
| return this.#networkVersion; | ||
| throw new Error(messages.errors.invalidPropertyChainId()); | ||
| } | ||
|
|
||
| get selectedAddress(): string | null { | ||
| if (!this._sentWarnings.selectedAddress) { | ||
| this._log.warn(messages.warnings.selectedAddressDeprecation); | ||
| this._sentWarnings.selectedAddress = true; | ||
| } | ||
| return super.selectedAddress; | ||
| throw new Error(messages.errors.invalidPropertySelectedAddress()); |
There was a problem hiding this comment.
These getter overrides must exist in order to prevent publicly exposing them via inheritance from the BaseProvider. We cannot just make those properties private in the BaseProvider themselves because the subclasses still need access to them.
There was a problem hiding this comment.
Could they be made protected, if anything?
There was a problem hiding this comment.
I tried making the BaseProvider getters protected and removing the overrides in MetaMaskInpageProvider, but the properties are still accessible. If i add protected to the MetaMaskInpageProvider getters, it does nothing and the properties are also still accessible
| }); | ||
| }); | ||
| }); | ||
|
|
||
| it('handles chain changes with intermittent disconnection', async () => { |
There was a problem hiding this comment.
we don't have networkId anymore, nor the notion of temporary disconnection/switching when networkId is set to loading
| if (networkVersion === 'loading') { | ||
| this._handleDisconnect(true); | ||
| } else { | ||
| super._handleChainChanged({ chainId }); | ||
| } |
There was a problem hiding this comment.
we don't have networkId anymore, nor the notion of temporary disconnection/switching when networkId is set to loading
|
Total sidebar here but
Wow looks like |
jiexi
changed the title
| return super.chainId; | ||
| } | ||
|
|
||
| get networkVersion(): string | null { |
There was a problem hiding this comment.
any reason we're not throwing a new Error(messages.errors.invalidPropertyNetworkVersion()) for this one?
There was a problem hiding this comment.
Mostly because networkVersion doesn't require a getter override, so it's possible to remove it completely without issue unlike the chainId and selectedAddress getters. I am also not throwing when networkChanged are listened to.
There was a problem hiding this comment.
Hmm, I agree. If we only throw for some of the things we've removed but not others, then couldn't it possibly be surprising?
There was a problem hiding this comment.
Throwing error for networkVersion here 10ee3a3
| @@ -62,11 +62,6 @@ export function initializeProvider({ | |||
| const proxiedProvider = new Proxy(provider, { | |||
There was a problem hiding this comment.
its not clear to me why this is a proxy, nor what the deleteProperty problem is.
When we use this in extension, the return value is never used, and I dont see anywhere that we are setting a new target, but maybe Im missing something. Heres the extension place im referring to.
There was a problem hiding this comment.
I believe it's a proxy to override deleteProperty: () => true,
…operties-events' into jl/mmp-2195/remove-deprecated-properties-events
This PR removes properties that are not officially documented and should not have been exposed in the first place. It also removes support for the networkChange event and offline
net_versionresolution which will allow us to remove the last remaining chunk of networkId usage in Extension. These have all been properly deprecated, the most recent deprecations being 180 days old now.Removes the following:
See: https://github.com/MetaMask/MetaMask-planning/issues/2195
See: MetaMask/metamask-improvement-proposals#23
See: https://github.com/MetaMask/MetaMask-planning/issues/2176
See: MetaMask/metamask-extension#23375
Before
After