From daa89730be235d5a29bd5f982a8a8bd9f02c2af0 Mon Sep 17 00:00:00 2001 From: Kathy Luo Date: Fri, 26 May 2023 17:43:04 +0200 Subject: [PATCH] chore(swaps): add more analytics for debugging (#3810) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### Description We are still getting many errors that indicate that we are allowing the user to proceed with a swap amount greater than their balance. It's not clear why this still happens, because we have a (high) fee estimation in place already. Ideally I'd like to have data to be able to say "oh yes, this swap was expected to fail because the swap amount was X, required fee was Y, and X + Y > the total balance". i think then we could go back and tweak the logic required for X, when we have a better idea of what X and Y are. Sadly during my debugging today I think some errors are being thrown at the calculation for the gas fee 🫠 so we may not have the value for Y. This PR adds data for - total "from" token balance at the time of executing the swap - context id's used for the swap transactions so that we can also check the TransactionEvents which contains some information about gas / errors etc. If an error is thrown when calculating the gas fee, then we will be able to tell (from the lack of the fee event). - add fee currency to the estimation and error TransactionEvents, to see if there's any correlation of token with errors. ### Test plan n/a ### Related issues n/a ### Backwards compatibility Y --- src/analytics/Properties.tsx | 13 +++- src/swap/saga.test.ts | 78 ++++++++++++++---------- src/swap/saga.ts | 96 ++++++++++++++++-------------- src/transactions/contract-utils.ts | 16 ++--- src/transactions/send.ts | 2 + 5 files changed, 120 insertions(+), 85 deletions(-) diff --git a/src/analytics/Properties.tsx b/src/analytics/Properties.tsx index 8fcd8fd436d..643cd69440d 100644 --- a/src/analytics/Properties.tsx +++ b/src/analytics/Properties.tsx @@ -10,9 +10,9 @@ import { AppEvents, AssetsEvents, AuthenticationEvents, - CICOEvents, CeloExchangeEvents, CeloNewsEvents, + CICOEvents, CoinbasePayEvents, ContractKitEvents, DappExplorerEvents, @@ -695,6 +695,7 @@ interface TransactionEventsProperties { txId: string estimatedGas: number prefilled: boolean + feeCurrencyAddress?: string } [TransactionEvents.transaction_hash_received]: { txId: string @@ -713,6 +714,7 @@ interface TransactionEventsProperties { [TransactionEvents.transaction_exception]: { txId: string error: string + feeCurrencyAddress?: string } } @@ -1217,9 +1219,16 @@ interface SwapEventsProperties { toToken: string fromToken: string } - [SwapEvents.swap_execute_success]: SwapQuoteEvent + [SwapEvents.swap_execute_success]: SwapQuoteEvent & { + fromTokenBalance: string + swapExecuteTxId: string + swapApproveTxId: string + } [SwapEvents.swap_execute_error]: SwapQuoteEvent & { error: string + fromTokenBalance: string + swapExecuteTxId: string + swapApproveTxId: string } [SwapEvents.swap_learn_more]: undefined } diff --git a/src/swap/saga.test.ts b/src/swap/saga.test.ts index 5e444cc23e3..a29e2f8dbe1 100644 --- a/src/swap/saga.test.ts +++ b/src/swap/saga.test.ts @@ -1,4 +1,6 @@ +import BigNumber from 'bignumber.js' import { expectSaga } from 'redux-saga-test-plan' +import { EffectProviders, StaticProvider } from 'redux-saga-test-plan/providers' import { call, select } from 'redux-saga/effects' import { SwapEvents } from 'src/analytics/Events' import ValoraAnalytics from 'src/analytics/ValoraAnalytics' @@ -6,12 +8,19 @@ import { store } from 'src/redux/store' import { swapSubmitSaga } from 'src/swap/saga' import { swapApprove, swapError, swapExecute, swapPriceChange } from 'src/swap/slice' import { getERC20TokenContract } from 'src/tokens/saga' +import { swappableTokensSelector } from 'src/tokens/selectors' import { sendTransaction } from 'src/transactions/send' import Logger from 'src/utils/Logger' import { getContractKit } from 'src/web3/contracts' import { getConnectedUnlockedAccount } from 'src/web3/saga' import { walletAddressSelector } from 'src/web3/selectors' -import { mockAccount, mockContract } from 'test/values' +import { + mockAccount, + mockCeloAddress, + mockCeurAddress, + mockContract, + mockTokenBalances, +} from 'test/values' const loggerErrorSpy = jest.spyOn(Logger, 'error') @@ -31,8 +40,8 @@ jest.mock('src/transactions/send', () => ({ const mockSwapTransaction = { buyAmount: '10000000000000000', - buyTokenAddress: '0xd8763cba276a3738e6de85b4b3bf5fded6d6ca73', - sellTokenAddress: '0xe8537a3d056da446677b9e9d6c5db704eaab4787', + buyTokenAddress: mockCeloAddress, + sellTokenAddress: mockCeurAddress, price: '1', guaranteedPrice: '1.02', from: '0x078e54ad49b0865fff9086fd084b92b3dac0857d', @@ -64,18 +73,29 @@ describe(swapSubmitSaga, () => { jest.clearAllMocks() }) + const defaultProviders: (EffectProviders | StaticProvider)[] = [ + [select(walletAddressSelector), mockAccount], + [call(getContractKit), contractKit], + [call(getConnectedUnlockedAccount), mockAccount], + [ + select(swappableTokensSelector), + [ + { + ...mockTokenBalances[mockCeurAddress], + balance: new BigNumber('10'), + }, + ], + ], + [ + call(getERC20TokenContract, mockSwap.payload.unvalidatedSwapTransaction.sellTokenAddress), + mockContract, + ], + ] + it('should complete swap', async () => { await expectSaga(swapSubmitSaga, mockSwap) .withState(store.getState()) - .provide([ - [select(walletAddressSelector), mockAccount], - [call(getContractKit), contractKit], - [call(getConnectedUnlockedAccount), mockAccount], - [ - call(getERC20TokenContract, mockSwap.payload.unvalidatedSwapTransaction.sellTokenAddress), - mockContract, - ], - ]) + .provide(defaultProviders) .put(swapApprove()) .put(swapExecute()) .run() @@ -84,14 +104,17 @@ describe(swapSubmitSaga, () => { expect(ValoraAnalytics.track).toHaveBeenCalledTimes(1) expect(ValoraAnalytics.track).toHaveBeenCalledWith(SwapEvents.swap_execute_success, { - toToken: '0xd8763cba276a3738e6de85b4b3bf5fded6d6ca73', - fromToken: '0xe8537a3d056da446677b9e9d6c5db704eaab4787', + toToken: mockCeloAddress, + fromToken: mockCeurAddress, amount: '10000000000000000', amountType: 'buyAmount', price: '1', allowanceTarget: '0xdef1c0ded9bec7f1a1670819833240f027b25eff', estimatedPriceImpact: '0.1', provider: '0x', + fromTokenBalance: '10000000000000000000', + swapApproveTxId: 'a uuid', + swapExecuteTxId: 'a uuid', }) }) @@ -101,29 +124,24 @@ describe(swapSubmitSaga, () => { }) await expectSaga(swapSubmitSaga, mockSwap) .withState(store.getState()) - .provide([ - [select(walletAddressSelector), mockAccount], - [call(getContractKit), contractKit], - [call(getConnectedUnlockedAccount), mockAccount], - [ - call(getERC20TokenContract, mockSwap.payload.unvalidatedSwapTransaction.sellTokenAddress), - mockContract, - ], - ]) + .provide(defaultProviders) .put(swapApprove()) .put(swapError()) .run() expect(ValoraAnalytics.track).toHaveBeenCalledTimes(1) expect(ValoraAnalytics.track).toHaveBeenCalledWith(SwapEvents.swap_execute_error, { error: 'fake error', - toToken: '0xd8763cba276a3738e6de85b4b3bf5fded6d6ca73', - fromToken: '0xe8537a3d056da446677b9e9d6c5db704eaab4787', + toToken: mockCeloAddress, + fromToken: mockCeurAddress, amount: '10000000000000000', amountType: 'buyAmount', price: '1', allowanceTarget: '0xdef1c0ded9bec7f1a1670819833240f027b25eff', estimatedPriceImpact: '0.1', provider: '0x', + fromTokenBalance: '10000000000000000000', + swapApproveTxId: 'a uuid', + swapExecuteTxId: 'a uuid', }) }) @@ -131,11 +149,7 @@ describe(swapSubmitSaga, () => { mockSwap.payload.unvalidatedSwapTransaction.guaranteedPrice = '1.021' await expectSaga(swapSubmitSaga, mockSwap) .withState(store.getState()) - .provide([ - [select(walletAddressSelector), mockAccount], - [call(getContractKit), contractKit], - [call(getConnectedUnlockedAccount), mockAccount], - ]) + .provide(defaultProviders) .put(swapPriceChange()) .run() @@ -143,8 +157,8 @@ describe(swapSubmitSaga, () => { expect(ValoraAnalytics.track).toHaveBeenCalledWith(SwapEvents.swap_execute_price_change, { price: '1', guaranteedPrice: '1.021', - toToken: '0xd8763cba276a3738e6de85b4b3bf5fded6d6ca73', - fromToken: '0xe8537a3d056da446677b9e9d6c5db704eaab4787', + toToken: mockCeloAddress, + fromToken: mockCeurAddress, }) }) }) diff --git a/src/swap/saga.ts b/src/swap/saga.ts index 082c9ff4b1f..42a7603aef4 100644 --- a/src/swap/saga.ts +++ b/src/swap/saga.ts @@ -18,10 +18,12 @@ import { swapStart, swapSuccess, } from 'src/swap/slice' -import { ApproveTransaction, Field, SwapInfo, SwapTransaction } from 'src/swap/types' +import { Field, SwapInfo, SwapTransaction } from 'src/swap/types' import { getERC20TokenContract } from 'src/tokens/saga' +import { swappableTokensSelector } from 'src/tokens/selectors' +import { TokenBalance } from 'src/tokens/slice' import { sendTransaction } from 'src/transactions/send' -import { newTransactionContext } from 'src/transactions/types' +import { newTransactionContext, TransactionContext } from 'src/transactions/types' import Logger from 'src/utils/Logger' import { safely } from 'src/utils/safely' import { getContractKit } from 'src/web3/contracts' @@ -36,8 +38,8 @@ function getPercentageDifference(price1: number, price2: number) { } function* handleSendSwapTransaction( - rawTx: ApproveTransaction | SwapTransaction, - tagDescription: string + rawTx: SwapTransaction, + transactionContext: TransactionContext ) { const kit: ContractKit = yield call(getContractKit) const walletAddress: string = yield call(getConnectedUnlockedAccount) @@ -47,15 +49,7 @@ function* handleSendSwapTransaction( const tx: CeloTx = yield call(normalizer.populate.bind(normalizer), rawTx) const txo = buildTxo(kit, tx) - yield call( - sendTransaction, - txo, - walletAddress, - newTransactionContext(TAG, tagDescription), - undefined, - undefined, - undefined - ) + yield call(sendTransaction, txo, walletAddress, transactionContext) } export function* swapSubmitSaga(action: PayloadAction) { @@ -69,9 +63,35 @@ export function* swapSubmitSaga(action: PayloadAction) { allowanceTarget, estimatedPriceImpact, } = action.payload.unvalidatedSwapTransaction - const amountType = action.payload.userInput.updatedField === Field.TO ? 'buyAmount' : 'sellAmount' + const amountType = + action.payload.userInput.updatedField === Field.TO + ? ('buyAmount' as const) + : ('sellAmount' as const) const amount = action.payload.unvalidatedSwapTransaction[amountType] + const tokenBalances: TokenBalance[] = yield select(swappableTokensSelector) + const fromToken = tokenBalances.find((token) => token.address === sellTokenAddress) + const fromTokenBalance = fromToken + ? fromToken.balance.shiftedBy(fromToken.decimals).toString() + : '' + + const swapApproveContext = newTransactionContext(TAG, 'Swap/Approve') + const swapExecuteContext = newTransactionContext(TAG, 'Swap/Execute') + + const defaultSwapExecuteProps = { + toToken: buyTokenAddress, + fromToken: sellTokenAddress, + amount, + amountType, + price, + allowanceTarget, + estimatedPriceImpact, + provider: action.payload.details.swapProvider, + fromTokenBalance, + swapExecuteTxId: swapExecuteContext.id, + swapApproveTxId: swapApproveContext.id, + } + try { // Navigate to swap pending screen yield call(navigate, Screens.SwapExecuteScreen) @@ -104,40 +124,31 @@ export function* swapSubmitSaga(action: PayloadAction) { TAG, `Approving ${amountToApprove} of ${sellTokenAddress} for address: ${allowanceTarget}` ) - yield call(sendApproveTx, sellTokenAddress, amountToApprove, allowanceTarget) + yield call( + sendApproveTx, + sellTokenAddress, + amountToApprove, + allowanceTarget, + swapApproveContext + ) // Execute transaction yield put(swapExecute()) Logger.debug(TAG, `Starting to swap execute for address: ${walletAddress}`) + yield call( handleSendSwapTransaction, { ...action.payload.unvalidatedSwapTransaction }, - 'Swap/Execute' + swapExecuteContext ) yield put(swapSuccess()) vibrateSuccess() - ValoraAnalytics.track(SwapEvents.swap_execute_success, { - toToken: buyTokenAddress, - fromToken: sellTokenAddress, - amount, - amountType, - price, - allowanceTarget, - estimatedPriceImpact, - provider: action.payload.details.swapProvider, - }) + ValoraAnalytics.track(SwapEvents.swap_execute_success, defaultSwapExecuteProps) } catch (error) { Logger.error(TAG, 'Error while swapping', error) ValoraAnalytics.track(SwapEvents.swap_execute_error, { + ...defaultSwapExecuteProps, error: error.message, - toToken: buyTokenAddress, - fromToken: sellTokenAddress, - amount, - amountType, - price, - allowanceTarget, - estimatedPriceImpact, - provider: action.payload.details.swapProvider, }) yield put(swapError()) vibrateError() @@ -148,7 +159,12 @@ export function* swapSaga() { yield takeLatest(swapStart.type, safely(swapSubmitSaga)) } -export function* sendApproveTx(tokenAddress: string, amount: string, recipientAddress: string) { +export function* sendApproveTx( + tokenAddress: string, + amount: string, + recipientAddress: string, + transactionContext: TransactionContext +) { const kit: ContractKit = yield call(getContractKit) const contract: Contract = yield call(getERC20TokenContract, tokenAddress) const walletAddress: string = yield call(getConnectedUnlockedAccount) @@ -158,13 +174,5 @@ export function* sendApproveTx(tokenAddress: string, amount: string, recipientAd contract.methods.approve(recipientAddress, amount) ) - yield call( - sendTransaction, - tx.txo, - walletAddress, - newTransactionContext(TAG, 'Swap/Approve'), - undefined, - undefined, - undefined - ) + yield call(sendTransaction, tx.txo, walletAddress, transactionContext) } diff --git a/src/transactions/contract-utils.ts b/src/transactions/contract-utils.ts index 292bf044af5..5f74b09d4cc 100644 --- a/src/transactions/contract-utils.ts +++ b/src/transactions/contract-utils.ts @@ -73,10 +73,11 @@ interface EstimatedGas { type: SendTransactionLogEventType.EstimatedGas gas: number prefilled: boolean + feeCurrencyAddress: string | undefined } -function EstimatedGas(gas: number, prefilled: boolean): EstimatedGas { - return { type: SendTransactionLogEventType.EstimatedGas, gas, prefilled } +function EstimatedGas(gas: number, prefilled: boolean, feeCurrencyAddress?: string): EstimatedGas { + return { type: SendTransactionLogEventType.EstimatedGas, gas, prefilled, feeCurrencyAddress } } interface ReceiptReceived { @@ -109,10 +110,11 @@ function Failed(error: Error): Failed { interface Exception { type: SendTransactionLogEventType.Exception error: Error + feeCurrencyAddress?: string } -function Exception(error: Error): Exception { - return { type: SendTransactionLogEventType.Exception, error } +function Exception(error: Error, feeCurrencyAddress?: string): Exception { + return { type: SendTransactionLogEventType.Exception, error, feeCurrencyAddress } } /** @@ -208,9 +210,9 @@ export async function sendTransactionAsync( if (gas === undefined) { gas = (await estimateGas(tx, txParams)).toNumber() - logger(EstimatedGas(gas, false)) + logger(EstimatedGas(gas, false, feeCurrencyAddress)) } else { - logger(EstimatedGas(gas, true)) + logger(EstimatedGas(gas, true, feeCurrencyAddress)) } emitter = tx.send({ ...txParams, gas }) @@ -244,7 +246,7 @@ export async function sendTransactionAsync( } }) } catch (error) { - logger(Exception(error)) + logger(Exception(error, feeCurrencyAddress)) rejectAll(error) } diff --git a/src/transactions/send.ts b/src/transactions/send.ts index 473c2e67c68..34e773cb067 100644 --- a/src/transactions/send.ts +++ b/src/transactions/send.ts @@ -65,6 +65,7 @@ const getLogger = (context: TransactionContext) => { txId, estimatedGas: event.gas, prefilled: event.prefilled, + feeCurrencyAddress: event.feeCurrencyAddress, }) break case SendTransactionLogEventType.TransactionHashReceived: @@ -97,6 +98,7 @@ const getLogger = (context: TransactionContext) => { ValoraAnalytics.track(TransactionEvents.transaction_exception, { txId, error: event.error.message, + feeCurrencyAddress: event.feeCurrencyAddress, }) break default: