Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve takeoffer error handing #5294

Merged
merged 8 commits into from
Mar 19, 2021
Merged

Improve takeoffer error handing #5294

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5c097d3
Adjust protos for takeoffer error handing
ghubstan Mar 10, 2021
067a5c7
Inject CoreContext into OpenOfferManager, adjust test
ghubstan Mar 10, 2021
d0590d9
Adjust grpc & core services to new takeoffer error handling
ghubstan Mar 10, 2021
697cf66
Convert AvailabilityResult to error msg when takeoffer fails
ghubstan Mar 10, 2021
d4d989d
Add missing grpc exception catch blocks
ghubstan Mar 10, 2021
48ecb5b
Derive AvailabilityResult from Task errorMessage
ghubstan Mar 10, 2021
4a68773
Merge branch 'master' into 01-fix-takeoffer-err-handling
ghubstan Mar 14, 2021
1d56d79
Refactor takeoffer reply proto & ErrorMessageHandler
ghubstan Mar 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 35 additions & 27 deletions apitest/src/test/java/bisq/apitest/method/trade/TakeBuyBTCOfferTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,41 +132,49 @@ public void testAlicesConfirmPaymentStarted(final TestInfo testInfo) {
@Test
@Order(3)
public void testBobsConfirmPaymentReceived(final TestInfo testInfo) {
var trade = bobClient.getTrade(tradeId);
bobClient.confirmPaymentReceived(trade.getTradeId());
sleep(3000);

trade = bobClient.getTrade(tradeId);
// Note: offer.state == available
assertEquals(AVAILABLE.name(), trade.getOffer().getState());
EXPECTED_PROTOCOL_STATUS.setState(SELLER_SAW_ARRIVED_PAYOUT_TX_PUBLISHED_MSG)
.setPhase(PAYOUT_PUBLISHED)
.setPayoutPublished(true)
.setFiatReceived(true);
verifyExpectedProtocolStatus(trade);
logTrade(log, testInfo, "Bob's view after confirming fiat payment received", trade);
try {
var trade = bobClient.getTrade(tradeId);
bobClient.confirmPaymentReceived(trade.getTradeId());
sleep(3000);

trade = bobClient.getTrade(tradeId);
// Note: offer.state == available
assertEquals(AVAILABLE.name(), trade.getOffer().getState());
EXPECTED_PROTOCOL_STATUS.setState(SELLER_SAW_ARRIVED_PAYOUT_TX_PUBLISHED_MSG)
.setPhase(PAYOUT_PUBLISHED)
.setPayoutPublished(true)
.setFiatReceived(true);
verifyExpectedProtocolStatus(trade);
logTrade(log, testInfo, "Bob's view after confirming fiat payment received", trade);
} catch (StatusRuntimeException e) {
fail(e);
}
}

@Test
@Order(4)
public void testAlicesKeepFunds(final TestInfo testInfo) {
genBtcBlocksThenWait(1, 1000);
try {
genBtcBlocksThenWait(1, 1000);

var trade = aliceClient.getTrade(tradeId);
logTrade(log, testInfo, "Alice's view before keeping funds", trade);
var trade = aliceClient.getTrade(tradeId);
logTrade(log, testInfo, "Alice's view before keeping funds", trade);

aliceClient.keepFunds(tradeId);
aliceClient.keepFunds(tradeId);

genBtcBlocksThenWait(1, 1000);
genBtcBlocksThenWait(1, 1000);

trade = aliceClient.getTrade(tradeId);
EXPECTED_PROTOCOL_STATUS.setState(BUYER_RECEIVED_PAYOUT_TX_PUBLISHED_MSG)
.setPhase(PAYOUT_PUBLISHED);
verifyExpectedProtocolStatus(trade);
logTrade(log, testInfo, "Alice's view after keeping funds", trade);
BtcBalanceInfo currentBalance = aliceClient.getBtcBalances();
log.debug("{} Alice's current available balance: {} BTC",
testName(testInfo),
formatSatoshis(currentBalance.getAvailableBalance()));
trade = aliceClient.getTrade(tradeId);
EXPECTED_PROTOCOL_STATUS.setState(BUYER_RECEIVED_PAYOUT_TX_PUBLISHED_MSG)
.setPhase(PAYOUT_PUBLISHED);
verifyExpectedProtocolStatus(trade);
logTrade(log, testInfo, "Alice's view after keeping funds", trade);
BtcBalanceInfo currentBalance = aliceClient.getBtcBalances();
log.debug("{} Alice's current available balance: {} BTC",
testName(testInfo),
formatSatoshis(currentBalance.getAvailableBalance()));
} catch (StatusRuntimeException e) {
fail(e);
}
}
}
72 changes: 40 additions & 32 deletions apitest/src/test/java/bisq/apitest/method/trade/TakeSellBTCOfferTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,42 +134,50 @@ public void testBobsConfirmPaymentStarted(final TestInfo testInfo) {
@Test
@Order(3)
public void testAlicesConfirmPaymentReceived(final TestInfo testInfo) {
var trade = aliceClient.getTrade(tradeId);
aliceClient.confirmPaymentReceived(trade.getTradeId());
sleep(3000);

trade = aliceClient.getTrade(tradeId);
assertEquals(OFFER_FEE_PAID.name(), trade.getOffer().getState());
EXPECTED_PROTOCOL_STATUS.setState(SELLER_SAW_ARRIVED_PAYOUT_TX_PUBLISHED_MSG)
.setPhase(PAYOUT_PUBLISHED)
.setPayoutPublished(true)
.setFiatReceived(true);
verifyExpectedProtocolStatus(trade);
logTrade(log, testInfo, "Alice's view after confirming fiat payment received", trade);
try {
var trade = aliceClient.getTrade(tradeId);
aliceClient.confirmPaymentReceived(trade.getTradeId());
sleep(3000);

trade = aliceClient.getTrade(tradeId);
assertEquals(OFFER_FEE_PAID.name(), trade.getOffer().getState());
EXPECTED_PROTOCOL_STATUS.setState(SELLER_SAW_ARRIVED_PAYOUT_TX_PUBLISHED_MSG)
.setPhase(PAYOUT_PUBLISHED)
.setPayoutPublished(true)
.setFiatReceived(true);
verifyExpectedProtocolStatus(trade);
logTrade(log, testInfo, "Alice's view after confirming fiat payment received", trade);
} catch (StatusRuntimeException e) {
fail(e);
}
}

@Test
@Order(4)
public void testBobsBtcWithdrawalToExternalAddress(final TestInfo testInfo) {
genBtcBlocksThenWait(1, 1000);

var trade = bobClient.getTrade(tradeId);
logTrade(log, testInfo, "Bob's view before withdrawing funds to external wallet", trade);

String toAddress = bitcoinCli.getNewBtcAddress();
bobClient.withdrawFunds(tradeId, toAddress, WITHDRAWAL_TX_MEMO);

genBtcBlocksThenWait(1, 1000);

trade = bobClient.getTrade(tradeId);
EXPECTED_PROTOCOL_STATUS.setState(WITHDRAW_COMPLETED)
.setPhase(WITHDRAWN)
.setWithdrawn(true);
verifyExpectedProtocolStatus(trade);
logTrade(log, testInfo, "Bob's view after withdrawing funds to external wallet", trade);
BtcBalanceInfo currentBalance = bobClient.getBtcBalances();
log.debug("{} Bob's current available balance: {} BTC",
testName(testInfo),
formatSatoshis(currentBalance.getAvailableBalance()));
try {
genBtcBlocksThenWait(1, 1000);

var trade = bobClient.getTrade(tradeId);
logTrade(log, testInfo, "Bob's view before withdrawing funds to external wallet", trade);

String toAddress = bitcoinCli.getNewBtcAddress();
bobClient.withdrawFunds(tradeId, toAddress, WITHDRAWAL_TX_MEMO);

genBtcBlocksThenWait(1, 1000);

trade = bobClient.getTrade(tradeId);
EXPECTED_PROTOCOL_STATUS.setState(WITHDRAW_COMPLETED)
.setPhase(WITHDRAWN)
.setWithdrawn(true);
verifyExpectedProtocolStatus(trade);
logTrade(log, testInfo, "Bob's view after withdrawing funds to external wallet", trade);
BtcBalanceInfo currentBalance = bobClient.getBtcBalances();
log.debug("{} Bob's current available balance: {} BTC",
testName(testInfo),
formatSatoshis(currentBalance.getAvailableBalance()));
} catch (StatusRuntimeException e) {
fail(e);
}
}
}
72 changes: 52 additions & 20 deletions cli/src/main/java/bisq/cli/GrpcClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,13 @@
import java.util.List;
import java.util.stream.Collectors;

import lombok.extern.slf4j.Slf4j;

import static java.util.Comparator.comparing;


@SuppressWarnings("ResultOfMethodCallIgnored")
@Slf4j
public final class GrpcClient {

private final GrpcStubs grpcStubs;
Expand Down Expand Up @@ -181,23 +185,23 @@ public TxInfo getTransaction(String txId) {
}

public OfferInfo createFixedPricedOffer(String direction,
String currencyCode,
long amount,
long minAmount,
String fixedPrice,
double securityDeposit,
String paymentAcctId,
String makerFeeCurrencyCode) {
return createOffer(direction,
currencyCode,
amount,
minAmount,
false,
fixedPrice,
0.00,
securityDeposit,
paymentAcctId,
makerFeeCurrencyCode);
String currencyCode,
long amount,
long minAmount,
String fixedPrice,
double securityDeposit,
String paymentAcctId,
String makerFeeCurrencyCode) {
return createOffer(direction,
currencyCode,
amount,
minAmount,
false,
fixedPrice,
0.00,
securityDeposit,
paymentAcctId,
makerFeeCurrencyCode);
}

public OfferInfo createMarketBasedPricedOffer(String direction,
Expand Down Expand Up @@ -289,8 +293,8 @@ public List<OfferInfo> getMyOffers(String direction, String currencyCode) {
}

public List<OfferInfo> getMyOffersSortedByDate(String direction, String currencyCode) {
var offers = getMyOffers(direction, currencyCode);
return offers.isEmpty() ? offers : sortOffersByDate(offers);
var offers = getMyOffers(direction, currencyCode);
return offers.isEmpty() ? offers : sortOffersByDate(offers);
}

public OfferInfo getMostRecentOffer(String direction, String currencyCode) {
Expand All @@ -311,7 +315,35 @@ public TradeInfo takeOffer(String offerId, String paymentAccountId, String taker
.setPaymentAccountId(paymentAccountId)
.setTakerFeeCurrencyCode(takerFeeCurrencyCode)
.build();
return grpcStubs.tradesService.takeOffer(request).getTrade();
var reply = grpcStubs.tradesService.takeOffer(request);
if (reply.hasTrade()) {
return reply.getTrade();
} else {
// If there is no trade, there should be a reason in the AvailabilityResult.
// Convert the enum to a user error message before throwing the exception.
switch (reply.getAvailabilityResult()) {
case MARKET_PRICE_NOT_AVAILABLE:
throw new IllegalStateException("could not take offer because market price for calculating trade price is unavailable");
case PRICE_OUT_OF_TOLERANCE:
throw new IllegalStateException("could not take offer because taker's price is outside tolerance");
case PRICE_CHECK_FAILED:
throw new IllegalStateException("could not take offer because trade price check failed");
case NO_ARBITRATORS:
throw new IllegalStateException("could not take offer because no arbitrators are available");
case NO_MEDIATORS:
throw new IllegalStateException("could not take offer because no mediators are available");
case NO_REFUND_AGENTS:
throw new IllegalStateException("could not take offer because no refund agents are available");
case USER_IGNORED:
throw new IllegalStateException("could not take offer from ignored user");
case MAKER_DENIED_API_USER:
throw new IllegalStateException("could not take offer because maker is api user");
case UNCONF_TX_LIMIT_HIT:
throw new IllegalStateException("could not take offer because you have too many unconfirmed transactions at this moment");
default:
throw new IllegalStateException("programmer error: could not take offer for unknown reason");
}
}
}

public TradeInfo getTrade(String tradeId) {
Expand Down
7 changes: 5 additions & 2 deletions core/src/main/java/bisq/core/api/CoreApi.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@

import bisq.common.app.Version;
import bisq.common.config.Config;
import bisq.common.handlers.ErrorMessageHandler;
import bisq.common.handlers.ResultHandler;

import org.bitcoinj.core.Coin;
Expand Down Expand Up @@ -224,12 +225,14 @@ public void getMarketPrice(String currencyCode, Consumer<Double> resultHandler)
public void takeOffer(String offerId,
String paymentAccountId,
String takerFeeCurrencyCode,
Consumer<Trade> resultHandler) {
Consumer<Trade> resultHandler,
ErrorMessageHandler errorMessageHandler) {
Offer offer = coreOffersService.getOffer(offerId);
coreTradesService.takeOffer(offer,
paymentAccountId,
takerFeeCurrencyCode,
resultHandler);
resultHandler,
errorMessageHandler);
}

public void confirmPaymentStarted(String tradeId) {
Expand Down
10 changes: 5 additions & 5 deletions core/src/main/java/bisq/core/api/CoreTradesService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
import bisq.core.user.User;
import bisq.core.util.validation.BtcAddressValidator;

import bisq.common.handlers.ErrorMessageHandler;

import org.bitcoinj.core.Coin;

import javax.inject.Inject;
Expand Down Expand Up @@ -86,7 +88,8 @@ public CoreTradesService(CoreContext coreContext,
void takeOffer(Offer offer,
String paymentAccountId,
String takerFeeCurrencyCode,
Consumer<Trade> resultHandler) {
Consumer<Trade> resultHandler,
ErrorMessageHandler errorMessageHandler) {
coreWalletsService.verifyWalletsAreAvailable();
coreWalletsService.verifyEncryptedWalletIsUnlocked();

Expand Down Expand Up @@ -114,10 +117,7 @@ void takeOffer(Offer offer,
useSavingsWallet,
coreContext.isApiUser(),
resultHandler::accept,
errorMessage -> {
log.error(errorMessage);
throw new IllegalStateException(errorMessage);
}
errorMessageHandler
);
}

Expand Down
9 changes: 5 additions & 4 deletions core/src/main/java/bisq/core/offer/AvailabilityResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,12 @@ public enum AvailabilityResult {
OFFER_TAKEN,
PRICE_OUT_OF_TOLERANCE,
MARKET_PRICE_NOT_AVAILABLE,
NO_ARBITRATORS,
@SuppressWarnings("unused") NO_ARBITRATORS,
NO_MEDIATORS,
USER_IGNORED,
MISSING_MANDATORY_CAPABILITY,
NO_REFUND_AGENTS,
@SuppressWarnings("unused") MISSING_MANDATORY_CAPABILITY,
@SuppressWarnings("unused") NO_REFUND_AGENTS,
UNCONF_TX_LIMIT_HIT,
MAKER_DENIED_API_USER
MAKER_DENIED_API_USER,
PRICE_CHECK_FAILED
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm, this would fail to parse by old clients and thus use the first element in the enum, which is UNKNOWN_FAILURE, which makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed this by sending protobuf.AvailabilityResult.PRICE_CHECK_FAILED(13) to a client that does not have that proto enum value, and the client received UNKNOWN_ENUM_VALUE_AvailabilityResult_13.

}
12 changes: 10 additions & 2 deletions core/src/main/java/bisq/core/offer/OpenOfferManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

package bisq.core.offer;

import bisq.core.api.CoreContext;
import bisq.core.btc.wallet.BsqWalletService;
import bisq.core.btc.wallet.BtcWalletService;
import bisq.core.btc.wallet.TradeWalletService;
Expand Down Expand Up @@ -101,6 +102,7 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe
private static final long REPUBLISH_INTERVAL_MS = TimeUnit.MINUTES.toMillis(40);
private static final long REFRESH_INTERVAL_MS = TimeUnit.MINUTES.toMillis(6);

private final CoreContext coreContext;
private final CreateOfferService createOfferService;
private final KeyRing keyRing;
private final User user;
Expand Down Expand Up @@ -133,7 +135,8 @@ public class OpenOfferManager implements PeerManager.Listener, DecryptedDirectMe
///////////////////////////////////////////////////////////////////////////////////////////

@Inject
public OpenOfferManager(CreateOfferService createOfferService,
public OpenOfferManager(CoreContext coreContext,
CreateOfferService createOfferService,
KeyRing keyRing,
User user,
P2PService p2PService,
Expand All @@ -152,6 +155,7 @@ public OpenOfferManager(CreateOfferService createOfferService,
FilterManager filterManager,
Broadcaster broadcaster,
PersistenceManager<TradableList<OpenOffer>> persistenceManager) {
this.coreContext = coreContext;
this.createOfferService = createOfferService;
this.keyRing = keyRing;
this.user = user;
Expand Down Expand Up @@ -658,7 +662,11 @@ private void handleOfferAvailabilityRequest(OfferAvailabilityRequest request, No
availabilityResult = AvailabilityResult.MARKET_PRICE_NOT_AVAILABLE;
} catch (Throwable e) {
log.warn("Trade price check failed. " + e.getMessage());
availabilityResult = AvailabilityResult.UNKNOWN_FAILURE;
if (coreContext.isApiUser())
// Give api user something more than 'unknown_failure'.
availabilityResult = AvailabilityResult.PRICE_CHECK_FAILED;
else
availabilityResult = AvailabilityResult.UNKNOWN_FAILURE;
}
} else {
availabilityResult = AvailabilityResult.USER_IGNORED;
Expand Down
Loading