Replace public config store with RPC methods/notifications #109
Conversation
rekmarks
force-pushed
the
delete-public-config
branch
2 times, most recently
from
0a8fc81
to
616d041
Compare
rekmarks
changed the title
|
How does this relate to the breaking changes? Did you just want to bundle it with them for the sake of avoiding another breaking change release, or is it functionally tied to them in some way? |
|
Ah, we're bundling this breaking change with all the rest just for the sake of convenience! |
* Delete public config store * Replace public config stream with notifications * Remove lock/unlock events; isUnlocked default false * Fix annotations, rename a function
rekmarks
force-pushed
the
delete-public-config
branch
from
616d041
to
b5f30cb
Compare
There was a problem hiding this comment.
Should we be using metamask_ here instead of wallet_? 🤔
I had thought we were, as a general rule, using metamask_ for methods that are internal-only or that we have no plans to standardize, and wallet_ for things that might be standardized at some point. These methods seem to me of internal relevance only, at least at the moment.
| @@ -18,7 +18,7 @@ async function sendSiteMetadata (engine, log) { | |||
| // call engine.handle directly to avoid normal RPC request handling | |||
| engine.handle( | |||
| { | |||
| method: 'wallet_sendDomainMetadata', | |||
| method: 'metamask_sendDomainMetadata', | |||
There was a problem hiding this comment.
This seems out of scope for this PR, and wallet_ might be the more appropriate prefix for this method anyway. We had talked about encouraging dapps to set their own metadata. So we might want to consider documenting this as-is.
There was a problem hiding this comment.
I always considered it to be kind of a hack, personally, and I think there's more work to be done on specifying the method before we commit to publishing its interface.
As to the out of scope, yeah kind of, but while we're here? 😅
There was a problem hiding this comment.
I think there's more work to be done on specifying the method before we commit to publishing its interface.
Makes sense. I hadn't really thought through whether it was ready to be public-facing in its current form.
It would have been better if this was metamask_ in the first place., but... it still seems better to leave it as is for now 🤷♂️ Technically dapps might already be sending this? Also the less provider changes we make, the less broken things with embedded providers will be (e.g. extensions, and dapps that import our provider as a workaround to that Firefox CSP bug). But I guess I won't block on it. Embedded providers will be fairly broken either way.
There was a problem hiding this comment.
Actually... maybe we can preserve the public config store in the extension to support old embedded providers, at least for a short while? 🤔 I don't know how feasible this is - I'll have to look more closely at the extension PRs.
| // this will get the exposed accounts, if any | ||
| try { | ||
| this._rpcRequest( | ||
| { method: 'eth_accounts', params: [] }, |
There was a problem hiding this comment.
Could we bundle the accounts with the unlock notification, to avoid this second call? It would simplify things a great deal. I think it would let us remove the entire isInternal parameter altogether, and that 'eth_accounts' unexpectedly updated accounts warning
There was a problem hiding this comment.
I guess this can wait for a subsequent PR either way though.
There was a problem hiding this comment.
Hm, this is actually very feasible now. Let's do that in a follow-up!
This event is deprecated, and is not part of EIP-1193. However we did not plan to remove this; it was removed accidentally as part of #109. It should now behave exactly as it did in v6.
This event is deprecated, and is not part of EIP-1193. However we did not plan to remove this; it was removed accidentally as part of #109. It should now behave exactly as it did in v6.
This event is deprecated, and is not part of EIP-1193. However we did not plan to remove this; it was removed accidentally as part of #109. It should now behave exactly as it did in v6.3.0. The text of the deprecation warning for this event has also been updated to provide a recommended alternative.
_publicConfigStore, andpublicConfigStoregetterwallet_getProviderStatewallet_unlockStateChangedwallet_chainChangedCorresponding PR in extension: MetaMask/metamask-extension#8640
CI: https://app.circleci.com/pipelines/github/MetaMask/inpage-provider?branch=delete-public-config