From fd214eb573c7185c01dfa2040aa914e87a0de359 Mon Sep 17 00:00:00 2001 From: Buck Perley Date: Fri, 22 May 2020 10:51:04 -0500 Subject: [PATCH] fix(wallet): fixes inconsistencies in wallet balance update after spend (#131) * fixes inconsistencies in wallet balance update after spend * add warning message if utxos don't update * update formatting of transaction preview totals * make slice querying after spend more selective * add error catching for utxo fetch --- src/actions/walletActions.js | 134 +++++++++++---- src/components/Wallet/TransactionPreview.jsx | 7 +- src/components/Wallet/WalletSign.jsx | 165 +++++++------------ src/reducers/braidReducer.js | 19 --- 4 files changed, 168 insertions(+), 157 deletions(-) diff --git a/src/actions/walletActions.js b/src/actions/walletActions.js index 4b59c377..e0e493aa 100644 --- a/src/actions/walletActions.js +++ b/src/actions/walletActions.js @@ -14,6 +14,7 @@ import { setFee, finalizeOutputs, } from "./transactionActions"; +import { setErrorNotification } from "./errorNotificationActions"; import { getSpendableSlices } from "../selectors/wallet"; export const UPDATE_DEPOSIT_SLICE = "UPDATE_DEPOSIT_SLICE"; @@ -155,71 +156,136 @@ export function autoSelectCoins() { }; } -export function spendSlices(inputs, changeSlice) { +/** + * @description Given the state of the transaction store, check status of + * the slices being used in the spend tx and update them once changed state is confirmed. + * @param {Object} changeSlice - the slice that was chosen as the change output + */ +export function updateTxSlices( + changeSlice, + retries = 10, + skipAddresses = new Set() +) { + // eslint-disable-next-line consistent-return return async (dispatch, getState) => { const { settings: { network }, client, spend: { - transaction: { changeAddress }, + transaction: { changeAddress, inputs, txid }, }, wallet: { - deposits: { nextNode }, + deposits: { nextNode: nextDepositSlice, nodes: depositSlices }, + change: { nodes: changeSlices }, }, } = getState(); - const sliceUpdates = []; - - // track which slices are already being queried since - // the fetch command will get all utxos for an address. - const addressSet = new Set(); + // utility function for getting utxo set of an address + // and formatting the result in a way we can use const fetchSliceStatus = async (address, bip32Path) => { const utxos = await fetchAddressUTXOs(address, network, client); return { addressUsed: true, change: isChange(bip32Path), + address, bip32Path, ...utxos, }; }; - inputs.forEach((input) => { - const { address } = input.multisig; - if (!addressSet.has(address)) { + // array of slices we want to query + const slices = [...inputs]; + + // if there is a change address in the transaction + // then we want to query the state of that slice as well + if (changeSlice && changeSlice.multisig.address === changeAddress) { + slices.push(changeSlice); + } + + // array to store results from queries for each slice + const sliceUpdates = []; + + // track which slices are already being queried since + // the fetch command will get all utxos for an address + // and some inputs might be for the same address + const addressSet = new Set(); + + // go through each slice and see if it needs to be queried + slices.forEach((slice) => { + const { address } = slice.multisig; + // if address isn't already being queried and isn't in the + // set of addresses to skip (which means it's already been + // successfully updated), then fetchSliceStatus + if (!addressSet.has(address) && !skipAddresses.has(address)) { addressSet.add(address); // setting up async network calls to await all of them - sliceUpdates.push(fetchSliceStatus(address, input.bip32Path)); + sliceUpdates.push(fetchSliceStatus(address, slice.bip32Path)); } }); - // if we have a change slice, then let's query an update for - // that slice too - if (changeSlice && changeSlice.multisig.address === changeAddress) { - addressSet.add(changeAddress); - sliceUpdates.push(fetchSliceStatus(changeAddress, changeSlice.bip32Path)); + let queriedSlices; + try { + queriedSlices = await Promise.all(sliceUpdates); + } catch (e) { + return dispatch( + setErrorNotification( + `There was a problem updating wallet state. Try a refresh. Error: ${e.message}` + ) + ); } - // check the next deposit slice just in case a spend is to own wallet - // this doesn't catch all self-spend cases but should catch the majority - // to avoid any confusion for less technical users. - sliceUpdates.push( - fetchSliceStatus(nextNode.multisig.address, nextNode.bip32Path) - ); + // once all queries have completed we can confirm which have been updated + // and dispatch the changes to the store + for (let i = 0; i < queriedSlices.length; i += 1) { + const slice = queriedSlices[i]; + const utxoCount = slice.change + ? changeSlices[slice.bip32Path].utxos.length + : depositSlices[slice.bip32Path].utxos.length; - const updatedSlices = await Promise.all(sliceUpdates); - - updatedSlices.forEach((slice) => { - if (slice.change) + // if the utxo count is not the same then we can reliably update + // this slice and can skip in any future calls + if (slice && slice.utxos && slice.utxos.length !== utxoCount) { dispatch({ - type: UPDATE_CHANGE_SLICE, + type: slice.change ? UPDATE_CHANGE_SLICE : UPDATE_DEPOSIT_SLICE, value: slice, }); - else - dispatch({ - type: UPDATE_DEPOSIT_SLICE, - value: slice, - }); - }); + skipAddresses.add(slice.address); + } + } + + // if not all slices queried were successful, then we want to recursively call + // updateTxSlices, counting down retries and with the full set of successful queries + // ALL input slices must return a different utxo set otherwise something went wrong + if (skipAddresses.size !== queriedSlices.length && retries) + return setTimeout( + () => dispatch(updateTxSlices(changeSlice, retries - 1, skipAddresses)), + 750 + ); + + // if we're out of retries and counts are still the same + // then we're done trying and should show an error + if (!retries) { + let message = `There was a problem updating the wallet balance. + It is recommended you refresh the wallet to make sure UTXO current set is up to date.`; + if (txid && txid.length) message += ` Transaction ID: ${txid}`; + return dispatch(setErrorNotification(message)); + } + + // Check the next deposit slice just in case a spend is to own wallet. + // This doesn't catch all self-spend cases but should catch the majority + // to avoid any confusion for less technical users. + const updatedSlice = await fetchSliceStatus( + nextDepositSlice.multisig.address, + nextDepositSlice.bip32Path + ); + + // if its status has changed and the utxo set for the next + // deposit slice is different, then update it as well + if ( + updatedSlice.utxos.length !== + depositSlices[updatedSlice.bip32Path].utxos.length + ) + return dispatch({ type: UPDATE_DEPOSIT_SLICE, value: updatedSlice }); }; } diff --git a/src/components/Wallet/TransactionPreview.jsx b/src/components/Wallet/TransactionPreview.jsx index e9fd3330..e4f600ed 100644 --- a/src/components/Wallet/TransactionPreview.jsx +++ b/src/components/Wallet/TransactionPreview.jsx @@ -38,16 +38,17 @@ class TransactionPreview extends React.Component {

Fee

-
{fee}
+
{BigNumber(fee).toFixed(8)} BTC

Fee Rate

-
{feeRate}
+
{feeRate} sats/byte

Total

- {satoshisToBitcoins(BigNumber(inputsTotalSats || 0)).toString()} + {satoshisToBitcoins(BigNumber(inputsTotalSats || 0)).toFixed(8)}{" "} + BTC
diff --git a/src/components/Wallet/WalletSign.jsx b/src/components/Wallet/WalletSign.jsx index f2374935..fd65dacb 100644 --- a/src/components/Wallet/WalletSign.jsx +++ b/src/components/Wallet/WalletSign.jsx @@ -16,9 +16,8 @@ import { SPEND_STEP_CREATE, } from "../../actions/transactionActions"; import { - spendSlices as spendSlicesAction, + updateTxSlices as updateTxSlicesAction, resetWalletView as resetWalletViewAction, - updateChangeSliceAction as updateChangeSliceActionImport, } from "../../actions/walletActions"; import UnsignedTransaction from "../UnsignedTransaction"; @@ -30,68 +29,27 @@ class WalletSign extends React.Component { }; } + static getDerivedStateFromProps(props, state) { + const { txid, changeSlice, updateTxSlices } = props; + if (txid.length && !state.spent) { + updateTxSlices(changeSlice); + return { + spent: true, + }; + } + return null; + } + componentWillUnmount() { - const { - resetTransaction, - transaction, - changeSlice, - spendSlices, - } = this.props; + const { resetTransaction } = this.props; const { spent } = this.state; + + // reset the transaction when we leave the view if tx is spent if (spent) { - // have a brief delay to make sure the queried node had enough time - // to update - setTimeout(() => spendSlices(transaction.inputs, changeSlice), 1000); resetTransaction(); } } - render = () => { - const { spent } = this.state; - return ( - - - - - - - {this.renderKeySelectors()} - - - - - - {this.signaturesFinalized() && ( - - - - )} - - {(this.transactionFinalized() || spent) && ( - - - - )} - - ); - }; - renderKeySelectors = () => { const { requiredSigners } = this.props; const keySelectors = []; @@ -119,29 +77,6 @@ class WalletSign extends React.Component { ); }; - transactionFinalized = () => { - const { transaction, changeSlice, updateChangeNode } = this.props; - - const { txid } = transaction; - const { spent } = this.state; - if (txid !== "" && !spent) { - this.setState({ spent: true }); - const changeAddress = changeSlice.multisig.address; - for (let i = 0; i < transaction.outputs.length; i += 1) { - if (changeAddress === transaction.outputs[i].address) { - updateChangeNode({ - bip32Path: changeSlice.bip32Path, - balanceSats: transaction.outputs[i].amountSats, - }); - break; - } - } - return true; - } - - return false; - }; - handleReturn = () => { const { resetTransaction, resetWalletView } = this.props; resetTransaction(); @@ -160,6 +95,51 @@ class WalletSign extends React.Component { finalizeOutputs(false); setSpendStep(SPEND_STEP_CREATE); }; + + render = () => { + const { spent } = this.state; + return ( + + + + + + {this.renderKeySelectors()} + + + + + + {this.signaturesFinalized() && ( + + + + )} + + {spent && ( + + + + )} + + ); + }; } WalletSign.propTypes = { @@ -176,25 +156,8 @@ WalletSign.propTypes = { setRequiredSigners: PropTypes.func.isRequired, setSpendStep: PropTypes.func.isRequired, signatureImporters: PropTypes.shape({}).isRequired, - spendSlices: PropTypes.func.isRequired, - transaction: PropTypes.shape({ - inputs: PropTypes.arrayOf( - PropTypes.shape({ - change: PropTypes.bool.isRequired, - multisig: PropTypes.shape({ - address: PropTypes.string, - }), - }) - ), - outputs: PropTypes.arrayOf( - PropTypes.shape({ - address: PropTypes.string, - amountSats: PropTypes.shape({}), - }) - ), - txid: PropTypes.string, - }).isRequired, - updateChangeNode: PropTypes.func.isRequired, + updateTxSlices: PropTypes.func.isRequired, + txid: PropTypes.string.isRequired, }; function mapStateToProps(state) { @@ -202,6 +165,7 @@ function mapStateToProps(state) { ...state.wallet, ...state.spend, ...state.quorum, + txid: state.spend.transaction.txid, requiredSigners: state.spend.transaction.requiredSigners, totalSigners: state.spend.transaction.totalSigners, changeSlice: state.wallet.change.nextNode, @@ -211,10 +175,9 @@ function mapStateToProps(state) { const mapDispatchToProps = { finalizeOutputs: finalizeOutputsAction, setRequiredSigners: setRequiredSignersAction, - spendSlices: spendSlicesAction, + updateTxSlices: updateTxSlicesAction, resetTransaction: resetTransactionAction, resetWalletView: resetWalletViewAction, - updateChangeNode: updateChangeSliceActionImport, setSpendStep: setSpendStepAction, }; diff --git a/src/reducers/braidReducer.js b/src/reducers/braidReducer.js index 2d3d250b..6a38b34c 100644 --- a/src/reducers/braidReducer.js +++ b/src/reducers/braidReducer.js @@ -1,7 +1,6 @@ import BigNumber from "bignumber.js"; import { RESET_NODES_SPEND, - SPEND_SLICES, RESET_NODES_FETCH_ERRORS, } from "../actions/walletActions"; import updateState from "./utils"; @@ -60,7 +59,6 @@ function updateSlice(state, action) { }, }, }; - if (typeof action.value.spend !== "undefined") { // TODO (BUCK): I'm not sure this works. If you change // the output value of a spend before spending, this will just @@ -113,21 +111,6 @@ function updateSlice(state, action) { return updatedState; } -function spendSlices(state) { - const updatedState = { ...state }; - Object.values(updatedState.nodes).forEach((node) => { - if (node.spend) { - updatedState.balanceSats = updatedState.balanceSats.minus( - node.balanceSats - ); - node.balanceSats = new BigNumber(0); // eslint-disable-line no-param-reassign - node.spend = false; // eslint-disable-line no-param-reassign - node.utxos = []; // eslint-disable-line no-param-reassign - } - }); - return updatedState; -} - function resetSpend(state) { const updatedState = { ...state }; Object.values(updatedState.nodes).forEach((node) => { @@ -140,8 +123,6 @@ export default (actionType) => (state = initialState, action) => { switch (action.type) { case RESET_NODES_SPEND: return resetSpend(state); - case SPEND_SLICES: - return spendSlices(state); case RESET_NODES_FETCH_ERRORS: return updateState(state, { fetchUTXOsErrors: 0,