Skip to content

Commit

Permalink
Fix #3486 "Order cancellation fee error" (#3492)
Browse files Browse the repository at this point in the history 
* fix 3486

* introduce raiseIfInsufficient to reduce duplicate code

* default to given preference if nothing else has balance

* forgot to adjust myopenorders catch

* fix  subscript overflow

* fix 3486

Co-authored-by: Stefan <toleantech@gmail.com>
  • Loading branch information
@xiangxn @sschiessl-bcp
xiangxn and sschiessl-bcp committed Apr 23, 2022
1 parent 9750065 commit 1795b13
Show file tree
Hide file tree
Showing 3 changed files with 105 additions and 35 deletions.
40 changes: 26 additions & 14 deletions app/actions/MarketsActions.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,10 +143,10 @@ class MarketsActions {

subscribeMarket(base, quote, bucketSize, groupedOrderLimit) {
/*
* DataFeed will call subscribeMarket with undefined groupedOrderLimit,
* so we keep track of the last value used and use that instead in that
* case
*/
* DataFeed will call subscribeMarket with undefined groupedOrderLimit,
* so we keep track of the last value used and use that instead in that
* case
*/
if (typeof groupedOrderLimit === "undefined")
groupedOrderLimit = currentGroupedOrderLimit;
else currentGroupedOrderLimit = groupedOrderLimit;
Expand All @@ -164,16 +164,16 @@ class MarketsActions {
return dispatch => {
let subscription = (marketId, subResult) => {
/*
** When switching markets rapidly we might receive sub notifications
** from the previous markets, in that case disregard them
*/
** When switching markets rapidly we might receive sub notifications
** from the previous markets, in that case disregard them
*/
if (marketId !== currentMarket) {
return;
}
/* In the case of many market notifications arriving at the same time,
* we queue them in a batch here and dispatch them all at once at a frequency
* defined by "subBatchTime"
*/
* we queue them in a batch here and dispatch them all at once at a frequency
* defined by "subBatchTime"
*/
if (!dispatchSubTimeout) {
subBatchResults = subBatchResults.concat(subResult);

Expand Down Expand Up @@ -703,7 +703,9 @@ class MarketsActions {
}

cancelLimitOrder(accountID, orderID) {
// Set the fee asset to use
// FIXME we need a global approach how gee asset id is selected,
// this is only doing it for the cancel action, but ideally,
// all fee selection in the UI have the same logic
let fee_asset_id = accountUtils.getFinalFeeAsset(
accountID,
"limit_order_cancel"
Expand All @@ -728,17 +730,27 @@ class MarketsActions {
console.log("cancelLimitOrders", accountID, orderIDs);
}
let tr = WalletApi.new_transaction();
let balances = accountUtils.getAccountBalances(accountID);
for (let i = 0; i < orderIDs.length; i++) {
let id = orderIDs[i];
let fallbackFeeAsset =
typeof fallbackFeeAssets === "string"
? fallbackFeeAssets
: fallbackFeeAssets[i];
let {fees} = accountUtils.getPossibleFees(
accountID,
"limit_order_cancel"
);
let fee_asset_id = accountUtils.getFinalFeeAsset(
accountID,
"limit_order_cancel",
fallbackFeeAsset
);
balances[fee_asset_id] -= fees[fee_asset_id];
// check balance
Object.keys(balances).forEach(key => {
if (balances[key] < 0) throw "Insufficient balance: " + key;
});
tr.add_type_operation("limit_order_cancel", {
fee: {
amount: 0,
Expand All @@ -756,9 +768,9 @@ class MarketsActions {
cancelLimitOrderSuccess(ids) {
return dispatch => {
/* In the case of many cancel orders being issued at the same time,
* we batch them here and dispatch them all at once at a frequency
* defined by "dispatchCancelTimeout"
*/
* we batch them here and dispatch them all at once at a frequency
* defined by "dispatchCancelTimeout"
*/
if (!dispatchCancelTimeout) {
cancelBatchIDs = cancelBatchIDs.concat(ids);
dispatchCancelTimeout = setTimeout(() => {
Expand Down
8 changes: 7 additions & 1 deletion app/components/Exchange/MyOpenOrders.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {LimitOrder, CallOrder} from "common/MarketClasses";
import ReactTooltip from "react-tooltip";
import {Button} from "bitshares-ui-style-guide";
import {MarketsOrderView, MarketOrdersRowView} from "./View/MarketOrdersView";
import NotificationActions from "actions/NotificationActions";

class MarketOrdersRow extends React.Component {
shouldComponentUpdate(nextProps) {
Expand Down Expand Up @@ -229,7 +230,12 @@ class MarketOrders extends React.Component {
this.resetSelected();
})
.catch(err => {
console.log("cancel orders error:", err);
if (
typeof err === "string" &&
err.startsWith("Insufficient balance")
)
NotificationActions.error(err);
else console.log("cancel orders error:", err);
});
});
}
Expand Down
92 changes: 72 additions & 20 deletions app/lib/common/account_utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
scamAccountsBittrex,
scamAccountsOther
} from "./scamAccounts";
import SettingsStore from "stores/SettingsStore";

export default class AccountUtils {
/**
Expand All @@ -29,8 +30,14 @@ export default class AccountUtils {
return feePool >= fee;
}

static getPossibleFees(account, operation) {
/**
* Returns all assets that the user can actually use to pay the fee given by the operation.
* This estimates the fee in core asset, checks user balance and fee pool of all balances
* and decides which one may be used.
*/
static getPossibleFees(account, operation, raiseIfInsufficient = false) {
let core = ChainStore.getAsset("1.3.0");

account =
!account || account.toJS ? account : ChainStore.getAccount(account);

Expand All @@ -46,18 +53,15 @@ export default class AccountUtils {
let fee = estimateFee(operation, null, globalObject);

let accountBalances = account.get("balances");
if (!accountBalances) {
return {assets: ["1.3.0"], fees: {"1.3.0": 0}};
if (!accountBalances || Object.keys(accountBalances).length == 0) {
return {assets: [], fees: {}};
}

accountBalances.forEach((balanceID, assetID) => {
let balanceObject = ChainStore.getObject(balanceID);
let balance = balanceObject
? parseInt(balanceObject.get("balance"), 10)
: 0;
for (const [assetID, balance] of Object.entries(
this.getAccountBalances(account)
)) {
let hasBalance = false,
eqFee;

if (assetID === "1.3.0" && balance >= fee) {
hasBalance = true;
} else if (balance && ChainStore.getAsset(assetID)) {
Expand All @@ -84,23 +88,71 @@ export default class AccountUtils {
assets.push(assetID);
fees[assetID] = eqFee ? eqFee : fee;
}
});
}

// if requested, raise exception if no fees found
if (raiseIfInsufficient && assets.length == 0) {
throw "Insufficient balance for fee";
}

return {assets, fees};
}

static getFinalFeeAsset(account, operation, fee_asset_id = "1.3.0") {
let {assets: feeAssets} = this.getPossibleFees(account, operation);
if (feeAssets.length === 1) {
fee_asset_id = feeAssets[0];
} else if (
feeAssets.length > 0 &&
feeAssets.indexOf(fee_asset_id) === -1
) {
fee_asset_id = feeAssets[0];
/**
* Returns the fee asset id that can be used to pay for the fee.
* It will try to use the globally set preference, and if not possible the given
* preference (usually defaults to core asset), and if also not possible,
* just any asset of the user that has sufficient balance and has a funded feepool.
* If nothing is available, it returns the globally set default, or raises an error.
*/
static getFinalFeeAsset(
account,
operation,
feeAssetId = "1.3.0",
raiseIfInsufficient = false
) {
// user can set a default in the settings
let default_fee_asset_symbol = SettingsStore.getSetting("fee_asset");
let default_fee_asset = ChainStore.getAsset(
default_fee_asset_symbol
).toJS();
let {assets: feeAssets} = this.getPossibleFees(
account,
operation,
raiseIfInsufficient
);
if (feeAssets.length > 0) {
if (
feeAssets.indexOf(default_fee_asset.id) !== -1
) {
return default_fee_asset.id;
} else if (
feeAssets.indexOf(feeAssetId) !== -1
) {
return feeAssetId;
} else {
// take any that allows to pay the fee
return feeAssets[0];
}
} else {
// can't pay fee, show user his chosen default
return default_fee_asset.id;
}
}

return fee_asset_id;
static getAccountBalances(account) {
account =
!account || account.toJS ? account : ChainStore.getAccount(account);
let accountBalances = account.get("balances");
let balances = {};
accountBalances.forEach((balanceID, assetID) => {
let balanceObject = ChainStore.getObject(balanceID);
let balance = balanceObject
? parseInt(balanceObject.get("balance"), 10)
: 0;
balances[assetID] = balance;
});
return balances;
}

static isKnownScammer(account) {
Expand Down

0 comments on commit 1795b13

Please sign in to comment.