Make chainId available in the metamask-inpage-provider #7110
Conversation
danjm
force-pushed
the
chain-id-inpage-provider
branch
from
b5dac37 to
1f80f3a
Compare
| @@ -0,0 +1,14 @@ | |||
| const standardNetworkId = { | |||
There was a problem hiding this comment.
Interesting - these seem to be hard-coded into app/scripts/controllers/network/createInfuraClient.js as well. Could be move them out into a shared location? Maybe app/scripts/lib/enums.js perhaps.
I don't know much about these ids - is there a reason the single-digit ids are padded with a zero? Do they all need the 0x prefix? I noticed that the fallback of parseInt doesn't add the 0x prefix, nor does it pad single digits.
There was a problem hiding this comment.
The EIP doesn't seem to mention anything about padding with 0 😕 The other example implementations don't seem to do that either.
I realize this was already done in createInfuraClient, but I don't understand why.
There was a problem hiding this comment.
Moved to enums.js in 16c83e140
I'm not going to touch the 0x prefix for now, and prefer to keep this event consistent with createInfuraClient. I don't want to change that file as there may be downstream dependencies on that format.
There was a problem hiding this comment.
It was the 0 padding on single-digits that contradicts the EIP, not the 0x prefix. The 0x prefix is correct (according to the EIP), it was just missing on the fallback. Sorry for the confusion - I should have made separate comments.
If I'm not mistaken, it looks like you've removed both the 0x prefix and the 0 padding on single-digit chain IDs.
There was a problem hiding this comment.
Just to be clear, I do think that this is supposed to return a string that is prefixed with 0x.
My interpretation of the EIP is that left-padding single-digit numbers with a single zero is not correct (though I'm not super confident in this). I'm guessing that we'd be better off not doing this, but I don't know what the implications would be of changing the behavior of createInfuraClient. I am concerned that that change would break things, but I'm equally concerned that adding the padding to a new public API would contradict the behavior of other wallets and the expectations of dapps.
There was a problem hiding this comment.
We could do 01f9910bb... and have the event return a different result from the method
There was a problem hiding this comment.
I think that's the least bad solution
There was a problem hiding this comment.
Yeah, maybe so. I'm not sure exactly how this new public API would be used, so I'm not sure whether it'd ever be assumed to match the result from the middleware.
There was a problem hiding this comment.
Yeah seems like this counts as a quantity and should therefore be hex prefixed with no 0 padding.
This API would be used for sites to detect a network change. chainId is used for transaction security, so it's much more meaningful to watch for changes than networkId.
There was a problem hiding this comment.
Right, that's what I thought the metamask-inpage-provider PR was for - the event upon chainId changing. I've since realized that that event comes from the publicConfig stream being updated here, so it all makes sense now.
I'm still not sure what we should do about the createInfuraClient middleware, but after learning a bit more about it, I at least don't think it'll conflict with this.
|
I guess we should bump |
danjm
force-pushed
the
chain-id-inpage-provider
branch
from
ffe3081 to
a208d5c
Compare
danjm
force-pushed
the
chain-id-inpage-provider
branch
2 times, most recently
from
9344d46 to
706c60b
Compare
danjm
force-pushed
the
chain-id-inpage-provider
branch
from
58fc0fc to
ea18463
Compare
This PR makes the current
chainIdavailable on the public state of the inpage provider.Together, this PR and MetaMask/providers#6 resolve #6885