Skip to content

Commit

Permalink
Use a different fee currency if there's not enough to pay for gas (#2232
Browse files Browse the repository at this point in the history
)

### Description

In the places we (or other dapp developers) don't set a feeCurrency, the app tries to use CELO if the user has enough balance. The problem is that  we check for > 0, not for > gasCost, so if the user has a very small amount of CELO (less than needed to send a tx) then we still try to use CELO and the tx fails.

This has already been affecting some users: https://sentry.io/organizations/valora-inc/issues/3004252806/?project=1250733&query=is%3Aunresolved+gas and the only workaround is to either get rid of their whole CELO balance or getting more CELO.

We are now only using a feeCurrency if the user has enough to pay for gas with it.

### Other changes

N/A

### Tested

With unit tests and manually

### How others should test

- Get an account with a very small amount of CELO (<0.000001)
- Invite someone using the Send flow.
- Try to reclaim the invite. Reclaiming should be successful.

### Related issues

- Fixes #2231

### Backwards compatibility

N/A
  • Loading branch information
gnardini committed Mar 29, 2022
1 parent c491f74 commit 54c505f
Show file tree
Hide file tree
Showing 6 changed files with 293 additions and 111 deletions.
3 changes: 2 additions & 1 deletion packages/mobile/__mocks__/@celo/contractkit/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ const connection = {
chainId: jest.fn(() => Promise.resolve(44787)),
nonce: jest.fn().mockResolvedValue(7),
estimateGas: jest.fn().mockResolvedValue(1000000),
gasPrice: jest.fn().mockResolvedValue(3),
estimateGasWithInflationFactor: jest.fn().mockResolvedValue(1000000),
gasPrice: jest.fn().mockResolvedValue('3'),
}

const kit = {
Expand Down
3 changes: 3 additions & 0 deletions packages/mobile/src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ export const SMS_RETRIEVER_APP_SIGNATURE = Config.SMS_RETRIEVER_APP_SIGNATURE
// TODO change this to new ODIS minimum dollar balance once deployed
export const ODIS_MINIMUM_DOLLAR_BALANCE = 0.1
export const ATTESTATION_REVEAL_TIMEOUT_SECONDS = 60 // 1 minute
// Additional gas added when setting the fee currency
// See details where used.
export const STATIC_GAS_PADDING = 50_000

// We can safely assume that any balance query returning a number
// higher than this is incorrect (currently set to 10M)
Expand Down
162 changes: 162 additions & 0 deletions packages/mobile/src/transactions/send.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
import BigNumber from 'bignumber.js'
import { expectSaga } from 'redux-saga-test-plan'
import { call } from 'redux-saga/effects'
import { chooseTxFeeDetails } from 'src/transactions/send'
import { createMockStore } from 'test/utils'
import { mockCeloAddress, mockCeurAddress, mockCusdAddress } from 'test/values'

const state = (override?: { celoBalance: string }) =>
createMockStore({
tokens: {
tokenBalances: {
[mockCusdAddress]: {
balance: '0',
usdPrice: '1',
symbol: 'cUSD',
address: mockCusdAddress,
isCoreToken: true,
priceFetchedAt: Date.now(),
},
[mockCeurAddress]: {
balance: '1',
usdPrice: '1.2',
symbol: 'cEUR',
address: mockCeurAddress,
isCoreToken: true,
priceFetchedAt: Date.now(),
},
[mockCeloAddress]: {
balance: override?.celoBalance ?? '0',
usdPrice: '3.5',
symbol: 'CELO',
address: mockCeloAddress,
isCoreToken: true,
priceFetchedAt: Date.now(),
},
},
},
}).getState()

function* wrapperSaga({
tx,
feeCurrency,
gas,
gasPrice,
}: {
tx: any
feeCurrency: string | undefined
gas?: number
gasPrice?: BigNumber
}) {
return yield call(chooseTxFeeDetails, tx, feeCurrency, gas, gasPrice)
}

describe('chooseTxFeeDetails', () => {
it("returns the passed values if there's enough balance to pay for gas", async () => {
await expectSaga(wrapperSaga, {
tx: { from: '0xTEST', data: '0xABC' },
feeCurrency: mockCeurAddress,
gas: 100,
gasPrice: new BigNumber(20),
})
.withState(state())
.returns({
feeCurrency: mockCeurAddress,
gas: 100,
gasPrice: '20',
})
.run()
})

it('returns a different fee currency if the passed one has no balance', async () => {
await expectSaga(wrapperSaga, {
tx: { from: '0xTEST', data: '0xABC' },
feeCurrency: mockCusdAddress,
gas: 100,
gasPrice: new BigNumber(20),
})
.withState(state())
.returns({
feeCurrency: mockCeurAddress,
gas: 100, // gas doesn't change since it went from a stable token to another
gasPrice: undefined,
})
.run()
})

it('adds a padding to gas if switching from CELO fee currency to another', async () => {
await expectSaga(wrapperSaga, {
tx: { from: '0xTEST', data: '0xABC' },
feeCurrency: undefined,
gas: 100,
gasPrice: new BigNumber(20),
})
.withState(state())
.returns({
feeCurrency: mockCeurAddress,
gas: 50100, // 50000 is STATIC_GAS_PADDING
gasPrice: undefined,
})
.run()
})

it("switches to another fee currency if there's some balance but not enough to cover the fee", async () => {
await expectSaga(wrapperSaga, {
tx: { from: '0xTEST', data: '0xABC' },
feeCurrency: undefined,
gas: 100,
gasPrice: new BigNumber(20),
})
.withState(state({ celoBalance: new BigNumber(200).shiftedBy(-18).toString() }))
.returns({
feeCurrency: mockCeurAddress,
gas: 50100, // 50000 is STATIC_GAS_PADDING
gasPrice: undefined,
})
.run()
})

it('keeps the gas amount if going from stable token to CELO fee currency', async () => {
await expectSaga(wrapperSaga, {
tx: { from: '0xTEST', data: '0xABC' },
feeCurrency: mockCusdAddress,
gas: 100,
gasPrice: new BigNumber(20),
})
.withState(state({ celoBalance: '20000' }))
.returns({
feeCurrency: undefined,
gas: 100,
gasPrice: undefined,
})
.run()
})

it('estimates gas and gas price if not passed', async () => {
await expectSaga(wrapperSaga, {
tx: { from: '0xTEST', data: '0xABC' },
feeCurrency: mockCeurAddress,
})
.withState(state())
.returns({
feeCurrency: mockCeurAddress,
gas: 1000000,
gasPrice: '50000',
})
.run()
})

it('returns only estimated gas (with padding), not gasPrice if estimate is not enough for gas cost', async () => {
await expectSaga(wrapperSaga, {
tx: { from: '0xTEST', data: '0xABC' },
feeCurrency: undefined,
})
.withState(state({ celoBalance: new BigNumber(200).shiftedBy(-18).toString() }))
.returns({
feeCurrency: mockCeurAddress,
gas: 1050000, // Added STATIC_GAS_PADDING
gasPrice: undefined,
})
.run()
})
})
127 changes: 69 additions & 58 deletions packages/mobile/src/transactions/send.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@ import { call, cancel, cancelled, delay, fork, join, race, select } from 'redux-
import { TransactionEvents } from 'src/analytics/Events'
import ValoraAnalytics from 'src/analytics/ValoraAnalytics'
import { ErrorMessages } from 'src/app/ErrorMessages'
import { DEFAULT_FORNO_URL } from 'src/config'
import { STATIC_GAS_PADDING } from 'src/config'
import { fetchFeeCurrencySaga } from 'src/fees/saga'
import { WEI_DECIMALS } from 'src/geth/consts'
import { TokenBalance } from 'src/tokens/reducer'
import { coreTokensSelector } from 'src/tokens/selectors'
import {
Expand All @@ -15,11 +16,11 @@ import {
TxPromises,
} from 'src/transactions/contract-utils'
import { TransactionContext } from 'src/transactions/types'
import { Currency } from 'src/utils/currencies'
import Logger from 'src/utils/Logger'
import { assertNever } from 'src/utils/typescript'
import { getGasPrice } from 'src/web3/gas'
import { fornoSelector } from 'src/web3/selectors'
import { fornoSelector, walletAddressSelector } from 'src/web3/selectors'
import { estimateGas } from 'src/web3/utils'

const TAG = 'transactions/send'

Expand Down Expand Up @@ -106,21 +107,65 @@ const getLogger = (context: TransactionContext, fornoMode?: boolean) => {
}
}

// If the preferred currency has enough balance, use that.
// Otherwise use any that has enough balance.
// TODO: Make fee currency choosing transparent for the user.
export function* chooseFeeCurrency(preferredFeeCurrency: string | undefined) {
// This function returns the feeCurrency and gas/gasPrice to use to send a tx.
// If gas or gasPrice are not passed, we estimate/calculate them.
// If the balance is not enough to pay for the gas fee, we return the currency with
// highest balance, sometimes return a gas value if we can return a good estimate
// and an empty gasPrice value.
export function* chooseTxFeeDetails(
tx: CeloTxObject<any>,
preferredFeeCurrency: string | undefined,
gas?: number,
gasPrice?: BigNumber
) {
const coreTokens: TokenBalance[] = yield select(coreTokensSelector)
const tokenInfo = coreTokens.find(
(token) =>
token.address === preferredFeeCurrency || (token.symbol === 'CELO' && !preferredFeeCurrency)
)
// TODO: Check if balance is enough to pay for fee, not just gt 0.
if (tokenInfo?.balance.gt(0)) {
return preferredFeeCurrency
if (!tokenInfo || tokenInfo.balance.isZero()) {
const feeCurrency: string | undefined = yield call(fetchFeeCurrencySaga)
return {
feeCurrency,
// If gas was set and we switched from CELO to a non-CELO fee currency, we add some padding to it
// since it takes a bit more gas to pay for fees using a non-CELO fee currency.
gas: gas && !preferredFeeCurrency && feeCurrency ? gas + STATIC_GAS_PADDING : gas,
// Set gasPrice to undefined if the currency being used for the fee changed.
gasPrice: preferredFeeCurrency !== feeCurrency ? undefined : gasPrice,
}
}
const userAddress: string = yield select(walletAddressSelector)
const feeCurrency = tokenInfo.symbol === 'CELO' ? undefined : tokenInfo.address
if (!gas) {
gas = ((yield call(estimateGas, tx, {
from: userAddress,
feeCurrency,
})) as BigNumber).toNumber()
}
if (!gasPrice) {
gasPrice = yield getGasPrice(feeCurrency)
}
if (new BigNumber(gasPrice!).times(gas!).lte(tokenInfo.balance.shiftedBy(WEI_DECIMALS))) {
return {
feeCurrency,
gas,
gasPrice: gasPrice?.toString(),
}
} else {
// Funds are not enough to pay for the fee.
const feeCurrency: string | undefined = yield call(fetchFeeCurrencySaga)
return {
feeCurrency,
// If gas was set and we switched from CELO to a non-CELO fee currency, we add some padding to it
// since it takes a bit more gas to pay for fees using a non-CELO fee currency.
// Why aren't we just estimating again?
// It may result in errors for the dapp. E.g. If a dapp developer is doing a two step approve and exchange and requesting both signatures
// together, they will set the gas on the second transaction because if estimateGas is run before the approve completes, execution will fail.
gas: gas && !preferredFeeCurrency && feeCurrency ? gas + STATIC_GAS_PADDING : gas,
// Set gasPrice to undefined if the currency being used for the fee changed.
gasPrice: preferredFeeCurrency !== feeCurrency ? undefined : gasPrice,
}
}
const feeCurrency: string | undefined = yield call(fetchFeeCurrencySaga)
return feeCurrency
}

// Sends a transaction and async returns promises for the txhash, confirmation, and receipt
Expand All @@ -132,8 +177,8 @@ export function* sendTransactionPromises(
account: string,
context: TransactionContext,
preferredFeeCurrency: string | undefined,
gas?: number,
gasPrice?: BigNumber,
proposedGas?: number,
proposedGasPrice?: BigNumber,
nonce?: number
) {
Logger.debug(
Expand All @@ -142,7 +187,15 @@ export function* sendTransactionPromises(
)

const fornoMode: boolean = yield select(fornoSelector)
const feeCurrency: Currency = yield call(chooseFeeCurrency, preferredFeeCurrency)
const {
feeCurrency,
gas,
gasPrice,
}: {
feeCurrency: string | undefined
gas?: number
gasPrice?: string
} = yield call(chooseTxFeeDetails, tx, preferredFeeCurrency, proposedGas, proposedGasPrice)

if (gas || gasPrice) {
Logger.debug(
Expand All @@ -151,39 +204,10 @@ export function* sendTransactionPromises(
)
}

if (feeCurrency !== preferredFeeCurrency) {
Logger.warn(
`${TAG}@sendTransactionPromises`,
`Using fallback fee currency ${feeCurrency} instead of preferred ${preferredFeeCurrency}.`
)
// If the currency is changed, the gas value and price are invalidated.
// TODO: Move the fallback currency logic up the stackso this will never happen.
if (gas || gasPrice) {
Logger.warn(
`${TAG}@sendTransactionPromises`,
`Resetting gas parameters because fee currency was changed.`
)
gas = undefined
gasPrice = undefined
}
}

Logger.debug(
`${TAG}@sendTransactionPromises`,
`Sending tx ${context.id} in ${fornoMode ? 'forno' : 'geth'} mode`
)
if (fornoMode) {
// In dev mode, verify that we are actually able to connect to the network. This
// ensures that we get a more meaningful error if the forno server is down, which
// can happen with networks without SLA guarantees like `integration`.
if (__DEV__) {
yield call(verifyUrlWorksOrThrow, DEFAULT_FORNO_URL)
}

if (!gasPrice) {
gasPrice = yield getGasPrice(feeCurrency)
}
}

const transactionPromises: TxPromises = yield call(
sendTransactionAsync,
Expand All @@ -192,7 +216,7 @@ export function* sendTransactionPromises(
feeCurrency,
getLogger(context, fornoMode),
gas,
gasPrice?.toString(),
gasPrice,
nonce
)
return transactionPromises
Expand Down Expand Up @@ -339,16 +363,3 @@ function shouldTxFailureRetry(err: any) {

return true
}

async function verifyUrlWorksOrThrow(url: string) {
try {
await fetch(url)
} catch (e) {
Logger.error(
'contracts@verifyUrlWorksOrThrow',
`Failed to perform HEAD request to url: "${url}"`,
e
)
throw new Error(`Failed to perform HEAD request to url: "${url}", is it working?`)
}
}

0 comments on commit 54c505f

Please sign in to comment.