Skip to content

Commit

Permalink
Fix an onboarding crash happening with some web3 provider setups (#442)
Browse files Browse the repository at this point in the history
When calling getBalance(), it was possible to sometimes get another value than an big integer as a string.

Having `null` as a result, and passing it to the BN.js constructor, could lead to an infinit loop [1].

To prevent this issue to happen again:

- In the app, `balance` is now always represented by a BN.js instance. To represent an unknown balance, `new BN(-1)` is now used rather than `null`.

- The result of getbalance() is now filtered to ensure that we are passing an integer to BN.js. Otherwise, we pass "-1".

[1] indutny/bn.js#186
  • Loading branch information
bpierre committed Nov 1, 2018
1 parent 4513eb3 commit f896c00
Show file tree
Hide file tree
Showing 5 changed files with 84 additions and 25 deletions.
4 changes: 2 additions & 2 deletions src/App.js
Expand Up @@ -10,7 +10,7 @@ import initWrapper, {
} from './aragonjs-wrapper'
import Wrapper from './Wrapper'
import Onboarding from './onboarding/Onboarding'
import { getWeb3 } from './web3-utils'
import { getWeb3, getUnknownBalance } from './web3-utils'
import { log } from './utils'
import { PermissionsProvider } from './contexts/PermissionsContext'
import { ModalProvider } from './components/ModalManager/ModalManager'
Expand All @@ -28,7 +28,7 @@ class App extends React.Component {
prevLocator: null,
wrapper: null,
account: '',
balance: null,
balance: getUnknownBalance(),
connected: false,
apps: [],
appsStatus: APPS_STATUS_LOADING,
Expand Down
35 changes: 26 additions & 9 deletions src/aragonjs-wrapper.js
Expand Up @@ -17,7 +17,7 @@ import {
defaultGasPriceFn,
} from './environment'
import { noop, removeStartingSlash, appendTrailingSlash } from './utils'
import { getWeb3 } from './web3-utils'
import { getWeb3, getUnknownBalance } from './web3-utils'
import { getBlobUrl, WorkerSubscriptionPool } from './worker-utils'
import { NoConnection, DAONotFound } from './errors'

Expand Down Expand Up @@ -108,28 +108,45 @@ const pollEvery = (fn, delay) => {
}
}

// Filter the value we get from getBalance() before passing it to BN.js.
// This is because passing some values to BN.js can lead to an infinite loop
// when .toString() is called. Returns "-1" when the value is invalid.
//
// See https://github.com/indutny/bn.js/issues/186
const filterBalanceValue = value => {
if (value === null) {
return '-1'
}
if (typeof value === 'object') {
value = String(value)
}
if (typeof value === 'string') {
return /^[0-9]+$/.test(value) ? value : '-1'
}
return '-1'
}

// Keep polling the main account.
// See https://github.com/MetaMask/faq/blob/master/DEVELOPERS.md#ear-listening-for-selected-account-changes
export const pollMainAccount = pollEvery(
(provider, { onAccount = () => {}, onBalance = () => {} } = {}) => {
const web3 = getWeb3(provider)
let lastAccount = null
let lastBalance = new BN(-1)
let lastBalance = getUnknownBalance()
return {
request: () =>
getMainAccount(web3)
.then(account => {
if (!account) {
throw new Error('no account')
}
return web3.eth.getBalance(account).then(balance => ({
account,
balance: new BN(balance),
}))
return web3.eth
.getBalance(account)
.then(filterBalanceValue)
.then(balance => ({ account, balance: new BN(balance) }))
})
.catch(() => {
return { account: null, balance: new BN(-1) }
}),
.catch(() => ({ account: null, balance: getUnknownBalance() })),

onResult: ({ account, balance }) => {
if (account !== lastAccount) {
lastAccount = account
Expand Down
3 changes: 2 additions & 1 deletion src/onboarding/Onboarding.js
Expand Up @@ -3,6 +3,7 @@ import styled from 'styled-components'
import { Motion, spring } from 'react-motion'
import { spring as springConf } from '@aragon/ui'
import { noop } from '../utils'
import { getUnknownBalance } from '../web3-utils'
import { isNameAvailable } from '../aragonjs-wrapper'

import * as Steps from './steps'
Expand Down Expand Up @@ -51,7 +52,7 @@ const initialState = {
class Onboarding extends React.PureComponent {
static defaultProps = {
account: '',
balance: null,
balance: getUnknownBalance(),
walletNetwork: '',
visible: true,
daoCreationStatus: 'none',
Expand Down
34 changes: 21 additions & 13 deletions src/onboarding/Start.js
Expand Up @@ -15,8 +15,8 @@ import {
import { network, web3Providers, getDemoDao } from '../environment'
import { sanitizeNetworkType } from '../network-config'
import { noop } from '../utils'
import { fromWei, toWei } from '../web3-utils'
import { formatNumber, lerp } from '../math-utils'
import { fromWei, toWei, getUnknownBalance, formatBalance } from '../web3-utils'
import { lerp } from '../math-utils'
import LoadingRing from '../components/LoadingRing'
import logo from './assets/logo-welcome.svg'

Expand All @@ -32,9 +32,6 @@ const MAINNET_RISKS_BLOG_POST =

const MINIMUM_BALANCE = new BN(toWei('0.1'))
const BALANCE_DECIMALS = 3
const formatBalance = (balance, decimals = BALANCE_DECIMALS) =>
// Don't show decimals if the user has no ETH
formatNumber(balance, balance ? decimals : 0)

const demoDao = getDemoDao()

Expand All @@ -43,7 +40,7 @@ class Start extends React.Component {
positionProgress: 0,
hasAccount: false,
walletNetwork: '',
balance: null,
balance: getUnknownBalance(),
onCreate: noop,
onDomainChange: noop,
domain: '',
Expand Down Expand Up @@ -107,10 +104,20 @@ class StartContent extends React.PureComponent {
this.props.onOpenOrganizationAddress(demoDao)
}
}
// Also returns false if the balance is unknown
enoughBalance() {
return this.props.balance.gte(MINIMUM_BALANCE)
}
unknownBalance() {
return this.props.balance.eqn(-1)
}
formattedBalance() {
const { balance } = this.props
const enough = balance && balance.lt && !balance.lt(MINIMUM_BALANCE)
return !!enough
return this.unknownBalance()
? '0'
: formatBalance(balance, {
precision: BALANCE_DECIMALS,
})
}

getNetworkChooserItems() {
Expand Down Expand Up @@ -280,7 +287,7 @@ class StartContent extends React.PureComponent {
)
}
renderWarning() {
const { hasWallet, hasAccount, walletNetwork, balance } = this.props
const { hasWallet, hasAccount, walletNetwork } = this.props
if (!hasWallet) {
return (
<ActionInfo>
Expand Down Expand Up @@ -314,9 +321,11 @@ class StartContent extends React.PureComponent {
if (!this.enoughBalance()) {
return (
<ActionInfo>
You need at least {fromWei(MINIMUM_BALANCE)} ETH (you have{' '}
{formatBalance(parseFloat(fromWei(balance || '0')))} ETH).
<br />
You need at least {fromWei(String(MINIMUM_BALANCE))} ETH
{this.unknownBalance()
? ' (your account balance is unknown)'
: ` (you have ${this.formattedBalance()} ETH)`}
.<br />
{network.type === 'rinkeby' && (
<SafeLink target="_blank" href="https://faucet.rinkeby.io/">
Request Ether on the Rinkeby Network
Expand Down Expand Up @@ -406,7 +415,6 @@ const ActionInfo = styled.span`
margin-top: 8px;
font-size: 12px;
white-space: nowrap;
text-align: center;
`

const Title = styled.h1`
Expand Down
33 changes: 33 additions & 0 deletions src/web3-utils.js
Expand Up @@ -4,6 +4,7 @@
* from this file.
*/
import Web3 from 'web3'
import BN from 'bn.js'

const EMPTY_ADDRESS = '0x0000000000000000000000000000000000000000'

Expand All @@ -19,6 +20,34 @@ export function addressesEqual(first, second) {
return first === second
}

/**
* Format the balance to a fixed number of decimals
*
* @param {BN} amount The total amount.
* @param {object} options The options object.
* @param {BN} options.base The decimals base.
* @param {number} options.precision Number of decimals to format.
*
* @returns {string} The formatted balance.
*/
export function formatBalance(
amount,
{ base = new BN(10).pow(new BN(18)), precision = 2 } = {}
) {
const baseLength = base.toString().length

const whole = amount.div(base).toString()
let fraction = amount.mod(base).toString()
const zeros = '0'.repeat(Math.max(0, baseLength - fraction.length - 1))
fraction = `${zeros}${fraction}`.replace(/0+$/, '').slice(0, precision)

if (fraction === '' || parseInt(fraction, 10) === 0) {
return whole
}

return `${whole}.${fraction}`
}

/**
* Shorten an Ethereum address. `charsLength` allows to change the number of
* characters on both sides of the ellipsis.
Expand Down Expand Up @@ -73,5 +102,9 @@ export function getEmptyAddress() {
return EMPTY_ADDRESS
}

export function getUnknownBalance() {
return new BN('-1')
}

// Re-export some utilities from web3-utils
export { fromWei, isAddress, toChecksumAddress, toWei } from 'web3-utils'

0 comments on commit f896c00

Please sign in to comment.