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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 11 additions & 0 deletions app/scripts/lib/enums.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@ const PLATFORM_EDGE = 'Edge'
const PLATFORM_FIREFOX = 'Firefox'
const PLATFORM_OPERA = 'Opera'

const MAINNET_CHAIN_ID = '0x1'
const ROPSTEN_CHAIN_ID = '0x3'
const RINKEBY_CHAIN_ID = '0x4'
const KOVAN_CHAIN_ID = '0x2a'
const GOERLI_CHAIN_ID = '0x5'

module.exports = {
ENVIRONMENT_TYPE_POPUP,
ENVIRONMENT_TYPE_NOTIFICATION,
Expand All @@ -19,4 +25,9 @@ module.exports = {
PLATFORM_EDGE,
PLATFORM_FIREFOX,
PLATFORM_OPERA,
MAINNET_CHAIN_ID,
ROPSTEN_CHAIN_ID,
RINKEBY_CHAIN_ID,
KOVAN_CHAIN_ID,
GOERLI_CHAIN_ID,
}
22 changes: 22 additions & 0 deletions app/scripts/lib/select-chain-id.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
const {
MAINNET_CHAIN_ID,
ROPSTEN_CHAIN_ID,
RINKEBY_CHAIN_ID,
KOVAN_CHAIN_ID,
GOERLI_CHAIN_ID,
} = require('./enums')

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

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

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': MAINNET_CHAIN_ID,
'3': ROPSTEN_CHAIN_ID,
'4': RINKEBY_CHAIN_ID,
'42': KOVAN_CHAIN_ID,
'5': GOERLI_CHAIN_ID,
}

function selectChainId (metamaskState) {
const { network, provider: { chaindId } } = metamaskState
return standardNetworkId[network] || `0x${parseInt(chaindId, 10).toString(16)}`
}

module.exports = selectChainId
4 changes: 3 additions & 1 deletion app/scripts/metamask-controller.js
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
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@
"lodash.shuffle": "^4.2.0",
"loglevel": "^1.4.1",
"luxon": "^1.8.2",
"metamask-inpage-provider": "^2.0.3",
"metamask-inpage-provider": "^2.1.0",
"metamask-logo": "^2.1.4",
"mkdirp": "^0.5.1",
"multihashes": "^0.4.12",
Expand Down
20 changes: 20 additions & 0 deletions test/e2e/contract-test/contract.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions test/e2e/contract-test/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@
<button id="approveTokensWithoutGas">Approve Tokens Without Gas</button>
</div>
</div>
<div style="display: flex; flex-flow: column;">
<div>Network: <div id="network"></div></div>
<div>ChainId: <div id="chainId"></div></div>
<div>Accounts: <div id="accounts"></div></div>
<div style="display: flex;">
</div>

<script src="contract.js"></script>
</body>
Expand Down
229 changes: 229 additions & 0 deletions test/e2e/ethereum-on.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,229 @@
const path = require('path')
const assert = require('assert')
const webdriver = require('selenium-webdriver')
const { By, until } = webdriver
const {
delay,
buildChromeWebDriver,
buildFirefoxWebdriver,
installWebExt,
getExtensionIdChrome,
getExtensionIdFirefox,
} = require('./func')
const {
checkBrowserForConsoleErrors,
closeAllWindowHandlesExcept,
findElement,
findElements,
openNewPage,
verboseReportOnFailure,
waitUntilXWindowHandles,
switchToWindowWithTitle,
} = require('./helpers')
const fetchMockResponses = require('./fetch-mocks.js')

describe('MetaMask', function () {
let extensionId
let driver
let publicAddress

const tinyDelayMs = 200
const regularDelayMs = tinyDelayMs * 2
const largeDelayMs = regularDelayMs * 2

this.timeout(0)
this.bail(true)

before(async function () {
let extensionUrl
switch (process.env.SELENIUM_BROWSER) {
case 'chrome': {
const extPath = path.resolve('dist/chrome')
driver = buildChromeWebDriver(extPath)
extensionId = await getExtensionIdChrome(driver)
await delay(largeDelayMs)
extensionUrl = `chrome-extension://${extensionId}/home.html`
break
}
case 'firefox': {
const extPath = path.resolve('dist/firefox')
driver = buildFirefoxWebdriver()
await installWebExt(driver, extPath)
await delay(largeDelayMs)
extensionId = await getExtensionIdFirefox(driver)
extensionUrl = `moz-extension://${extensionId}/home.html`
break
}
}
// Depending on the state of the application built into the above directory (extPath) and the value of
// METAMASK_DEBUG we will see different post-install behaviour and possibly some extra windows. Here we
// are closing any extraneous windows to reset us to a single window before continuing.
const [tab1] = await driver.getAllWindowHandles()
await closeAllWindowHandlesExcept(driver, [tab1])
await driver.switchTo().window(tab1)
await driver.get(extensionUrl)
})

beforeEach(async function () {
await driver.executeScript(
'window.origFetch = window.fetch.bind(window);' +
'window.fetch = ' +
'(...args) => { ' +
'if (args[0] === "https://ethgasstation.info/json/ethgasAPI.json") { return ' +
'Promise.resolve({ json: () => Promise.resolve(JSON.parse(\'' + fetchMockResponses.ethGasBasic + '\')) }); } else if ' +
'(args[0] === "https://ethgasstation.info/json/predictTable.json") { return ' +
'Promise.resolve({ json: () => Promise.resolve(JSON.parse(\'' + fetchMockResponses.ethGasPredictTable + '\')) }); } else if ' +
'(args[0].match(/chromeextensionmm/)) { return ' +
'Promise.resolve({ json: () => Promise.resolve(JSON.parse(\'' + fetchMockResponses.metametrics + '\')) }); } else if ' +
'(args[0] === "https://dev.blockscale.net/api/gasexpress.json") { return ' +
'Promise.resolve({ json: () => Promise.resolve(JSON.parse(\'' + fetchMockResponses.gasExpress + '\')) }); } ' +
'return window.origFetch(...args); };' +
'function cancelInfuraRequest(requestDetails) {' +
'console.log("Canceling: " + requestDetails.url);' +
'return {' +
'cancel: true' +
'};' +
' }' +
'window.chrome && window.chrome.webRequest && window.chrome.webRequest.onBeforeRequest.addListener(' +
'cancelInfuraRequest,' +
'{urls: ["https://*.infura.io/*"]},' +
'["blocking"]' +
');'
)
})

afterEach(async function () {
if (process.env.SELENIUM_BROWSER === 'chrome') {
const errors = await checkBrowserForConsoleErrors(driver)
if (errors.length) {
const errorReports = errors.map(err => err.message)
const errorMessage = `Errors found in browser console:\n${errorReports.join('\n')}`
console.error(new Error(errorMessage))
}
}
if (this.currentTest.state === 'failed') {
await verboseReportOnFailure(driver, this.currentTest)
}
})

after(async function () {
await driver.quit()
})

describe('Going through the first time flow, but skipping the seed phrase challenge', () => {
it('clicks the continue button on the welcome screen', async () => {
await findElement(driver, By.css('.welcome-page__header'))
const welcomeScreenBtn = await findElement(driver, By.css('.first-time-flow__button'))
welcomeScreenBtn.click()
await delay(largeDelayMs)
})

it('clicks the "Create New Wallet" option', async () => {
const customRpcButton = await findElement(driver, By.xpath(`//button[contains(text(), 'Create a Wallet')]`))
customRpcButton.click()
await delay(largeDelayMs)
})

it('clicks the "No thanks" option on the metametrics opt-in screen', async () => {
const optOutButton = await findElement(driver, By.css('.btn-default'))
optOutButton.click()
await delay(largeDelayMs)
})

it('accepts a secure password', async () => {
const passwordBox = await findElement(driver, By.css('.first-time-flow__form #create-password'))
const passwordBoxConfirm = await findElement(driver, By.css('.first-time-flow__form #confirm-password'))
const button = await findElement(driver, By.css('.first-time-flow__form button'))

await passwordBox.sendKeys('correct horse battery staple')
await passwordBoxConfirm.sendKeys('correct horse battery staple')

const tosCheckBox = await findElement(driver, By.css('.first-time-flow__checkbox'))
await tosCheckBox.click()

await button.click()
await delay(largeDelayMs)
})

it('skips the seed phrase challenge', async () => {
const buttons = await findElements(driver, By.css('.first-time-flow__button'))
await buttons[0].click()
await delay(regularDelayMs)

const detailsButton = await findElement(driver, By.css('.account-details__details-button'))
await detailsButton.click()
await delay(regularDelayMs)
})

it('gets the current accounts address', async () => {
const addressInput = await findElement(driver, By.css('.qr-ellip-address'))
publicAddress = await addressInput.getAttribute('value')
const accountModal = await driver.findElement(By.css('span .modal'))

await driver.executeScript("document.querySelector('.account-modal-close').click()")

await driver.wait(until.stalenessOf(accountModal))
await delay(regularDelayMs)
})

})

describe('provider listening for events', () => {
let extension
let popup
let dapp
it('switches to a dapp', async () => {
await openNewPage(driver, 'http://127.0.0.1:8080/')
await delay(regularDelayMs)

await waitUntilXWindowHandles(driver, 3)
const windowHandles = await driver.getAllWindowHandles()

extension = windowHandles[0]
popup = await switchToWindowWithTitle(driver, 'MetaMask Notification', windowHandles)
dapp = windowHandles.find(handle => handle !== extension && handle !== popup)

await delay(regularDelayMs)
const approveButton = await findElement(driver, By.xpath(`//button[contains(text(), 'Connect')]`))
await approveButton.click()

await driver.switchTo().window(dapp)
await delay(regularDelayMs)
})

it('has not set the network within the dapp', async () => {
const networkDiv = await findElement(driver, By.css('#network'))
assert.equal(await networkDiv.getText(), '')
})

it('changes the network', async () => {
await driver.switchTo().window(extension)

const networkDropdown = await findElement(driver, By.css('.network-name'))
await networkDropdown.click()
await delay(regularDelayMs)

const ropstenButton = await findElement(driver, By.xpath(`//span[contains(text(), 'Ropsten')]`))
await ropstenButton.click()
await delay(largeDelayMs)
})

it('sets the network div within the dapp', async () => {
await driver.switchTo().window(dapp)
const networkDiv = await findElement(driver, By.css('#network'))
assert.equal(await networkDiv.getText(), '3')
})

it('sets the chainId div within the dapp', async () => {
await driver.switchTo().window(dapp)
const chainIdDiv = await findElement(driver, By.css('#chainId'))
assert.equal(await chainIdDiv.getText(), '0x3')
})

it('sets the account div within the dapp', async () => {
await driver.switchTo().window(dapp)
const accountsDiv = await findElement(driver, By.css('#accounts'))
assert.equal(await accountsDiv.getText(), publicAddress.toLowerCase())
})
})
})
8 changes: 8 additions & 0 deletions test/e2e/run-all.sh
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,14 @@ concurrently --kill-others \
'sleep 5 && mocha test/e2e/send-edit.spec'


concurrently --kill-others \
--names 'ganache,dapp,e2e' \
--prefix '[{time}][{name}]' \
--success first \
'yarn ganache:start' \
'yarn dapp' \
'sleep 5 && mocha test/e2e/ethereum-on.spec'

export GANACHE_ARGS="${BASE_GANACHE_ARGS} --deterministic --account=0x250F458997A364988956409A164BA4E16F0F99F916ACDD73ADCD3A1DE30CF8D1,0 --account=0x53CB0AB5226EEBF4D872113D98332C1555DC304443BEE1CF759D15798D3C55A9,25000000000000000000"
concurrently --kill-others \
--names 'ganache,sendwithprivatedapp,e2e' \
Expand Down
8 changes: 4 additions & 4 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -14591,10 +14591,10 @@ mersenne-twister@^1.0.1:
resolved "https://registry.yarnpkg.com/mersenne-twister/-/mersenne-twister-1.1.0.tgz#f916618ee43d7179efcf641bec4531eb9670978a"
integrity sha1-+RZhjuQ9cXnvz2Qb7EUx65Zwl4o=

metamask-inpage-provider@^2.0.3:
version "2.0.3"
resolved "https://registry.yarnpkg.com/metamask-inpage-provider/-/metamask-inpage-provider-2.0.3.tgz#8a8ccf73d830ec76b3c746265d5977edf36a5c2f"
integrity sha512-8/c77tyM56RgKIEUDTenORVmX2xy5a5isOWVRqE8w8u584WTrf56iWewzFpU2KxbHDF3Drx+jR9tII3Qwx83kg==
metamask-inpage-provider@^2.1.0:
version "2.1.0"
resolved "https://registry.yarnpkg.com/metamask-inpage-provider/-/metamask-inpage-provider-2.1.0.tgz#69e23d63893a5e2bb7a09a6ec96c6f1500588673"
integrity sha512-1+m8Mp8/RM9JMTvDHAMt6a7aqwigRMLvU/VKKmHQFi7AZaagG8Fe4wBe8HdAMysRF3NHV/qOMw0UwP3w9m1Vaw==
dependencies:
json-rpc-engine "^5.1.3"
json-rpc-middleware-stream "^2.1.1"
Expand Down