From 6bde12ba405ebe63a85e67d4d2f537274dd8056d Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Thu, 1 Apr 2021 16:40:08 -0300 Subject: [PATCH] Improve takeoffer output and failure reason messaging - Added AvailabilityResultWithDescription proto for better takeoffer failure msgs. - Added VerifyBsqSentToAddress impl to api, but don't expose to CLI yet. - Show BSQ Buyer Address in gettrade output (changed cli output formatting classes). - Fixed api.model.PaymentAccountPayloadInfo altcoin instant acct support bug --- .../java/bisq/cli/ColumnHeaderConstants.java | 1 + .../main/java/bisq/cli/CurrencyFormat.java | 2 +- cli/src/main/java/bisq/cli/GrpcClient.java | 11 ++++- cli/src/main/java/bisq/cli/TradeFormat.java | 45 +++++++++++++++++-- .../bisq/core/api/CoreWalletsService.java | 42 +++++++++++++++++ .../api/model/PaymentAccountPayloadInfo.java | 13 ++++-- .../daemon/grpc/GrpcErrorMessageHandler.java | 32 +++++++++---- proto/src/main/proto/grpc.proto | 20 +++++++-- 8 files changed, 145 insertions(+), 21 deletions(-) diff --git a/cli/src/main/java/bisq/cli/ColumnHeaderConstants.java b/cli/src/main/java/bisq/cli/ColumnHeaderConstants.java index 0ad303dd64..775221b5ed 100644 --- a/cli/src/main/java/bisq/cli/ColumnHeaderConstants.java +++ b/cli/src/main/java/bisq/cli/ColumnHeaderConstants.java @@ -51,6 +51,7 @@ class ColumnHeaderConstants { static final String COL_HEADER_PRICE = "Price in %-3s for 1 BTC"; static final String COL_HEADER_PRICE_OF_ALTCOIN = "Price in BTC for 1 %-3s"; static final String COL_HEADER_TRADE_AMOUNT = padStart("Amount(%-3s)", 12, ' '); + static final String COL_HEADER_TRADE_BSQ_BUYER_ADDRESS = "BSQ Buyer Address"; static final String COL_HEADER_TRADE_BUYER_COST = padEnd("Buyer Cost(%-3s)", 15, ' '); static final String COL_HEADER_TRADE_DEPOSIT_CONFIRMED = "Deposit Confirmed"; static final String COL_HEADER_TRADE_DEPOSIT_PUBLISHED = "Deposit Published"; diff --git a/cli/src/main/java/bisq/cli/CurrencyFormat.java b/cli/src/main/java/bisq/cli/CurrencyFormat.java index 97d16fc6bd..baeeb775f9 100644 --- a/cli/src/main/java/bisq/cli/CurrencyFormat.java +++ b/cli/src/main/java/bisq/cli/CurrencyFormat.java @@ -57,7 +57,7 @@ public class CurrencyFormat { return BSQ_FORMAT.format(BigDecimal.valueOf(sats).divide(BSQ_SATOSHI_DIVISOR)); } - public static String formatBsqSendAmount(long bsqSats) { + public static String formatBsqAmount(long bsqSats) { // BSQ sats = trade.getOffer().getVolume() NUMBER_FORMAT.setMinimumFractionDigits(2); NUMBER_FORMAT.setMaximumFractionDigits(2); diff --git a/cli/src/main/java/bisq/cli/GrpcClient.java b/cli/src/main/java/bisq/cli/GrpcClient.java index e9e87f0ed6..b238c17e5f 100644 --- a/cli/src/main/java/bisq/cli/GrpcClient.java +++ b/cli/src/main/java/bisq/cli/GrpcClient.java @@ -62,6 +62,7 @@ import bisq.proto.grpc.TxFeeRateInfo; import bisq.proto.grpc.TxInfo; import bisq.proto.grpc.UnlockWalletRequest; import bisq.proto.grpc.UnsetTxFeeRatePreferenceRequest; +import bisq.proto.grpc.VerifyBsqSentToAddressRequest; import bisq.proto.grpc.WithdrawFundsRequest; import protobuf.PaymentAccount; @@ -166,6 +167,14 @@ public final class GrpcClient { return grpcStubs.walletsService.sendBtc(request).getTxInfo(); } + public boolean verifyBsqSentToAddress(String address, String amount) { + var request = VerifyBsqSentToAddressRequest.newBuilder() + .setAddress(address) + .setAmount(amount) + .build(); + return grpcStubs.walletsService.verifyBsqSentToAddress(request).getIsAmountReceived(); + } + public TxFeeRateInfo getTxFeeRate() { var request = GetTxFeeRateRequest.newBuilder().build(); return grpcStubs.walletsService.getTxFeeRate(request).getTxFeeRateInfo(); @@ -367,7 +376,7 @@ public final class GrpcClient { if (reply.hasTrade()) return reply.getTrade(); else - throw new IllegalStateException(reply.getAvailabilityResultDescription()); + throw new IllegalStateException(reply.getFailureReason().getDescription()); } public TradeInfo getTrade(String tradeId) { diff --git a/cli/src/main/java/bisq/cli/TradeFormat.java b/cli/src/main/java/bisq/cli/TradeFormat.java index 04816eca24..d11f8847be 100644 --- a/cli/src/main/java/bisq/cli/TradeFormat.java +++ b/cli/src/main/java/bisq/cli/TradeFormat.java @@ -17,6 +17,7 @@ package bisq.cli; +import bisq.proto.grpc.ContractInfo; import bisq.proto.grpc.TradeInfo; import com.google.common.annotations.VisibleForTesting; @@ -58,6 +59,10 @@ public class TradeFormat { "%" + (COL_HEADER_TRADE_TAKER_FEE.length() + 2) + "s" : ""; + boolean showBsqBuyerAddress = shouldShowBqsBuyerAddress(tradeInfo, isTaker); + Supplier bsqBuyerAddressHeader = () -> showBsqBuyerAddress ? COL_HEADER_TRADE_BSQ_BUYER_ADDRESS : ""; + Supplier bsqBuyerAddressHeaderSpec = () -> showBsqBuyerAddress ? "%s" : ""; + String headersFormat = padEnd(COL_HEADER_TRADE_SHORT_ID, shortIdColWidth, ' ') + COL_HEADER_DELIMITER + padEnd(COL_HEADER_TRADE_ROLE, roleColWidth, ' ') + COL_HEADER_DELIMITER + priceHeader.apply(tradeInfo) + COL_HEADER_DELIMITER // includes %s -> currencyCode @@ -73,6 +78,7 @@ public class TradeFormat { + COL_HEADER_TRADE_PAYMENT_RECEIVED + COL_HEADER_DELIMITER + COL_HEADER_TRADE_PAYOUT_PUBLISHED + COL_HEADER_DELIMITER + COL_HEADER_TRADE_WITHDRAWN + COL_HEADER_DELIMITER + + bsqBuyerAddressHeader.get() + "%n"; String counterCurrencyCode = tradeInfo.getOffer().getCounterCurrencyCode(); @@ -100,14 +106,16 @@ public class TradeFormat { + " %-" + (COL_HEADER_TRADE_PAYMENT_SENT.length() - 1) + "s" // left + " %-" + (COL_HEADER_TRADE_PAYMENT_RECEIVED.length() - 1) + "s" // left + " %-" + COL_HEADER_TRADE_PAYOUT_PUBLISHED.length() + "s" // lt justify - + " %-" + COL_HEADER_TRADE_WITHDRAWN.length() + "s"; // lt justify + + " %-" + (COL_HEADER_TRADE_WITHDRAWN.length() + 2) + "s" + + bsqBuyerAddressHeaderSpec.get(); - return headerLine + formatTradeData(colDataFormat, tradeInfo, isTaker); + return headerLine + formatTradeData(colDataFormat, tradeInfo, isTaker, showBsqBuyerAddress); } private static String formatTradeData(String format, TradeInfo tradeInfo, - boolean isTaker) { + boolean isTaker, + boolean showBsqBuyerAddress) { return String.format(format, tradeInfo.getShortId(), tradeInfo.getRole(), @@ -121,7 +129,8 @@ public class TradeFormat { tradeInfo.getIsFiatSent() ? YES : NO, tradeInfo.getIsFiatReceived() ? YES : NO, tradeInfo.getIsPayoutPublished() ? YES : NO, - tradeInfo.getIsWithdrawn() ? YES : NO); + tradeInfo.getIsWithdrawn() ? YES : NO, + bsqReceiveAddress.apply(tradeInfo, showBsqBuyerAddress)); } private static final Function priceHeader = (t) -> @@ -181,4 +190,32 @@ public class TradeFormat { t.getOffer().getBaseCurrencyCode().equals("BTC") ? formatOfferVolume(t.getOffer().getVolume()) : formatSatoshis(t.getTradeAmountAsLong()); + + private static final BiFunction bsqReceiveAddress = (t, showBsqBuyerAddress) -> { + if (showBsqBuyerAddress) { + ContractInfo contract = t.getContract(); + boolean isBuyerMakerAndSellerTaker = contract.getIsBuyerMakerAndSellerTaker(); + return isBuyerMakerAndSellerTaker // (is BTC buyer / maker) + ? contract.getTakerPaymentAccountPayload().getAddress() + : contract.getMakerPaymentAccountPayload().getAddress(); + } else { + return ""; + } + }; + + private static boolean shouldShowBqsBuyerAddress(TradeInfo tradeInfo, boolean isTaker) { + if (tradeInfo.getOffer().getBaseCurrencyCode().equals("BTC")) { + return false; + } else { + ContractInfo contract = tradeInfo.getContract(); + // Do not forget buyer and seller refer to BTC buyer and seller, not BSQ + // buyer and seller. If you are buying BSQ, you are the (BTC) seller. + boolean isBuyerMakerAndSellerTaker = contract.getIsBuyerMakerAndSellerTaker(); + if (isTaker) { + return !isBuyerMakerAndSellerTaker; + } else { + return isBuyerMakerAndSellerTaker; + } + } + } } diff --git a/core/src/main/java/bisq/core/api/CoreWalletsService.java b/core/src/main/java/bisq/core/api/CoreWalletsService.java index d6aa8a240a..4af8c3dd12 100644 --- a/core/src/main/java/bisq/core/api/CoreWalletsService.java +++ b/core/src/main/java/bisq/core/api/CoreWalletsService.java @@ -52,8 +52,10 @@ import org.bitcoinj.core.Address; import org.bitcoinj.core.Coin; import org.bitcoinj.core.InsufficientMoneyException; import org.bitcoinj.core.LegacyAddress; +import org.bitcoinj.core.NetworkParameters; import org.bitcoinj.core.Transaction; import org.bitcoinj.core.TransactionConfidence; +import org.bitcoinj.core.TransactionOutput; import org.bitcoinj.crypto.KeyCrypterScrypt; import javax.inject.Inject; @@ -75,6 +77,7 @@ import java.util.List; import java.util.Optional; import java.util.Set; import java.util.function.Function; +import java.util.function.Predicate; import java.util.stream.Collectors; import lombok.extern.slf4j.Slf4j; @@ -145,6 +148,10 @@ class CoreWalletsService { return tempAesKey; } + NetworkParameters getNetworkParameters() { + return btcWalletService.getWallet().getContext().getParams(); + } + BalancesInfo getBalances(String currencyCode) { verifyWalletCurrencyCodeIsValid(currencyCode); verifyWalletsAreAvailable(); @@ -305,6 +312,41 @@ class CoreWalletsService { } } + boolean verifyBsqSentToAddress(String address, String amount) { + Address receiverAddress = getValidBsqLegacyAddress(address); + NetworkParameters networkParameters = getNetworkParameters(); + Predicate isTxOutputAddressMatch = (txOut) -> + txOut.getScriptPubKey().getToAddress(networkParameters).equals(receiverAddress); + Coin coinValue = parseToCoin(amount, bsqFormatter); + Predicate isTxOutputValueMatch = (txOut) -> + txOut.getValue().longValue() == coinValue.longValue(); + List spendableBsqTxOutputs = bsqWalletService.getSpendableBsqTransactionOutputs(); + + log.info("Searching {} spendable tx outputs for matching address {} and value {}:", + spendableBsqTxOutputs.size(), + address, + coinValue.toPlainString()); + long numMatches = 0; + for (TransactionOutput txOut : spendableBsqTxOutputs) { + if (isTxOutputAddressMatch.test(txOut) && isTxOutputValueMatch.test(txOut)) { + log.info("\t\tTx {} output has matching address {} and value {}.", + txOut.getParentTransaction().getTxId(), + address, + txOut.getValue().toPlainString()); + numMatches++; + } + } + if (numMatches > 1) { + log.warn("{} tx outputs matched address {} and value {}, could be a" + + " false positive BSQ payment verification result.", + numMatches, + address, + coinValue.toPlainString()); + + } + return numMatches > 0; + } + void getTxFeeRate(ResultHandler resultHandler) { try { @SuppressWarnings({"unchecked", "Convert2MethodRef"}) diff --git a/core/src/main/java/bisq/core/api/model/PaymentAccountPayloadInfo.java b/core/src/main/java/bisq/core/api/model/PaymentAccountPayloadInfo.java index 9af1277ce3..8bce2e96db 100644 --- a/core/src/main/java/bisq/core/api/model/PaymentAccountPayloadInfo.java +++ b/core/src/main/java/bisq/core/api/model/PaymentAccountPayloadInfo.java @@ -18,10 +18,12 @@ package bisq.core.api.model; import bisq.core.payment.payload.CryptoCurrencyAccountPayload; +import bisq.core.payment.payload.InstantCryptoCurrencyPayload; import bisq.core.payment.payload.PaymentAccountPayload; import bisq.common.Payload; +import java.util.Optional; import java.util.function.Supplier; import lombok.Getter; @@ -45,12 +47,15 @@ public class PaymentAccountPayloadInfo implements Payload { } public static PaymentAccountPayloadInfo toPaymentAccountPayloadInfo(PaymentAccountPayload paymentAccountPayload) { - String address = paymentAccountPayload instanceof CryptoCurrencyAccountPayload - ? ((CryptoCurrencyAccountPayload) paymentAccountPayload).getAddress() - : ""; + Optional address = Optional.empty(); + if (paymentAccountPayload instanceof CryptoCurrencyAccountPayload) + address = Optional.of(((CryptoCurrencyAccountPayload) paymentAccountPayload).getAddress()); + else if (paymentAccountPayload instanceof InstantCryptoCurrencyPayload) + address = Optional.of(((InstantCryptoCurrencyPayload) paymentAccountPayload).getAddress()); + return new PaymentAccountPayloadInfo(paymentAccountPayload.getId(), paymentAccountPayload.getPaymentMethodId(), - address); + address.orElse("")); } // For transmitting TradeInfo messages when no contract & payloads are available. diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcErrorMessageHandler.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcErrorMessageHandler.java index 435284252b..4c139e1709 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcErrorMessageHandler.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcErrorMessageHandler.java @@ -19,6 +19,7 @@ package bisq.daemon.grpc; import bisq.common.handlers.ErrorMessageHandler; +import bisq.proto.grpc.AvailabilityResultWithDescription; import bisq.proto.grpc.TakeOfferReply; import protobuf.AvailabilityResult; @@ -77,7 +78,7 @@ public class GrpcErrorMessageHandler implements ErrorMessageHandler { this.isErrorHandled = true; log.error(errorMessage); - if (isTakeOfferError()) { + if (takeOfferWasCalled()) { handleTakeOfferError(errorMessage); } else { exceptionHandler.handleErrorMessage(log, @@ -88,14 +89,20 @@ public class GrpcErrorMessageHandler implements ErrorMessageHandler { } private void handleTakeOfferError(String errorMessage) { - // Send the AvailabilityResult to the client instead of throwing an exception. - // The client should look at the grpc reply object's AvailabilityResult - // field if reply.hasTrade = false, and use it give the user a human readable msg. + // If the errorMessage originated from a UI purposed TaskRunner, it should + // contain an AvailabilityResult enum name. If it does, derive the + // AvailabilityResult enum from the errorMessage, wrap it in a new + // AvailabilityResultWithDescription enum, then send the + // AvailabilityResultWithDescription to the client instead of throwing + // an exception. The client should use the grpc reply object's + // AvailabilityResultWithDescription field if reply.hasTrade = false, and the + // client can decide to throw an exception with the client friendly error + // description, or take some other action based on the AvailabilityResult enum. + // (Some offer availability problems are not fatal, and retries are appropriate.) try { - AvailabilityResult availabilityResultProto = getAvailabilityResult(errorMessage); + var failureReason = getAvailabilityResultWithDescription(errorMessage); var reply = TakeOfferReply.newBuilder() - .setAvailabilityResult(availabilityResultProto) - .setAvailabilityResultDescription(getAvailabilityResultDescription(availabilityResultProto)) + .setFailureReason(failureReason) .build(); @SuppressWarnings("unchecked") var takeOfferResponseObserver = (StreamObserver) responseObserver; @@ -109,6 +116,15 @@ public class GrpcErrorMessageHandler implements ErrorMessageHandler { } } + private AvailabilityResultWithDescription getAvailabilityResultWithDescription(String errorMessage) { + AvailabilityResult proto = getAvailabilityResult(errorMessage); + String description = getAvailabilityResultDescription(proto); + return AvailabilityResultWithDescription.newBuilder() + .setAvailabilityResult(proto) + .setDescription(description) + .build(); + } + private AvailabilityResult getAvailabilityResult(String errorMessage) { return stream(AvailabilityResult.values()) .filter((e) -> errorMessage.toUpperCase().contains(e.name())) @@ -121,7 +137,7 @@ public class GrpcErrorMessageHandler implements ErrorMessageHandler { return bisq.core.offer.AvailabilityResult.fromProto(proto).description(); } - private boolean isTakeOfferError() { + private boolean takeOfferWasCalled() { return fullMethodName.equals(getTakeOfferMethod().getFullMethodName()); } } diff --git a/proto/src/main/proto/grpc.proto b/proto/src/main/proto/grpc.proto index 836d2e731b..14a3208c35 100644 --- a/proto/src/main/proto/grpc.proto +++ b/proto/src/main/proto/grpc.proto @@ -161,6 +161,11 @@ message OfferInfo { uint64 makerFee = 23; } +message AvailabilityResultWithDescription { + AvailabilityResult availabilityResult = 1; + string description = 2; +} + /////////////////////////////////////////////////////////////////////////////////////////// // PaymentAccounts /////////////////////////////////////////////////////////////////////////////////////////// @@ -303,8 +308,7 @@ message TakeOfferRequest { message TakeOfferReply { TradeInfo trade = 1; - AvailabilityResult availabilityResult = 2; - string availabilityResultDescription = 3; + AvailabilityResultWithDescription failureReason = 2; } message ConfirmPaymentStartedRequest { @@ -430,6 +434,8 @@ service Wallets { } rpc SendBtc (SendBtcRequest) returns (SendBtcReply) { } + rpc VerifyBsqSentToAddress (VerifyBsqSentToAddressRequest) returns (VerifyBsqSentToAddressReply) { + } rpc GetTxFeeRate (GetTxFeeRateRequest) returns (GetTxFeeRateReply) { } rpc SetTxFeeRatePreference (SetTxFeeRatePreferenceRequest) returns (SetTxFeeRatePreferenceReply) { @@ -494,6 +500,15 @@ message SendBtcReply { TxInfo txInfo = 1; } +message VerifyBsqSentToAddressRequest { + string address = 1; + string amount = 2; +} + +message VerifyBsqSentToAddressReply { + bool isAmountReceived = 1; +} + message GetTxFeeRateRequest { } @@ -606,4 +621,3 @@ message GetVersionRequest { message GetVersionReply { string version = 1; } -