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

Make chainId available in the metamask-inpage-provider #7110

Merged
merged 6 commits into from
Sep 10, 2019
Merged

Make chainId available in the metamask-inpage-provider #7110

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1f80f3a
Make chainId available in the metamask-inpage-provider
danjm Sep 5, 2019
22326c4
Update metamask-inpage-provider to 2.1.0
danjm Sep 9, 2019
a208d5c
Add e2e tests for ethereum.on events
danjm Sep 9, 2019
eb8b3d7
Move chainId constants to lib/enums.js
danjm Sep 9, 2019
4b06051
Don't use new chainId enums in createInfuraClient
danjm Sep 9, 2019
ea18463
Fix app/scripts/lib/select-chain-id.js
danjm Sep 9, 2019
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
14 changes: 14 additions & 0 deletions app/scripts/lib/select-chain-id.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
const standardNetworkId = {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@Gudahtt Gudahtt Sep 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@Gudahtt Gudahtt Sep 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do 01f9910bb... and have the event return a different result from the method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's the least bad solution

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

'1': '0x01',
'3': '0x03',
'4': '0x04',
'42': '0x2a',
'5': '0x05',
}

function selectChainId (metamaskState) {
const { network, provider: { chaindId } } = metamaskState
return standardNetworkId[network] || parseInt(chaindId, 10).toString(16)
Gudahtt marked this conversation as resolved.
Show resolved Hide resolved
}

module.exports = selectChainId
4 changes: 3 additions & 1 deletion app/scripts/metamask-controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ const ProviderApprovalController = require('./controllers/provider-approval')
const nodeify = require('./lib/nodeify')
const accountImporter = require('./account-import-strategies')
const getBuyEthUrl = require('./lib/buy-eth-url')
const selectChainId = require('./lib/select-chain-id')
const {Mutex} = require('await-semaphore')
const {version} = require('../manifest.json')
const {BN} = require('ethereumjs-util')
Expand Down Expand Up @@ -360,7 +361,7 @@ module.exports = class MetamaskController extends EventEmitter {
publicConfigStore.putState(publicState)
}

function selectPublicState ({ isUnlocked, selectedAddress, network, completedOnboarding }) {
function selectPublicState ({ isUnlocked, selectedAddress, network, completedOnboarding, provider }) {
const isEnabled = checkIsEnabled()
const isReady = isUnlocked && isEnabled
const result = {
Expand All @@ -369,6 +370,7 @@ module.exports = class MetamaskController extends EventEmitter {
selectedAddress: isReady ? selectedAddress : undefined,
networkVersion: network,
onboardingcomplete: completedOnboarding,
chainId: selectChainId({ network, provider }),
}
return result
}
Expand Down