Skip to content

Commit

Permalink
AMMBid: use tecINTERNAL for 'impossible' errors (#4674)
Browse files Browse the repository at this point in the history
Modify two error cases in AMMBid transactor to return `tecINTERNAL` to
more clearly indicate that these errors should not be possible unless
operating in unforeseen circumstances. It likely indicates a bug.

The log level has been updated to `fatal()` since it indicates a
(potentially network-wide) unexpected condition when either of these
errors occurs.

Details:

The two specific transaction error cases changed are:

- `tecAMM_BALANCE` - In this case, this error (total LP Tokens
  outstanding is lower than the amount to be burned for the bid) is a
  subset of the case where the user doesn't have enough LP Tokens to pay
  for the bid. When this case is reached, the bidder's LP Tokens balance
  has already been checked first. The user's LP Tokens should always be
  a subset of total LP Tokens issued, so this should be impossible.
- `tecINSUFFICIENT_PAYMENT` - In this case, the amount to be refunded as
  a result of the bid is greater than the price paid for the auction
  slot. This should never occur unless something is wrong with the math
  for calculating the refund amount.

Both error cases in question are "defense in depth" measures meant to
protect against making things worse if the code has already reached a
state that is supposed to be impossible, likely due to a bug elsewhere.

Such "shouldn't ever occur" checks should use an error code that
categorically indicates a larger problem. This is similar to how
`tecINVARIANT_FAILED` is a warning sign that something went wrong and
likely could've been worse, but since there isn't an Invariant Check
applying here, `tecINTERNAL` is the appropriate error code.

This is "debatably" a transaction processing change since it could
hypothetically change how transactions are processed if there's a bug we
don't know about.
  • Loading branch information
mDuo13 authored Sep 1, 2023
1 parent f31c50d commit a61a88e
Showing 1 changed file with 9 additions and 6 deletions.
15 changes: 9 additions & 6 deletions src/ripple/app/tx/impl/AMMBid.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -223,9 +223,11 @@ applyBid(
lptAMMBalance, toSTAmount(lptAMMBalance.issue(), burn), false);
if (saBurn >= lptAMMBalance)
{
JLOG(ctx_.journal.debug())
<< "AMM Bid: invalid burn " << burn << " " << lptAMMBalance;
return tecAMM_BALANCE;
// This error case should never occur.
JLOG(ctx_.journal.fatal())
<< "AMM Bid: LP Token burn exceeds AMM balance " << burn << " "
<< lptAMMBalance;
return tecINTERNAL;
}
auto res =
redeemIOU(sb, account_, saBurn, lpTokens.issue(), ctx_.journal);
Expand Down Expand Up @@ -316,9 +318,10 @@ applyBid(
auto const refund = fractionRemaining * pricePurchased;
if (refund > *payPrice)
{
JLOG(ctx_.journal.debug())
<< "AMM Bid: invalid refund " << refund << " " << *payPrice;
return {tecINSUFFICIENT_PAYMENT, false};
// This error case should never occur.
JLOG(ctx_.journal.fatal()) << "AMM Bid: refund exceeds payPrice "
<< refund << " " << *payPrice;
return {tecINTERNAL, false};
}
res = accountSend(
sb,
Expand Down

0 comments on commit a61a88e

Please sign in to comment.