From dc6144d337ae57d7d8c445f04758585acd7c3136 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Fri, 4 Dec 2020 14:17:24 -0300 Subject: [PATCH 01/45] Refactor BtcWalletService to let api override fee rates BtcWalletService was changed to allow the api to override tx fee rates from the sendbsq and sendbtc methods. The api methods will still be able to use the network fee service and custom tx fee rate preference, and set / unset the custom tx fee rate preference, but the change will permit the addition of an optional txFeeRate parameter to the sendbsq and sendbtc methods (todo). A few other minor changes (style and removal of never thrown ex spec) were also made to this class. Two BtcWalletService methods were refactored. - The redundant (was always true) boolean isSendTx argument was removed from the completePreparedVoteRevealTx method signature. - The redundant (was always true) boolean useCustomTxFee was removed from the completePreparedBsqTx method signature. - The completePreparedSendBsqTx method was overloaded with a 2nd parameter (Coin txFeePerVbyte) to allow api to override fee service and custom tx fee rate when sending BSQ or BTC. - The completePreparedBsqTx method was overloaded with a 3rd parameter (Coin txFeePerVbyte) to allow api to override fee service and custom tx fee rate when sending BSQ or BTC. The following line was deleted from the completePreparedBsqTx method because txFeePerVbyte is now an argument: Coin txFeePerVbyte = useCustomTxFee ? getTxFeeForWithdrawalPerVbyte() : feeService.getTxFeePerVbyte(); This useCustomTxFee value was always true, and redudant here because getTxFeeForWithdrawalPerVbyte() returns feeService.getTxFeePerVbyte() or the custom fee rate preference. i.e., Coin txFeePerVbyte = useCustomTxFee ? getTxFeeForWithdrawalPerVbyte() : feeService.getTxFeePerVbyte(); is equivalent to Coin txFeePerVbyte = getTxFeeForWithdrawalPerVbyte(); LockupTxService, UnlockTxService, BsqSendView, and BsqTransferService were adjusted to this BtcWalletService refactoring. --- .../core/btc/wallet/BsqTransferService.java | 2 +- .../core/btc/wallet/BtcWalletService.java | 40 ++++++++++++------- .../bond/lockup/LockupTxService.java | 2 +- .../bond/unlock/UnlockTxService.java | 2 +- .../main/dao/wallet/send/BsqSendView.java | 4 +- 5 files changed, 30 insertions(+), 20 deletions(-) diff --git a/core/src/main/java/bisq/core/btc/wallet/BsqTransferService.java b/core/src/main/java/bisq/core/btc/wallet/BsqTransferService.java index b6cc83e8c77..6bb9c79f039 100644 --- a/core/src/main/java/bisq/core/btc/wallet/BsqTransferService.java +++ b/core/src/main/java/bisq/core/btc/wallet/BsqTransferService.java @@ -40,7 +40,7 @@ public BsqTransferModel getBsqTransferModel(LegacyAddress address, InsufficientMoneyException { Transaction preparedSendTx = bsqWalletService.getPreparedSendBsqTx(address.toString(), receiverAmount); - Transaction txWithBtcFee = btcWalletService.completePreparedSendBsqTx(preparedSendTx, true); + Transaction txWithBtcFee = btcWalletService.completePreparedSendBsqTx(preparedSendTx); Transaction signedTx = bsqWalletService.signTx(txWithBtcFee); return new BsqTransferModel(address, diff --git a/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java b/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java index 3d45c491878..59ea556d591 100644 --- a/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java +++ b/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java @@ -439,8 +439,7 @@ public Transaction completePreparedVoteRevealTx(Transaction preparedTx, byte[] o // Add fee input to prepared BSQ send tx /////////////////////////////////////////////////////////////////////////////////////////// - - public Transaction completePreparedSendBsqTx(Transaction preparedBsqTx, boolean isSendTx) throws + public Transaction completePreparedSendBsqTx(Transaction preparedBsqTx) throws TransactionVerificationException, WalletException, InsufficientMoneyException { // preparedBsqTx has following structure: // inputs [1-n] BSQ inputs @@ -454,13 +453,26 @@ public Transaction completePreparedSendBsqTx(Transaction preparedBsqTx, boolean // outputs [0-1] BSQ change output // outputs [0-1] BTC change output // mining fee: BTC mining fee - return completePreparedBsqTx(preparedBsqTx, isSendTx, null); + Coin txFeePerVbyte = getTxFeeForWithdrawalPerVbyte(); + return completePreparedBsqTx(preparedBsqTx, null, txFeePerVbyte); + } + + public Transaction completePreparedSendBsqTx(Transaction preparedBsqTx, Coin txFeePerVbyte) throws + TransactionVerificationException, WalletException, InsufficientMoneyException { + return completePreparedBsqTx(preparedBsqTx, null, txFeePerVbyte); } public Transaction completePreparedBsqTx(Transaction preparedBsqTx, - boolean useCustomTxFee, @Nullable byte[] opReturnData) throws TransactionVerificationException, WalletException, InsufficientMoneyException { + Coin txFeePerVbyte = getTxFeeForWithdrawalPerVbyte(); + return completePreparedBsqTx(preparedBsqTx, opReturnData, txFeePerVbyte); + } + + public Transaction completePreparedBsqTx(Transaction preparedBsqTx, + @Nullable byte[] opReturnData, + Coin txFeePerVbyte) throws + TransactionVerificationException, WalletException, InsufficientMoneyException { // preparedBsqTx has following structure: // inputs [1-n] BSQ inputs @@ -487,8 +499,6 @@ public Transaction completePreparedBsqTx(Transaction preparedBsqTx, int sigSizePerInput = 106; // typical size for a tx with 2 inputs int txVsizeWithUnsignedInputs = 203; - // If useCustomTxFee we allow overriding the estimated fee from preferences - Coin txFeePerVbyte = useCustomTxFee ? getTxFeeForWithdrawalPerVbyte() : feeService.getTxFeePerVbyte(); // In case there are no change outputs we force a change by adding min dust to the BTC input Coin forcedChangeValue = Coin.ZERO; @@ -532,8 +542,8 @@ public Transaction completePreparedBsqTx(Transaction preparedBsqTx, sendRequest.signInputs = false; sendRequest.fee = txFeePerVbyte.multiply(txVsizeWithUnsignedInputs + - sigSizePerInput * numLegacyInputs + - sigSizePerInput * numSegwitInputs / 4); + sigSizePerInput * numLegacyInputs + + sigSizePerInput * numSegwitInputs / 4); sendRequest.feePerKb = Coin.ZERO; sendRequest.ensureMinRequiredFee = false; @@ -558,8 +568,8 @@ public Transaction completePreparedBsqTx(Transaction preparedBsqTx, numSegwitInputs = numInputs.second; txVsizeWithUnsignedInputs = resultTx.getVsize(); final long estimatedFeeAsLong = txFeePerVbyte.multiply(txVsizeWithUnsignedInputs + - sigSizePerInput * numLegacyInputs + - sigSizePerInput * numSegwitInputs / 4).value; + sigSizePerInput * numLegacyInputs + + sigSizePerInput * numSegwitInputs / 4).value; // calculated fee must be inside of a tolerance range with tx fee isFeeOutsideTolerance = Math.abs(resultTx.getFee().value - estimatedFeeAsLong) > 1000; } @@ -933,7 +943,7 @@ public void doubleSpendTransaction(String txId, Runnable resultHandler, ErrorMes } if (sendResult != null) { log.info("Broadcasting double spending transaction. " + sendResult.tx); - Futures.addCallback(sendResult.broadcastComplete, new FutureCallback() { + Futures.addCallback(sendResult.broadcastComplete, new FutureCallback<>() { @Override public void onSuccess(Transaction result) { log.info("Double spending transaction published. " + result); @@ -1163,7 +1173,7 @@ private SendRequest getSendRequestForMultipleAddresses(Set fromAddresses Coin fee, @Nullable String changeAddress, @Nullable KeyParameter aesKey) throws - AddressFormatException, AddressEntryException, InsufficientMoneyException { + AddressFormatException, AddressEntryException { Transaction tx = new Transaction(params); final Coin netValue = amount.subtract(fee); checkArgument(Restrictions.isAboveDust(netValue), @@ -1196,12 +1206,12 @@ private SendRequest getSendRequestForMultipleAddresses(Set fromAddresses sendRequest.coinSelector = new BtcCoinSelector(walletsSetup.getAddressesFromAddressEntries(addressEntries), preferences.getIgnoreDustThreshold()); - Optional addressEntryOptional = Optional.empty(); - AddressEntry changeAddressAddressEntry = null; + Optional addressEntryOptional = Optional.empty(); + if (changeAddress != null) addressEntryOptional = findAddressEntry(changeAddress, AddressEntry.Context.AVAILABLE); - changeAddressAddressEntry = addressEntryOptional.orElseGet(() -> getFreshAddressEntry()); + AddressEntry changeAddressAddressEntry = addressEntryOptional.orElseGet(this::getFreshAddressEntry); checkNotNull(changeAddressAddressEntry, "change address must not be null"); sendRequest.changeAddress = changeAddressAddressEntry.getAddress(); return sendRequest; diff --git a/core/src/main/java/bisq/core/dao/governance/bond/lockup/LockupTxService.java b/core/src/main/java/bisq/core/dao/governance/bond/lockup/LockupTxService.java index 4a466811e0c..901793c74de 100644 --- a/core/src/main/java/bisq/core/dao/governance/bond/lockup/LockupTxService.java +++ b/core/src/main/java/bisq/core/dao/governance/bond/lockup/LockupTxService.java @@ -103,7 +103,7 @@ private Transaction getLockupTx(Coin lockupAmount, int lockTime, LockupReason lo throws InsufficientMoneyException, WalletException, TransactionVerificationException, IOException { byte[] opReturnData = BondConsensus.getLockupOpReturnData(lockTime, lockupReason, hash); Transaction preparedTx = bsqWalletService.getPreparedLockupTx(lockupAmount); - Transaction txWithBtcFee = btcWalletService.completePreparedBsqTx(preparedTx, true, opReturnData); + Transaction txWithBtcFee = btcWalletService.completePreparedBsqTx(preparedTx, opReturnData); Transaction transaction = bsqWalletService.signTx(txWithBtcFee); log.info("Lockup tx: " + transaction); return transaction; diff --git a/core/src/main/java/bisq/core/dao/governance/bond/unlock/UnlockTxService.java b/core/src/main/java/bisq/core/dao/governance/bond/unlock/UnlockTxService.java index 0defe5e4752..a4c6691cf67 100644 --- a/core/src/main/java/bisq/core/dao/governance/bond/unlock/UnlockTxService.java +++ b/core/src/main/java/bisq/core/dao/governance/bond/unlock/UnlockTxService.java @@ -103,7 +103,7 @@ private Transaction getUnlockTx(String lockupTxId) checkArgument(optionalLockupTxOutput.isPresent(), "lockupTxOutput must be present"); TxOutput lockupTxOutput = optionalLockupTxOutput.get(); Transaction preparedTx = bsqWalletService.getPreparedUnlockTx(lockupTxOutput); - Transaction txWithBtcFee = btcWalletService.completePreparedBsqTx(preparedTx, true, null); + Transaction txWithBtcFee = btcWalletService.completePreparedBsqTx(preparedTx, null); Transaction transaction = bsqWalletService.signTx(txWithBtcFee); log.info("Unlock tx: " + transaction); return transaction; diff --git a/desktop/src/main/java/bisq/desktop/main/dao/wallet/send/BsqSendView.java b/desktop/src/main/java/bisq/desktop/main/dao/wallet/send/BsqSendView.java index 917d1290aae..533064b62c1 100644 --- a/desktop/src/main/java/bisq/desktop/main/dao/wallet/send/BsqSendView.java +++ b/desktop/src/main/java/bisq/desktop/main/dao/wallet/send/BsqSendView.java @@ -247,7 +247,7 @@ private void addSendBsqGroup() { Coin receiverAmount = ParsingUtils.parseToCoin(amountInputTextField.getText(), bsqFormatter); try { Transaction preparedSendTx = bsqWalletService.getPreparedSendBsqTx(receiversAddressString, receiverAmount); - Transaction txWithBtcFee = btcWalletService.completePreparedSendBsqTx(preparedSendTx, true); + Transaction txWithBtcFee = btcWalletService.completePreparedSendBsqTx(preparedSendTx); Transaction signedTx = bsqWalletService.signTx(txWithBtcFee); Coin miningFee = signedTx.getFee(); int txVsize = signedTx.getVsize(); @@ -305,7 +305,7 @@ private void addSendBtcGroup() { Coin receiverAmount = bsqFormatter.parseToBTC(btcAmountInputTextField.getText()); try { Transaction preparedSendTx = bsqWalletService.getPreparedSendBtcTx(receiversAddressString, receiverAmount); - Transaction txWithBtcFee = btcWalletService.completePreparedSendBsqTx(preparedSendTx, true); + Transaction txWithBtcFee = btcWalletService.completePreparedSendBsqTx(preparedSendTx); Transaction signedTx = bsqWalletService.signTx(txWithBtcFee); Coin miningFee = signedTx.getFee(); From 159d4cc6f505a178b5ab304888a276a35098c1b1 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Fri, 4 Dec 2020 17:17:37 -0300 Subject: [PATCH 02/45] Add optional txFeeRate parameter to api sendbsq If present in the sendbsq command, the parameter will override the fee service and custom fee rate setting for the BSQ transaction. Also changed the sendbsq grpc return type to a lightweight TX proto wrapper. Besides some small refactoring in the CLI, all the changes are adjustments for this new sendbsq parameter and its new grpc return value. --- .../java/bisq/apitest/method/MethodTest.java | 27 +++++- .../apitest/method/wallet/BsqWalletTest.java | 2 +- cli/src/main/java/bisq/cli/CliMain.java | 57 ++++++++----- core/src/main/java/bisq/core/api/CoreApi.java | 7 +- .../bisq/core/api/CoreWalletsService.java | 18 +++- .../main/java/bisq/core/api/model/TxInfo.java | 84 +++++++++++++++++++ .../core/btc/wallet/BsqTransferService.java | 5 +- .../bisq/daemon/grpc/GrpcWalletsService.java | 41 +++++---- proto/src/main/proto/grpc.proto | 27 ++++-- 9 files changed, 212 insertions(+), 56 deletions(-) create mode 100644 core/src/main/java/bisq/core/api/model/TxInfo.java diff --git a/apitest/src/test/java/bisq/apitest/method/MethodTest.java b/apitest/src/test/java/bisq/apitest/method/MethodTest.java index 0a43258d9a5..f275e885d19 100644 --- a/apitest/src/test/java/bisq/apitest/method/MethodTest.java +++ b/apitest/src/test/java/bisq/apitest/method/MethodTest.java @@ -52,6 +52,7 @@ import bisq.proto.grpc.SetWalletPasswordRequest; import bisq.proto.grpc.TakeOfferRequest; import bisq.proto.grpc.TradeInfo; +import bisq.proto.grpc.TxInfo; import bisq.proto.grpc.UnlockWalletRequest; import bisq.proto.grpc.UnsetTxFeeRatePreferenceRequest; import bisq.proto.grpc.WithdrawFundsRequest; @@ -160,8 +161,14 @@ protected final GetUnusedBsqAddressRequest createGetUnusedBsqAddressRequest() { return GetUnusedBsqAddressRequest.newBuilder().build(); } - protected final SendBsqRequest createSendBsqRequest(String address, String amount) { - return SendBsqRequest.newBuilder().setAddress(address).setAmount(amount).build(); + protected final SendBsqRequest createSendBsqRequest(String address, + String amount, + String txFeeRate) { + return SendBsqRequest.newBuilder() + .setAddress(address) + .setAmount(amount) + .setTxFeeRate(txFeeRate) + .build(); } protected final GetFundingAddressesRequest createGetFundingAddressesRequest() { @@ -247,9 +254,21 @@ protected final String getUnusedBsqAddress(BisqAppConfig bisqAppConfig) { return grpcStubs(bisqAppConfig).walletsService.getUnusedBsqAddress(createGetUnusedBsqAddressRequest()).getAddress(); } - protected final void sendBsq(BisqAppConfig bisqAppConfig, String address, String amount) { + protected final TxInfo sendBsq(BisqAppConfig bisqAppConfig, + String address, + String amount) { + return sendBsq(bisqAppConfig, address, amount, ""); + } + + protected final TxInfo sendBsq(BisqAppConfig bisqAppConfig, + String address, + String amount, + String txFeeRate) { //noinspection ResultOfMethodCallIgnored - grpcStubs(bisqAppConfig).walletsService.sendBsq(createSendBsqRequest(address, amount)); + return grpcStubs(bisqAppConfig).walletsService.sendBsq(createSendBsqRequest(address, + amount, + txFeeRate)) + .getTxInfo(); } protected final String getUnusedBtcAddress(BisqAppConfig bisqAppConfig) { diff --git a/apitest/src/test/java/bisq/apitest/method/wallet/BsqWalletTest.java b/apitest/src/test/java/bisq/apitest/method/wallet/BsqWalletTest.java index f9202af778a..2884555e3c7 100644 --- a/apitest/src/test/java/bisq/apitest/method/wallet/BsqWalletTest.java +++ b/apitest/src/test/java/bisq/apitest/method/wallet/BsqWalletTest.java @@ -108,7 +108,7 @@ public void testInitialBsqBalances(final TestInfo testInfo) { @Order(3) public void testSendBsqAndCheckBalancesBeforeGeneratingBtcBlock(final TestInfo testInfo) { String bobsBsqAddress = getUnusedBsqAddress(bobdaemon); - sendBsq(alicedaemon, bobsBsqAddress, SEND_BSQ_AMOUNT); + sendBsq(alicedaemon, bobsBsqAddress, SEND_BSQ_AMOUNT, "100"); sleep(2000); BsqBalanceInfo alicesBsqBalances = getBsqBalances(alicedaemon); diff --git a/cli/src/main/java/bisq/cli/CliMain.java b/cli/src/main/java/bisq/cli/CliMain.java index 44590cfbd6a..dc9c1204584 100644 --- a/cli/src/main/java/bisq/cli/CliMain.java +++ b/cli/src/main/java/bisq/cli/CliMain.java @@ -43,6 +43,7 @@ import bisq.proto.grpc.SetTxFeeRatePreferenceRequest; import bisq.proto.grpc.SetWalletPasswordRequest; import bisq.proto.grpc.TakeOfferRequest; +import bisq.proto.grpc.TxInfo; import bisq.proto.grpc.UnlockWalletRequest; import bisq.proto.grpc.UnsetTxFeeRatePreferenceRequest; import bisq.proto.grpc.WithdrawFundsRequest; @@ -259,19 +260,20 @@ public static void run(String[] args) { throw new IllegalArgumentException("no bsq amount specified"); var amount = nonOptionArgs.get(2); + verifyStringIsValidDecimal(amount); - try { - Double.parseDouble(amount); - } catch (NumberFormatException e) { - throw new IllegalArgumentException(format("'%s' is not a number", amount)); - } + var txFeeRate = nonOptionArgs.size() == 4 ? nonOptionArgs.get(3) : ""; + if (!txFeeRate.isEmpty()) + verifyStringIsValidLong(txFeeRate); var request = SendBsqRequest.newBuilder() .setAddress(address) .setAmount(amount) + .setTxFeeRate(txFeeRate) .build(); - walletsService.sendBsq(request); - out.printf("%s BSQ sent to %s%n", amount, address); + var reply = walletsService.sendBsq(request); + TxInfo txInfo = reply.getTxInfo(); + out.printf("%s bsq sent to %s in tx %s%n", amount, address, txInfo.getId()); return; } case gettxfeerate: { @@ -284,13 +286,7 @@ public static void run(String[] args) { if (nonOptionArgs.size() < 2) throw new IllegalArgumentException("no tx fee rate specified"); - long txFeeRate; - try { - txFeeRate = Long.parseLong(nonOptionArgs.get(2)); - } catch (NumberFormatException e) { - throw new IllegalArgumentException(format("'%s' is not a number", nonOptionArgs.get(2))); - } - + var txFeeRate = toLong(nonOptionArgs.get(2)); var request = SetTxFeeRatePreferenceRequest.newBuilder() .setTxFeeRatePreference(txFeeRate) .build(); @@ -560,12 +556,7 @@ public static void run(String[] args) { if (nonOptionArgs.size() < 3) throw new IllegalArgumentException("no unlock timeout specified"); - long timeout; - try { - timeout = Long.parseLong(nonOptionArgs.get(2)); - } catch (NumberFormatException e) { - throw new IllegalArgumentException(format("'%s' is not a number", nonOptionArgs.get(2))); - } + var timeout = toLong(nonOptionArgs.get(2)); var request = UnlockWalletRequest.newBuilder() .setPassword(nonOptionArgs.get(1)) .setTimeout(timeout).build(); @@ -627,6 +618,30 @@ private static Method getMethodFromCmd(String methodName) { return Method.valueOf(methodName.toLowerCase()); } + private static void verifyStringIsValidDecimal(String param) { + try { + Double.parseDouble(param); + } catch (NumberFormatException ex) { + throw new IllegalArgumentException(format("'%s' is not a number", param)); + } + } + + private static void verifyStringIsValidLong(String param) { + try { + Long.parseLong(param); + } catch (NumberFormatException ex) { + throw new IllegalArgumentException(format("'%s' is not a number", param)); + } + } + + private static long toLong(String param) { + try { + return Long.parseLong(param); + } catch (NumberFormatException ex) { + throw new IllegalArgumentException(format("'%s' is not a number", param)); + } + } + private static File saveFileToDisk(String prefix, @SuppressWarnings("SameParameterValue") String suffix, String text) { @@ -663,7 +678,7 @@ private static void printHelp(OptionParser parser, PrintStream stream) { stream.format(rowFormat, "getaddressbalance", "address", "Get server wallet address balance"); stream.format(rowFormat, "getfundingaddresses", "", "Get BTC funding addresses"); stream.format(rowFormat, "getunusedbsqaddress", "", "Get unused BSQ address"); - stream.format(rowFormat, "sendbsq", "address, amount", "Send BSQ"); + stream.format(rowFormat, "sendbsq", "address, amount [,tx fee rate (sats/byte)]", "Send BSQ"); stream.format(rowFormat, "gettxfeerate", "", "Get current tx fee rate in sats/byte"); stream.format(rowFormat, "settxfeerate", "satoshis (per byte)", "Set custom tx fee rate in sats/byte"); stream.format(rowFormat, "unsettxfeerate", "", "Unset custom tx fee rate"); diff --git a/core/src/main/java/bisq/core/api/CoreApi.java b/core/src/main/java/bisq/core/api/CoreApi.java index 9b7b25bc8f2..3319db51649 100644 --- a/core/src/main/java/bisq/core/api/CoreApi.java +++ b/core/src/main/java/bisq/core/api/CoreApi.java @@ -244,8 +244,11 @@ public String getUnusedBsqAddress() { return walletsService.getUnusedBsqAddress(); } - public void sendBsq(String address, String amount, TxBroadcaster.Callback callback) { - walletsService.sendBsq(address, amount, callback); + public void sendBsq(String address, + String amount, + String txFeeRate, + TxBroadcaster.Callback callback) { + walletsService.sendBsq(address, amount, txFeeRate, callback); } public void getTxFeeRate(ResultHandler resultHandler) { diff --git a/core/src/main/java/bisq/core/api/CoreWalletsService.java b/core/src/main/java/bisq/core/api/CoreWalletsService.java index 346c97a78de..5d52d9f9022 100644 --- a/core/src/main/java/bisq/core/api/CoreWalletsService.java +++ b/core/src/main/java/bisq/core/api/CoreWalletsService.java @@ -189,13 +189,27 @@ String getUnusedBsqAddress() { void sendBsq(String address, String amount, + String txFeeRate, TxBroadcaster.Callback callback) { try { LegacyAddress legacyAddress = getValidBsqLegacyAddress(address); Coin receiverAmount = getValidBsqTransferAmount(amount); - BsqTransferModel model = bsqTransferService.getBsqTransferModel(legacyAddress, receiverAmount); + + // A non txFeeRate String value overrides the fee service and custom fee. + Coin txFeePerVbyte = txFeeRate.isEmpty() + ? btcWalletService.getTxFeeForWithdrawalPerVbyte() + : Coin.valueOf(Long.parseLong(txFeeRate)); + + BsqTransferModel model = bsqTransferService.getBsqTransferModel(legacyAddress, + receiverAmount, + txFeePerVbyte); + log.info("Sending {} BSQ to {} with tx fee rate {} sats/byte).", + amount, + address, + txFeePerVbyte.value); bsqTransferService.sendFunds(model, callback); - } catch (InsufficientMoneyException + } catch (NumberFormatException + | InsufficientMoneyException | BsqChangeBelowDustException | TransactionVerificationException | WalletException ex) { diff --git a/core/src/main/java/bisq/core/api/model/TxInfo.java b/core/src/main/java/bisq/core/api/model/TxInfo.java new file mode 100644 index 00000000000..f1b24f6b0fa --- /dev/null +++ b/core/src/main/java/bisq/core/api/model/TxInfo.java @@ -0,0 +1,84 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + +package bisq.core.api.model; + +import bisq.common.Payload; + +import org.bitcoinj.core.Transaction; + +import lombok.EqualsAndHashCode; +import lombok.Getter; + +@EqualsAndHashCode +@Getter +public class TxInfo implements Payload { + + private final String id; + private final long outputSum; + private final long fee; + private final int size; + + public TxInfo(String id, long outputSum, long fee, int size) { + this.id = id; + this.outputSum = outputSum; + this.fee = fee; + this.size = size; + } + + public static TxInfo toTxInfo(Transaction transaction) { + if (transaction == null) + throw new IllegalStateException("server created a null transaction"); + + return new TxInfo(transaction.getTxId().toString(), + transaction.getOutputSum().value, + transaction.getFee().value, + transaction.getMessageSize()); + } + + ////////////////////////////////////////////////////////////////////////////////////// + // PROTO BUFFER + ////////////////////////////////////////////////////////////////////////////////////// + + @Override + public bisq.proto.grpc.TxInfo toProtoMessage() { + return bisq.proto.grpc.TxInfo.newBuilder() + .setId(id) + .setOutputSum(outputSum) + .setFee(fee) + .setSize(size) + .build(); + } + + @SuppressWarnings("unused") + public static TxInfo fromProto(bisq.proto.grpc.TxInfo proto) { + return new TxInfo(proto.getId(), + proto.getOutputSum(), + proto.getFee(), + proto.getSize()); + } + + @Override + public String toString() { + return "TxInfo{" + "\n" + + " id='" + id + '\'' + "\n" + + ", outputSum=" + outputSum + " sats" + "\n" + + ", fee=" + fee + " sats" + "\n" + + ", size=" + size + " bytes" + "\n" + + '}'; + } +} diff --git a/core/src/main/java/bisq/core/btc/wallet/BsqTransferService.java b/core/src/main/java/bisq/core/btc/wallet/BsqTransferService.java index 6bb9c79f039..4558b0acf74 100644 --- a/core/src/main/java/bisq/core/btc/wallet/BsqTransferService.java +++ b/core/src/main/java/bisq/core/btc/wallet/BsqTransferService.java @@ -33,14 +33,15 @@ public BsqTransferService(WalletsManager walletsManager, } public BsqTransferModel getBsqTransferModel(LegacyAddress address, - Coin receiverAmount) + Coin receiverAmount, + Coin txFeePerVbyte) throws TransactionVerificationException, WalletException, BsqChangeBelowDustException, InsufficientMoneyException { Transaction preparedSendTx = bsqWalletService.getPreparedSendBsqTx(address.toString(), receiverAmount); - Transaction txWithBtcFee = btcWalletService.completePreparedSendBsqTx(preparedSendTx); + Transaction txWithBtcFee = btcWalletService.completePreparedSendBsqTx(preparedSendTx, txFeePerVbyte); Transaction signedTx = bsqWalletService.signTx(txWithBtcFee); return new BsqTransferModel(address, diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java index b138a6c6931..c38d5209d1b 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java @@ -62,6 +62,8 @@ import lombok.extern.slf4j.Slf4j; +import static bisq.core.api.model.TxInfo.toTxInfo; + @Slf4j class GrpcWalletsService extends WalletsGrpc.WalletsImplBase { @@ -145,24 +147,29 @@ public void getUnusedBsqAddress(GetUnusedBsqAddressRequest req, public void sendBsq(SendBsqRequest req, StreamObserver responseObserver) { try { - coreApi.sendBsq(req.getAddress(), req.getAmount(), new TxBroadcaster.Callback() { - @Override - public void onSuccess(Transaction tx) { - log.info("Successfully published BSQ tx: id {}, output sum {} sats, fee {} sats, size {} bytes", - tx.getTxId().toString(), - tx.getOutputSum(), - tx.getFee(), - tx.getMessageSize()); - var reply = SendBsqReply.newBuilder().build(); - responseObserver.onNext(reply); - responseObserver.onCompleted(); - } + coreApi.sendBsq(req.getAddress(), + req.getAmount(), + req.getTxFeeRate(), + new TxBroadcaster.Callback() { + @Override + public void onSuccess(Transaction tx) { + log.info("Successfully published BSQ tx: id {}, output sum {} sats, fee {} sats, size {} bytes", + tx.getTxId().toString(), + tx.getOutputSum(), + tx.getFee(), + tx.getMessageSize()); + var reply = SendBsqReply.newBuilder() + .setTxInfo(toTxInfo(tx).toProtoMessage()) + .build(); + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } - @Override - public void onFailure(TxBroadcastException ex) { - throw new IllegalStateException(ex); - } - }); + @Override + public void onFailure(TxBroadcastException ex) { + throw new IllegalStateException(ex); + } + }); } catch (IllegalStateException cause) { var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); responseObserver.onError(ex); diff --git a/proto/src/main/proto/grpc.proto b/proto/src/main/proto/grpc.proto index 517cfc4ea46..b4950b96a25 100644 --- a/proto/src/main/proto/grpc.proto +++ b/proto/src/main/proto/grpc.proto @@ -287,6 +287,24 @@ message TradeInfo { string contractAsJson = 24; } +/////////////////////////////////////////////////////////////////////////////////////////// +// Transactions +/////////////////////////////////////////////////////////////////////////////////////////// + +message TxFeeRateInfo { + bool useCustomTxFeeRate = 1; + uint64 customTxFeeRate = 2; + uint64 feeServiceRate = 3; + uint64 lastFeeServiceRequestTs = 4; +} + +message TxInfo { + string id = 1; + uint64 outputSum = 2; + uint64 fee = 3; + int32 size = 4; +} + /////////////////////////////////////////////////////////////////////////////////////////// // Wallets /////////////////////////////////////////////////////////////////////////////////////////// @@ -344,9 +362,11 @@ message GetUnusedBsqAddressReply { message SendBsqRequest { string address = 1; string amount = 2; + string txFeeRate = 3; } message SendBsqReply { + TxInfo txInfo = 1; } message GetTxFeeRateRequest { @@ -437,13 +457,6 @@ message AddressBalanceInfo { int64 numConfirmations = 3; } -message TxFeeRateInfo { - bool useCustomTxFeeRate = 1; - uint64 customTxFeeRate = 2; - uint64 feeServiceRate = 3; - uint64 lastFeeServiceRequestTs = 4; -} - /////////////////////////////////////////////////////////////////////////////////////////// // Version /////////////////////////////////////////////////////////////////////////////////////////// From 6c9f0c252d061bd713e4bd6c399e8473c5649f91 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Tue, 8 Dec 2020 21:12:02 -0300 Subject: [PATCH 03/45] Add new api method 'sendbtc' and test Takes an address, amount, and optional txfeerate param, returns a lightweight TxInfo proto. Also overloaded two BtcWalletService methods to allow sendbtc to pass in the tx fee rate -- overriding the fee service and custom fee rate setting. --- .../java/bisq/apitest/method/MethodTest.java | 26 +++++ .../apitest/method/wallet/BtcWalletTest.java | 33 +++++++ .../bisq/apitest/scenario/WalletTest.java | 1 + cli/src/main/java/bisq/cli/CliMain.java | 29 ++++++ core/src/main/java/bisq/core/api/CoreApi.java | 10 ++ .../bisq/core/api/CoreWalletsService.java | 97 ++++++++++++++++--- .../core/btc/wallet/BtcWalletService.java | 15 ++- .../bisq/daemon/grpc/GrpcWalletsService.java | 45 +++++++++ proto/src/main/proto/grpc.proto | 12 +++ 9 files changed, 253 insertions(+), 15 deletions(-) diff --git a/apitest/src/test/java/bisq/apitest/method/MethodTest.java b/apitest/src/test/java/bisq/apitest/method/MethodTest.java index f275e885d19..f64c2a5fe92 100644 --- a/apitest/src/test/java/bisq/apitest/method/MethodTest.java +++ b/apitest/src/test/java/bisq/apitest/method/MethodTest.java @@ -48,6 +48,7 @@ import bisq.proto.grpc.RegisterDisputeAgentRequest; import bisq.proto.grpc.RemoveWalletPasswordRequest; import bisq.proto.grpc.SendBsqRequest; +import bisq.proto.grpc.SendBtcRequest; import bisq.proto.grpc.SetTxFeeRatePreferenceRequest; import bisq.proto.grpc.SetWalletPasswordRequest; import bisq.proto.grpc.TakeOfferRequest; @@ -171,6 +172,16 @@ protected final SendBsqRequest createSendBsqRequest(String address, .build(); } + protected final SendBtcRequest createSendBtcRequest(String address, + String amount, + String txFeeRate) { + return SendBtcRequest.newBuilder() + .setAddress(address) + .setAmount(amount) + .setTxFeeRate(txFeeRate) + .build(); + } + protected final GetFundingAddressesRequest createGetFundingAddressesRequest() { return GetFundingAddressesRequest.newBuilder().build(); } @@ -271,6 +282,21 @@ protected final TxInfo sendBsq(BisqAppConfig bisqAppConfig, .getTxInfo(); } + protected final TxInfo sendBtc(BisqAppConfig bisqAppConfig, String address, String amount) { + return sendBtc(bisqAppConfig, address, amount, ""); + } + + protected final TxInfo sendBtc(BisqAppConfig bisqAppConfig, + String address, + String amount, + String txFeeRate) { + //noinspection ResultOfMethodCallIgnored + return grpcStubs(bisqAppConfig).walletsService.sendBtc(createSendBtcRequest(address, + amount, + txFeeRate)) + .getTxInfo(); + } + protected final String getUnusedBtcAddress(BisqAppConfig bisqAppConfig) { //noinspection OptionalGetWithoutIsPresent return grpcStubs(bisqAppConfig).walletsService.getFundingAddresses(createGetFundingAddressesRequest()) diff --git a/apitest/src/test/java/bisq/apitest/method/wallet/BtcWalletTest.java b/apitest/src/test/java/bisq/apitest/method/wallet/BtcWalletTest.java index daee479b89a..a97e9a690da 100644 --- a/apitest/src/test/java/bisq/apitest/method/wallet/BtcWalletTest.java +++ b/apitest/src/test/java/bisq/apitest/method/wallet/BtcWalletTest.java @@ -20,6 +20,7 @@ import static bisq.cli.TableFormat.formatBtcBalanceInfoTbl; import static java.util.Collections.singletonList; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.MethodOrderer.OrderAnnotation; @@ -92,6 +93,38 @@ public void testFundAlicesBtcWallet(final TestInfo testInfo) { formatBtcBalanceInfoTbl(btcBalanceInfo)); } + @Test + @Order(3) + public void testAliceSendBTCToBob(TestInfo testInfo) { + String bobsBtcAddress = getUnusedBtcAddress(bobdaemon); + log.debug("Sending 5.5 BTC From Alice to Bob @ {}", bobsBtcAddress); + + sendBtc(alicedaemon, bobsBtcAddress, "5.50", "100"); + genBtcBlocksThenWait(1, 3000); + + BtcBalanceInfo alicesBalances = getBtcBalances(alicedaemon); + + log.debug("{} Alice's BTC Balances:\n{}", + testName(testInfo), + formatBtcBalanceInfoTbl(alicesBalances)); + bisq.core.api.model.BtcBalanceInfo alicesExpectedBalances = + bisq.core.api.model.BtcBalanceInfo.valueOf(700000000, + 0, + 700000000, + 0); + verifyBtcBalances(alicesExpectedBalances, alicesBalances); + + BtcBalanceInfo bobsBalances = getBtcBalances(bobdaemon); + log.debug("{} Bob's BTC Balances:\n{}", + testName(testInfo), + formatBtcBalanceInfoTbl(bobsBalances)); + // We cannot (?) predict the exact tx size and calculate how much in tx fees were + // deducted from the 5.5 BTC sent to Bob, but we do know Bob should have something + // between 15.49978000 and 15.49978100 BTC. + assertTrue(bobsBalances.getAvailableBalance() >= 1549978000); + assertTrue(bobsBalances.getAvailableBalance() <= 1549978100); + } + @AfterAll public static void tearDown() { tearDownScaffold(); diff --git a/apitest/src/test/java/bisq/apitest/scenario/WalletTest.java b/apitest/src/test/java/bisq/apitest/scenario/WalletTest.java index 6fadf3cc251..0fff4bf694d 100644 --- a/apitest/src/test/java/bisq/apitest/scenario/WalletTest.java +++ b/apitest/src/test/java/bisq/apitest/scenario/WalletTest.java @@ -67,6 +67,7 @@ public void testBtcWalletFunding(final TestInfo testInfo) { btcWalletTest.testInitialBtcBalances(testInfo); btcWalletTest.testFundAlicesBtcWallet(testInfo); + btcWalletTest.testAliceSendBTCToBob(testInfo); } @Test diff --git a/cli/src/main/java/bisq/cli/CliMain.java b/cli/src/main/java/bisq/cli/CliMain.java index dc9c1204584..5a98f9a7eb8 100644 --- a/cli/src/main/java/bisq/cli/CliMain.java +++ b/cli/src/main/java/bisq/cli/CliMain.java @@ -40,6 +40,7 @@ import bisq.proto.grpc.RegisterDisputeAgentRequest; import bisq.proto.grpc.RemoveWalletPasswordRequest; import bisq.proto.grpc.SendBsqRequest; +import bisq.proto.grpc.SendBtcRequest; import bisq.proto.grpc.SetTxFeeRatePreferenceRequest; import bisq.proto.grpc.SetWalletPasswordRequest; import bisq.proto.grpc.TakeOfferRequest; @@ -111,6 +112,7 @@ private enum Method { getfundingaddresses, getunusedbsqaddress, sendbsq, + sendbtc, gettxfeerate, settxfeerate, unsettxfeerate, @@ -276,6 +278,32 @@ public static void run(String[] args) { out.printf("%s bsq sent to %s in tx %s%n", amount, address, txInfo.getId()); return; } + case sendbtc: { + if (nonOptionArgs.size() < 2) + throw new IllegalArgumentException("no btc address specified"); + + var address = nonOptionArgs.get(1); + + if (nonOptionArgs.size() < 3) + throw new IllegalArgumentException("no btc amount specified"); + + var amount = nonOptionArgs.get(2); + verifyStringIsValidDecimal(amount); + + var txFeeRate = nonOptionArgs.size() == 4 ? nonOptionArgs.get(3) : ""; + if (!txFeeRate.isEmpty()) + verifyStringIsValidLong(txFeeRate); + + var request = SendBtcRequest.newBuilder() + .setAddress(address) + .setAmount(amount) + .setTxFeeRate(txFeeRate) + .build(); + var reply = walletsService.sendBtc(request); + TxInfo txInfo = reply.getTxInfo(); + out.printf("%s btc sent to %s in tx %s%n", amount, address, txInfo.getId()); + return; + } case gettxfeerate: { var request = GetTxFeeRateRequest.newBuilder().build(); var reply = walletsService.getTxFeeRate(request); @@ -679,6 +707,7 @@ private static void printHelp(OptionParser parser, PrintStream stream) { stream.format(rowFormat, "getfundingaddresses", "", "Get BTC funding addresses"); stream.format(rowFormat, "getunusedbsqaddress", "", "Get unused BSQ address"); stream.format(rowFormat, "sendbsq", "address, amount [,tx fee rate (sats/byte)]", "Send BSQ"); + stream.format(rowFormat, "sendbtc", "address, amount [,tx fee rate (sats/byte)]", "Send BTC"); stream.format(rowFormat, "gettxfeerate", "", "Get current tx fee rate in sats/byte"); stream.format(rowFormat, "settxfeerate", "satoshis (per byte)", "Set custom tx fee rate in sats/byte"); stream.format(rowFormat, "unsettxfeerate", "", "Unset custom tx fee rate"); diff --git a/core/src/main/java/bisq/core/api/CoreApi.java b/core/src/main/java/bisq/core/api/CoreApi.java index af8a5a4bcee..522fe8fb243 100644 --- a/core/src/main/java/bisq/core/api/CoreApi.java +++ b/core/src/main/java/bisq/core/api/CoreApi.java @@ -34,10 +34,13 @@ import bisq.common.handlers.ResultHandler; import org.bitcoinj.core.Coin; +import org.bitcoinj.core.Transaction; import javax.inject.Inject; import javax.inject.Singleton; +import com.google.common.util.concurrent.FutureCallback; + import java.util.ArrayList; import java.util.List; import java.util.Set; @@ -253,6 +256,13 @@ public void sendBsq(String address, walletsService.sendBsq(address, amount, txFeeRate, callback); } + public void sendBtc(String address, + String amount, + String txFeeRate, + FutureCallback callback) { + walletsService.sendBtc(address, amount, txFeeRate, callback); + } + public void getTxFeeRate(ResultHandler resultHandler) { walletsService.getTxFeeRate(resultHandler); } diff --git a/core/src/main/java/bisq/core/api/CoreWalletsService.java b/core/src/main/java/bisq/core/api/CoreWalletsService.java index 5d52d9f9022..2862f6c7e9b 100644 --- a/core/src/main/java/bisq/core/api/CoreWalletsService.java +++ b/core/src/main/java/bisq/core/api/CoreWalletsService.java @@ -23,7 +23,9 @@ import bisq.core.api.model.BtcBalanceInfo; import bisq.core.api.model.TxFeeRateInfo; import bisq.core.btc.Balances; +import bisq.core.btc.exceptions.AddressEntryException; import bisq.core.btc.exceptions.BsqChangeBelowDustException; +import bisq.core.btc.exceptions.InsufficientFundsException; import bisq.core.btc.exceptions.TransactionVerificationException; import bisq.core.btc.exceptions.WalletException; import bisq.core.btc.model.AddressEntry; @@ -35,7 +37,9 @@ import bisq.core.btc.wallet.WalletsManager; import bisq.core.provider.fee.FeeService; import bisq.core.user.Preferences; +import bisq.core.util.FormattingUtils; import bisq.core.util.coin.BsqFormatter; +import bisq.core.util.coin.CoinFormatter; import bisq.common.Timer; import bisq.common.UserThread; @@ -46,10 +50,12 @@ import org.bitcoinj.core.Coin; import org.bitcoinj.core.InsufficientMoneyException; import org.bitcoinj.core.LegacyAddress; +import org.bitcoinj.core.Transaction; import org.bitcoinj.core.TransactionConfidence; import org.bitcoinj.crypto.KeyCrypterScrypt; import javax.inject.Inject; +import javax.inject.Named; import com.google.common.cache.CacheBuilder; import com.google.common.cache.CacheLoader; @@ -64,6 +70,7 @@ import java.util.List; import java.util.Optional; +import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; @@ -85,6 +92,7 @@ class CoreWalletsService { private final BsqTransferService bsqTransferService; private final BsqFormatter bsqFormatter; private final BtcWalletService btcWalletService; + private final CoinFormatter btcFormatter; private final FeeService feeService; private final Preferences preferences; @@ -103,6 +111,7 @@ public CoreWalletsService(Balances balances, BsqTransferService bsqTransferService, BsqFormatter bsqFormatter, BtcWalletService btcWalletService, + @Named(FormattingUtils.BTC_FORMATTER_KEY) CoinFormatter btcFormatter, FeeService feeService, Preferences preferences) { this.balances = balances; @@ -111,6 +120,7 @@ public CoreWalletsService(Balances balances, this.bsqTransferService = bsqTransferService; this.bsqFormatter = bsqFormatter; this.btcWalletService = btcWalletService; + this.btcFormatter = btcFormatter; this.feeService = feeService; this.preferences = preferences; } @@ -191,25 +201,25 @@ void sendBsq(String address, String amount, String txFeeRate, TxBroadcaster.Callback callback) { + verifyWalletsAreAvailable(); + verifyEncryptedWalletIsUnlocked(); + try { LegacyAddress legacyAddress = getValidBsqLegacyAddress(address); - Coin receiverAmount = getValidBsqTransferAmount(amount); - - // A non txFeeRate String value overrides the fee service and custom fee. - Coin txFeePerVbyte = txFeeRate.isEmpty() - ? btcWalletService.getTxFeeForWithdrawalPerVbyte() - : Coin.valueOf(Long.parseLong(txFeeRate)); - + Coin receiverAmount = getValidTransferAmount(amount, bsqFormatter); + Coin txFeePerVbyte = getTxFeeRateFromParamOrPreferenceOrFeeService(txFeeRate); BsqTransferModel model = bsqTransferService.getBsqTransferModel(legacyAddress, receiverAmount, txFeePerVbyte); - log.info("Sending {} BSQ to {} with tx fee rate {} sats/byte).", + log.info("Sending {} BSQ to {} with tx fee rate {} sats/byte.", amount, address, txFeePerVbyte.value); bsqTransferService.sendFunds(model, callback); + } catch (InsufficientMoneyException ex) { + log.error("", ex); + throw new IllegalStateException("cannot send bsq due to insufficient funds", ex); } catch (NumberFormatException - | InsufficientMoneyException | BsqChangeBelowDustException | TransactionVerificationException | WalletException ex) { @@ -218,6 +228,60 @@ void sendBsq(String address, } } + void sendBtc(String address, + String amount, + String txFeeRate, + FutureCallback callback) { + verifyWalletsAreAvailable(); + verifyEncryptedWalletIsUnlocked(); + + try { + Set fromAddresses = btcWalletService.getAddressEntriesForAvailableBalanceStream() + .map(AddressEntry::getAddressString) + .collect(Collectors.toSet()); + Coin receiverAmount = getValidTransferAmount(amount, btcFormatter); + Coin txFeePerVbyte = getTxFeeRateFromParamOrPreferenceOrFeeService(txFeeRate); + + // TODO Support feeExcluded (or included), default is fee included. + // See WithdrawalView # onWithdraw (and refactor). + Transaction feeEstimationTransaction = + btcWalletService.getFeeEstimationTransactionForMultipleAddresses(fromAddresses, + receiverAmount, + txFeePerVbyte); + if (feeEstimationTransaction == null) + throw new IllegalStateException("could not estimate the transaction fee"); + + Coin dust = btcWalletService.getDust(feeEstimationTransaction); + Coin fee = feeEstimationTransaction.getFee().add(dust); + if (dust.isPositive()) { + fee = feeEstimationTransaction.getFee().add(dust); + log.info("Dust txo ({} sats) was detected, the dust amount has been added to the fee (was {}, now {})", + dust.value, + feeEstimationTransaction.getFee(), + fee.value); + } + log.info("Sending {} BTC to {} with tx fee of {} sats (fee rate {} sats/byte).", + amount, + address, + fee.value, + txFeePerVbyte.value); + btcWalletService.sendFundsForMultipleAddresses(fromAddresses, + address, + receiverAmount, + fee, + null, + tempAesKey, + null, /* memo todo */ + callback); + } catch (AddressEntryException ex) { + log.error("", ex); + throw new IllegalStateException("cannot send btc from any addresses in wallet", ex); + } catch (InsufficientFundsException | InsufficientMoneyException ex) { + log.error("", ex); + throw new IllegalStateException("cannot send btc due to insufficient funds", ex); + } + } + void getTxFeeRate(ResultHandler resultHandler) { try { @SuppressWarnings({"unchecked", "Convert2MethodRef"}) @@ -437,15 +501,22 @@ private LegacyAddress getValidBsqLegacyAddress(String address) { } } - // Returns a Coin for the amount string, or a RuntimeException if invalid. - private Coin getValidBsqTransferAmount(String amount) { - Coin amountAsCoin = parseToCoin(amount, bsqFormatter); + // Returns a Coin for the transfer amount string, or a RuntimeException if invalid. + private Coin getValidTransferAmount(String amount, CoinFormatter coinFormatter) { + Coin amountAsCoin = parseToCoin(amount, coinFormatter); if (amountAsCoin.isLessThan(getMinNonDustOutput())) - throw new IllegalStateException(format("%s bsq is an invalid send amount", amount)); + throw new IllegalStateException(format("%s is an invalid transfer amount", amount)); return amountAsCoin; } + private Coin getTxFeeRateFromParamOrPreferenceOrFeeService(String txFeeRate) { + // A non txFeeRate String value overrides the fee service and custom fee. + return txFeeRate.isEmpty() + ? btcWalletService.getTxFeeForWithdrawalPerVbyte() + : Coin.valueOf(Long.parseLong(txFeeRate)); + } + private KeyCrypterScrypt getKeyCrypterScrypt() { KeyCrypterScrypt keyCrypterScrypt = walletsManager.getKeyCrypterScrypt(); if (keyCrypterScrypt == null) diff --git a/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java b/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java index f6b7ac05f4b..2d534caf4e3 100644 --- a/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java +++ b/core/src/main/java/bisq/core/btc/wallet/BtcWalletService.java @@ -1023,6 +1023,14 @@ public Transaction getFeeEstimationTransaction(String fromAddress, public Transaction getFeeEstimationTransactionForMultipleAddresses(Set fromAddresses, Coin amount) throws AddressFormatException, AddressEntryException, InsufficientFundsException { + Coin txFeeForWithdrawalPerVbyte = getTxFeeForWithdrawalPerVbyte(); + return getFeeEstimationTransactionForMultipleAddresses(fromAddresses, amount, txFeeForWithdrawalPerVbyte); + } + + public Transaction getFeeEstimationTransactionForMultipleAddresses(Set fromAddresses, + Coin amount, + Coin txFeeForWithdrawalPerVbyte) + throws AddressFormatException, AddressEntryException, InsufficientFundsException { Set addressEntries = fromAddresses.stream() .map(address -> { Optional addressEntryOptional = findAddressEntry(address, AddressEntry.Context.AVAILABLE); @@ -1045,7 +1053,6 @@ public Transaction getFeeEstimationTransactionForMultipleAddresses(Set f int counter = 0; int txVsize = 0; Transaction tx; - Coin txFeeForWithdrawalPerVbyte = getTxFeeForWithdrawalPerVbyte(); do { counter++; fee = txFeeForWithdrawalPerVbyte.multiply(txVsize); @@ -1072,7 +1079,11 @@ public Transaction getFeeEstimationTransactionForMultipleAddresses(Set f } private boolean feeEstimationNotSatisfied(int counter, Transaction tx) { - long targetFee = getTxFeeForWithdrawalPerVbyte().multiply(tx.getVsize()).value; + return feeEstimationNotSatisfied(counter, tx, getTxFeeForWithdrawalPerVbyte()); + } + + private boolean feeEstimationNotSatisfied(int counter, Transaction tx, Coin txFeeForWithdrawalPerVbyte) { + long targetFee = txFeeForWithdrawalPerVbyte.multiply(tx.getVsize()).value; return counter < 10 && (tx.getFee().value < targetFee || tx.getFee().value - targetFee > 1000); diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java index c38d5209d1b..b902e805edd 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java @@ -39,6 +39,8 @@ import bisq.proto.grpc.RemoveWalletPasswordRequest; import bisq.proto.grpc.SendBsqReply; import bisq.proto.grpc.SendBsqRequest; +import bisq.proto.grpc.SendBtcReply; +import bisq.proto.grpc.SendBtcRequest; import bisq.proto.grpc.SetTxFeeRatePreferenceReply; import bisq.proto.grpc.SetTxFeeRatePreferenceRequest; import bisq.proto.grpc.SetWalletPasswordReply; @@ -57,11 +59,15 @@ import javax.inject.Inject; +import com.google.common.util.concurrent.FutureCallback; + import java.util.List; import java.util.stream.Collectors; import lombok.extern.slf4j.Slf4j; +import org.jetbrains.annotations.NotNull; + import static bisq.core.api.model.TxInfo.toTxInfo; @Slf4j @@ -177,6 +183,45 @@ public void onFailure(TxBroadcastException ex) { } } + @Override + public void sendBtc(SendBtcRequest req, + StreamObserver responseObserver) { + try { + coreApi.sendBtc(req.getAddress(), + req.getAmount(), + req.getTxFeeRate(), + new FutureCallback<>() { + @Override + public void onSuccess(Transaction tx) { + if (tx != null) { + log.info("Successfully published BTC tx: id {}, output sum {} sats, fee {} sats, size {} bytes", + tx.getTxId().toString(), + tx.getOutputSum(), + tx.getFee(), + tx.getMessageSize()); + var reply = SendBtcReply.newBuilder() + .setTxInfo(toTxInfo(tx).toProtoMessage()) + .build(); + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } else { + throw new IllegalStateException("btc transaction is null"); + } + } + + @Override + public void onFailure(@NotNull Throwable t) { + log.error("", t); + throw new IllegalStateException(t); + } + }); + } catch (IllegalStateException | IllegalArgumentException cause) { + var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); + responseObserver.onError(ex); + throw ex; + } + } + @Override public void getTxFeeRate(GetTxFeeRateRequest req, StreamObserver responseObserver) { diff --git a/proto/src/main/proto/grpc.proto b/proto/src/main/proto/grpc.proto index b4950b96a25..7bfaf219f73 100644 --- a/proto/src/main/proto/grpc.proto +++ b/proto/src/main/proto/grpc.proto @@ -318,6 +318,8 @@ service Wallets { } rpc SendBsq (SendBsqRequest) returns (SendBsqReply) { } + rpc SendBtc (SendBtcRequest) returns (SendBtcReply) { + } rpc GetTxFeeRate (GetTxFeeRateRequest) returns (GetTxFeeRateReply) { } rpc SetTxFeeRatePreference (SetTxFeeRatePreferenceRequest) returns (SetTxFeeRatePreferenceReply) { @@ -369,6 +371,16 @@ message SendBsqReply { TxInfo txInfo = 1; } +message SendBtcRequest { + string address = 1; + string amount = 2; + string txFeeRate = 3; +} + +message SendBtcReply { + TxInfo txInfo = 1; +} + message GetTxFeeRateRequest { } From bd66008062027b3677c276f5bbf99a8d1b2bc9d2 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Wed, 9 Dec 2020 16:51:56 -0300 Subject: [PATCH 04/45] Support tx memo field for btc withdrawals from api - Added optional memo parameter to the api's sendbtc and withdrawfunds commands. - Removed the @Nullable annotation was removed because protobuf does not support null. - Visibility in two wallet check methods were changed from private to pkg protected so the CoreTradeService could use them. - Adjusted affected tests. (Asserting the memo field was set on a transaction cannot be checked from apitest yet.) --- .../java/bisq/apitest/method/MethodTest.java | 26 ++++++++++++------- .../method/trade/AbstractTradeTest.java | 10 ++++--- .../method/trade/TakeSellBTCOfferTest.java | 4 +-- .../apitest/method/wallet/BtcWalletTest.java | 6 ++++- cli/src/main/java/bisq/cli/CliMain.java | 20 ++++++++++---- core/src/main/java/bisq/core/api/CoreApi.java | 7 +++-- .../java/bisq/core/api/CoreTradesService.java | 25 ++++++++++++------ .../bisq/core/api/CoreWalletsService.java | 9 ++++--- .../bisq/daemon/grpc/GrpcTradesService.java | 6 +++-- .../bisq/daemon/grpc/GrpcWalletsService.java | 1 + proto/src/main/proto/grpc.proto | 2 ++ 11 files changed, 77 insertions(+), 39 deletions(-) diff --git a/apitest/src/test/java/bisq/apitest/method/MethodTest.java b/apitest/src/test/java/bisq/apitest/method/MethodTest.java index f64c2a5fe92..95918189424 100644 --- a/apitest/src/test/java/bisq/apitest/method/MethodTest.java +++ b/apitest/src/test/java/bisq/apitest/method/MethodTest.java @@ -174,11 +174,13 @@ protected final SendBsqRequest createSendBsqRequest(String address, protected final SendBtcRequest createSendBtcRequest(String address, String amount, - String txFeeRate) { + String txFeeRate, + String memo) { return SendBtcRequest.newBuilder() .setAddress(address) .setAmount(amount) .setTxFeeRate(txFeeRate) + .setMemo(memo) .build(); } @@ -226,10 +228,13 @@ protected final KeepFundsRequest createKeepFundsRequest(String tradeId) { .build(); } - protected final WithdrawFundsRequest createWithdrawFundsRequest(String tradeId, String address) { + protected final WithdrawFundsRequest createWithdrawFundsRequest(String tradeId, + String address, + String memo) { return WithdrawFundsRequest.newBuilder() .setTradeId(tradeId) .setAddress(address) + .setMemo(memo) .build(); } @@ -283,17 +288,17 @@ protected final TxInfo sendBsq(BisqAppConfig bisqAppConfig, } protected final TxInfo sendBtc(BisqAppConfig bisqAppConfig, String address, String amount) { - return sendBtc(bisqAppConfig, address, amount, ""); + return sendBtc(bisqAppConfig, address, amount, "", ""); } protected final TxInfo sendBtc(BisqAppConfig bisqAppConfig, String address, String amount, - String txFeeRate) { + String txFeeRate, + String memo) { //noinspection ResultOfMethodCallIgnored - return grpcStubs(bisqAppConfig).walletsService.sendBtc(createSendBtcRequest(address, - amount, - txFeeRate)) + return grpcStubs(bisqAppConfig).walletsService.sendBtc( + createSendBtcRequest(address, amount, txFeeRate, memo)) .getTxInfo(); } @@ -399,8 +404,11 @@ protected final void keepFunds(BisqAppConfig bisqAppConfig, String tradeId) { } @SuppressWarnings("ResultOfMethodCallIgnored") - protected final void withdrawFunds(BisqAppConfig bisqAppConfig, String tradeId, String address) { - var req = createWithdrawFundsRequest(tradeId, address); + protected final void withdrawFunds(BisqAppConfig bisqAppConfig, + String tradeId, + String address, + String memo) { + var req = createWithdrawFundsRequest(tradeId, address, memo); grpcStubs(bisqAppConfig).tradesService.withdrawFunds(req); } diff --git a/apitest/src/test/java/bisq/apitest/method/trade/AbstractTradeTest.java b/apitest/src/test/java/bisq/apitest/method/trade/AbstractTradeTest.java index 2b88e4f2700..7f4038a0344 100644 --- a/apitest/src/test/java/bisq/apitest/method/trade/AbstractTradeTest.java +++ b/apitest/src/test/java/bisq/apitest/method/trade/AbstractTradeTest.java @@ -64,9 +64,11 @@ protected final void logTrade(Logger log, TestInfo testInfo, String description, TradeInfo trade) { - log.info(String.format("%s %s%n%s", - testName(testInfo), - description.toUpperCase(), - format(trade))); + if (log.isDebugEnabled()) { + log.debug(String.format("%s %s%n%s", + testName(testInfo), + description.toUpperCase(), + format(trade))); + } } } diff --git a/apitest/src/test/java/bisq/apitest/method/trade/TakeSellBTCOfferTest.java b/apitest/src/test/java/bisq/apitest/method/trade/TakeSellBTCOfferTest.java index 2278ce315cd..69445657416 100644 --- a/apitest/src/test/java/bisq/apitest/method/trade/TakeSellBTCOfferTest.java +++ b/apitest/src/test/java/bisq/apitest/method/trade/TakeSellBTCOfferTest.java @@ -147,7 +147,7 @@ public void testBobsBtcWithdrawalToExternalAddress(final TestInfo testInfo) { logTrade(log, testInfo, "Bob's view before withdrawing funds to external wallet", trade); String toAddress = bitcoinCli.getNewBtcAddress(); - withdrawFunds(bobdaemon, tradeId, toAddress); + withdrawFunds(bobdaemon, tradeId, toAddress, "to whom it may concern"); genBtcBlocksThenWait(1, 2250); @@ -158,7 +158,7 @@ public void testBobsBtcWithdrawalToExternalAddress(final TestInfo testInfo) { verifyExpectedProtocolStatus(trade); logTrade(log, testInfo, "Bob's view after withdrawing funds to external wallet", trade); BtcBalanceInfo currentBalance = getBtcBalances(bobdaemon); - log.info("{} Bob's current available balance: {} BTC", + log.debug("{} Bob's current available balance: {} BTC", testName(testInfo), formatSatoshis(currentBalance.getAvailableBalance())); } diff --git a/apitest/src/test/java/bisq/apitest/method/wallet/BtcWalletTest.java b/apitest/src/test/java/bisq/apitest/method/wallet/BtcWalletTest.java index a97e9a690da..7af31dcb3fc 100644 --- a/apitest/src/test/java/bisq/apitest/method/wallet/BtcWalletTest.java +++ b/apitest/src/test/java/bisq/apitest/method/wallet/BtcWalletTest.java @@ -99,7 +99,11 @@ public void testAliceSendBTCToBob(TestInfo testInfo) { String bobsBtcAddress = getUnusedBtcAddress(bobdaemon); log.debug("Sending 5.5 BTC From Alice to Bob @ {}", bobsBtcAddress); - sendBtc(alicedaemon, bobsBtcAddress, "5.50", "100"); + sendBtc(alicedaemon, + bobsBtcAddress, + "5.50", + "100", + "to whom it may concern"); genBtcBlocksThenWait(1, 3000); BtcBalanceInfo alicesBalances = getBtcBalances(alicedaemon); diff --git a/cli/src/main/java/bisq/cli/CliMain.java b/cli/src/main/java/bisq/cli/CliMain.java index 5a98f9a7eb8..b9cb4ede0fd 100644 --- a/cli/src/main/java/bisq/cli/CliMain.java +++ b/cli/src/main/java/bisq/cli/CliMain.java @@ -290,14 +290,18 @@ public static void run(String[] args) { var amount = nonOptionArgs.get(2); verifyStringIsValidDecimal(amount); - var txFeeRate = nonOptionArgs.size() == 4 ? nonOptionArgs.get(3) : ""; + // TODO Find a better way to handle the two optional parameters. + var txFeeRate = nonOptionArgs.size() >= 4 ? nonOptionArgs.get(3) : ""; if (!txFeeRate.isEmpty()) verifyStringIsValidLong(txFeeRate); + var memo = nonOptionArgs.size() == 5 ? nonOptionArgs.get(4) : ""; + var request = SendBtcRequest.newBuilder() .setAddress(address) .setAmount(amount) .setTxFeeRate(txFeeRate) + .setMemo(memo) .build(); var reply = walletsService.sendBtc(request); TxInfo txInfo = reply.getTxInfo(); @@ -496,16 +500,21 @@ public static void run(String[] args) { case withdrawfunds: { if (nonOptionArgs.size() < 3) throw new IllegalArgumentException("incorrect parameter count, " - + " expecting trade id, bitcoin wallet address"); + + " expecting trade id, bitcoin wallet address [,\"memo\"]"); var tradeId = nonOptionArgs.get(1); var address = nonOptionArgs.get(2); + // A multi-word memo must be double quoted. + var memo = nonOptionArgs.size() == 4 + ? nonOptionArgs.get(3) + : ""; var request = WithdrawFundsRequest.newBuilder() .setTradeId(tradeId) .setAddress(address) + .setMemo(memo) .build(); tradesService.withdrawFunds(request); - out.printf("funds from trade %s sent to btc address %s%n", tradeId, address); + out.printf("trade %s funds sent to btc address %s%n", tradeId, address); return; } case getpaymentmethods: { @@ -707,7 +716,7 @@ private static void printHelp(OptionParser parser, PrintStream stream) { stream.format(rowFormat, "getfundingaddresses", "", "Get BTC funding addresses"); stream.format(rowFormat, "getunusedbsqaddress", "", "Get unused BSQ address"); stream.format(rowFormat, "sendbsq", "address, amount [,tx fee rate (sats/byte)]", "Send BSQ"); - stream.format(rowFormat, "sendbtc", "address, amount [,tx fee rate (sats/byte)]", "Send BTC"); + stream.format(rowFormat, "sendbtc", "address, amount [,tx fee rate (sats/byte), \"memo\"]", "Send BTC"); stream.format(rowFormat, "gettxfeerate", "", "Get current tx fee rate in sats/byte"); stream.format(rowFormat, "settxfeerate", "satoshis (per byte)", "Set custom tx fee rate in sats/byte"); stream.format(rowFormat, "unsettxfeerate", "", "Unset custom tx fee rate"); @@ -723,7 +732,8 @@ private static void printHelp(OptionParser parser, PrintStream stream) { stream.format(rowFormat, "confirmpaymentstarted", "trade id", "Confirm payment started"); stream.format(rowFormat, "confirmpaymentreceived", "trade id", "Confirm payment received"); stream.format(rowFormat, "keepfunds", "trade id", "Keep received funds in Bisq wallet"); - stream.format(rowFormat, "withdrawfunds", "trade id, bitcoin wallet address", "Withdraw received funds to external wallet address"); + stream.format(rowFormat, "withdrawfunds", "trade id, bitcoin wallet address [,\"memo\"]", + "Withdraw received funds to external wallet address"); stream.format(rowFormat, "getpaymentmethods", "", "Get list of supported payment account method ids"); stream.format(rowFormat, "getpaymentacctform", "payment method id", "Get a new payment account form"); stream.format(rowFormat, "createpaymentacct", "path to payment account form", "Create a new payment account"); diff --git a/core/src/main/java/bisq/core/api/CoreApi.java b/core/src/main/java/bisq/core/api/CoreApi.java index 522fe8fb243..dbba310f5dd 100644 --- a/core/src/main/java/bisq/core/api/CoreApi.java +++ b/core/src/main/java/bisq/core/api/CoreApi.java @@ -48,8 +48,6 @@ import lombok.extern.slf4j.Slf4j; -import javax.annotation.Nullable; - /** * Provides high level interface to functionality of core Bisq features. * E.g. useful for different APIs to access data of different domains of Bisq. @@ -213,7 +211,7 @@ public void keepFunds(String tradeId) { coreTradesService.keepFunds(tradeId); } - public void withdrawFunds(String tradeId, String address, @Nullable String memo) { + public void withdrawFunds(String tradeId, String address, String memo) { coreTradesService.withdrawFunds(tradeId, address, memo); } @@ -259,8 +257,9 @@ public void sendBsq(String address, public void sendBtc(String address, String amount, String txFeeRate, + String memo, FutureCallback callback) { - walletsService.sendBtc(address, amount, txFeeRate, callback); + walletsService.sendBtc(address, amount, txFeeRate, memo, callback); } public void getTxFeeRate(ResultHandler resultHandler) { diff --git a/core/src/main/java/bisq/core/api/CoreTradesService.java b/core/src/main/java/bisq/core/api/CoreTradesService.java index 2f75f241407..10d21d6415d 100644 --- a/core/src/main/java/bisq/core/api/CoreTradesService.java +++ b/core/src/main/java/bisq/core/api/CoreTradesService.java @@ -41,8 +41,6 @@ import lombok.extern.slf4j.Slf4j; -import javax.annotation.Nullable; - import static bisq.core.btc.model.AddressEntry.Context.TRADE_PAYOUT; import static java.lang.String.format; @@ -85,6 +83,8 @@ void takeOffer(Offer offer, String paymentAccountId, String takerFeeCurrencyCode, Consumer resultHandler) { + coreWalletsService.verifyWalletsAreAvailable(); + coreWalletsService.verifyEncryptedWalletIsUnlocked(); offerUtil.maybeSetFeePaymentCurrencyPreference(takerFeeCurrencyCode); @@ -149,6 +149,9 @@ void confirmPaymentReceived(String tradeId) { } void keepFunds(String tradeId) { + coreWalletsService.verifyWalletsAreAvailable(); + coreWalletsService.verifyEncryptedWalletIsUnlocked(); + verifyTradeIsNotClosed(tradeId); var trade = getOpenTrade(tradeId).orElseThrow(() -> new IllegalArgumentException(format("trade with id '%s' not found", tradeId))); @@ -156,8 +159,10 @@ void keepFunds(String tradeId) { tradeManager.onTradeCompleted(trade); } - void withdrawFunds(String tradeId, String toAddress, @Nullable String memo) { - // An encrypted wallet must be unlocked for this operation. + void withdrawFunds(String tradeId, String toAddress, String memo) { + coreWalletsService.verifyWalletsAreAvailable(); + coreWalletsService.verifyEncryptedWalletIsUnlocked(); + verifyTradeIsNotClosed(tradeId); var trade = getOpenTrade(tradeId).orElseThrow(() -> new IllegalArgumentException(format("trade with id '%s' not found", tradeId))); @@ -172,21 +177,21 @@ void withdrawFunds(String tradeId, String toAddress, @Nullable String memo) { var receiverAmount = amount.subtract(fee); log.info(format("Withdrawing funds received from trade %s:" - + "%n From %s%n To %s%n Amt %s%n Tx Fee %s%n Receiver Amt %s", + + "%n From %s%n To %s%n Amt %s%n Tx Fee %s%n Receiver Amt %s%n Memo %s%n", tradeId, fromAddressEntry.getAddressString(), toAddress, amount.toFriendlyString(), fee.toFriendlyString(), - receiverAmount.toFriendlyString())); - + receiverAmount.toFriendlyString(), + memo)); tradeManager.onWithdrawRequest( toAddress, amount, fee, coreWalletsService.getKey(), trade, - memo, + memo.isEmpty() ? null : memo, () -> { }, (errorMessage, throwable) -> { @@ -196,10 +201,14 @@ void withdrawFunds(String tradeId, String toAddress, @Nullable String memo) { } String getTradeRole(String tradeId) { + coreWalletsService.verifyWalletsAreAvailable(); + coreWalletsService.verifyEncryptedWalletIsUnlocked(); return tradeUtil.getRole(getTrade(tradeId)); } Trade getTrade(String tradeId) { + coreWalletsService.verifyWalletsAreAvailable(); + coreWalletsService.verifyEncryptedWalletIsUnlocked(); return getOpenTrade(tradeId).orElseGet(() -> getClosedTrade(tradeId).orElseThrow(() -> new IllegalArgumentException(format("trade with id '%s' not found", tradeId)) diff --git a/core/src/main/java/bisq/core/api/CoreWalletsService.java b/core/src/main/java/bisq/core/api/CoreWalletsService.java index 2862f6c7e9b..2bbd585f8dd 100644 --- a/core/src/main/java/bisq/core/api/CoreWalletsService.java +++ b/core/src/main/java/bisq/core/api/CoreWalletsService.java @@ -231,6 +231,7 @@ void sendBsq(String address, void sendBtc(String address, String amount, String txFeeRate, + String memo, FutureCallback callback) { verifyWalletsAreAvailable(); verifyEncryptedWalletIsUnlocked(); @@ -271,7 +272,7 @@ void sendBtc(String address, fee, null, tempAesKey, - null, /* memo todo */ + memo.isEmpty() ? null : memo, callback); } catch (AddressEntryException ex) { log.error("", ex); @@ -420,13 +421,13 @@ void removeWalletPassword(String password) { } // Throws a RuntimeException if wallets are not available (encrypted or not). - private void verifyWalletsAreAvailable() { + void verifyWalletsAreAvailable() { if (!walletsManager.areWalletsAvailable()) throw new IllegalStateException("wallet is not yet available"); } // Throws a RuntimeException if wallets are not available or not encrypted. - private void verifyWalletIsAvailableAndEncrypted() { + void verifyWalletIsAvailableAndEncrypted() { if (!walletsManager.areWalletsAvailable()) throw new IllegalStateException("wallet is not yet available"); @@ -435,7 +436,7 @@ private void verifyWalletIsAvailableAndEncrypted() { } // Throws a RuntimeException if wallets are encrypted and locked. - private void verifyEncryptedWalletIsUnlocked() { + void verifyEncryptedWalletIsUnlocked() { if (walletsManager.areWalletsEncrypted() && tempAesKey == null) throw new IllegalStateException("wallet is locked"); } diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcTradesService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcTradesService.java index 1dbb453a22a..8c04f67b059 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcTradesService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcTradesService.java @@ -39,11 +39,14 @@ import io.grpc.StatusRuntimeException; import io.grpc.stub.StreamObserver; +import org.bitcoinj.core.Transaction; + import javax.inject.Inject; import lombok.extern.slf4j.Slf4j; import static bisq.core.api.model.TradeInfo.toTradeInfo; +import static bisq.core.api.model.TxInfo.toTxInfo; @Slf4j class GrpcTradesService extends TradesGrpc.TradesImplBase { @@ -144,8 +147,7 @@ public void keepFunds(KeepFundsRequest req, public void withdrawFunds(WithdrawFundsRequest req, StreamObserver responseObserver) { try { - //TODO @ghubstan Feel free to add a memo param for withdrawal requests (was just added in UI) - coreApi.withdrawFunds(req.getTradeId(), req.getAddress(), null); + coreApi.withdrawFunds(req.getTradeId(), req.getAddress(), req.getMemo()); var reply = WithdrawFundsReply.newBuilder().build(); responseObserver.onNext(reply); responseObserver.onCompleted(); diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java index b902e805edd..c7bbb4623da 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java @@ -190,6 +190,7 @@ public void sendBtc(SendBtcRequest req, coreApi.sendBtc(req.getAddress(), req.getAmount(), req.getTxFeeRate(), + req.getMemo(), new FutureCallback<>() { @Override public void onSuccess(Transaction tx) { diff --git a/proto/src/main/proto/grpc.proto b/proto/src/main/proto/grpc.proto index 7bfaf219f73..a6e30cdb473 100644 --- a/proto/src/main/proto/grpc.proto +++ b/proto/src/main/proto/grpc.proto @@ -255,6 +255,7 @@ message KeepFundsReply { message WithdrawFundsRequest { string tradeId = 1; string address = 2; + string memo = 3; } message WithdrawFundsReply { @@ -375,6 +376,7 @@ message SendBtcRequest { string address = 1; string amount = 2; string txFeeRate = 3; + string memo = 4; } message SendBtcReply { From 478c8f4eb23c510a06e0fdde1be9342684005902 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Wed, 9 Dec 2020 17:38:20 -0300 Subject: [PATCH 05/45] Remove unused imports --- daemon/src/main/java/bisq/daemon/grpc/GrpcTradesService.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcTradesService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcTradesService.java index 8c04f67b059..1028c0cf9d7 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcTradesService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcTradesService.java @@ -39,14 +39,11 @@ import io.grpc.StatusRuntimeException; import io.grpc.stub.StreamObserver; -import org.bitcoinj.core.Transaction; - import javax.inject.Inject; import lombok.extern.slf4j.Slf4j; import static bisq.core.api.model.TradeInfo.toTradeInfo; -import static bisq.core.api.model.TxInfo.toTxInfo; @Slf4j class GrpcTradesService extends TradesGrpc.TradesImplBase { From 150e2f685126e7ab7f7501d3f212ce5fa58452fd Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Fri, 11 Dec 2020 18:33:19 -0300 Subject: [PATCH 06/45] Use Bisq's UserThread.executor in gRPC server --- daemon/src/main/java/bisq/daemon/grpc/GrpcServer.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcServer.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcServer.java index bb9dbebd273..40efdd07ac2 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcServer.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcServer.java @@ -17,6 +17,7 @@ package bisq.daemon.grpc; +import bisq.common.UserThread; import bisq.common.config.Config; import io.grpc.Server; @@ -48,6 +49,7 @@ public GrpcServer(Config config, GrpcTradesService tradesService, GrpcWalletsService walletsService) { this.server = ServerBuilder.forPort(config.apiPort) + .executor(UserThread.getExecutor()) .addService(disputeAgentsService) .addService(offersService) .addService(paymentAccountsService) From 6aa385e494eb8fa870257c76e078108607503d03 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Sat, 12 Dec 2020 12:01:55 -0300 Subject: [PATCH 07/45] Append nullable withdrawalTxId field to Trade proto message The withdrawalTxId field will be set in TradeManager#onWithdrawRequest upon successful trade completion and withdrawal of funds. Persisting withdrawalTxId will allow the api and ui to find the withdrawalTxId for a completed trade after the seller withdraws funds to an external wallet. In turn, the withdrawal tx's memo field will be accessible in a new (todo) api getTx(txID) api method. Changed: - Appended field 'string withdrawal_tx_id = 40' to pb.proto's Trade message. - Added nullable 'String withdrawalTxId' to Trade entity class. - Added trade.setWithdrawalTxId(transaction.getTxId().toString()) in TradeManager#onWithdrawRequest's callback. --- core/src/main/java/bisq/core/trade/Trade.java | 7 +++++++ core/src/main/java/bisq/core/trade/TradeManager.java | 1 + proto/src/main/proto/pb.proto | 1 + 3 files changed, 9 insertions(+) diff --git a/core/src/main/java/bisq/core/trade/Trade.java b/core/src/main/java/bisq/core/trade/Trade.java index de5c1949bb7..b25587eba97 100644 --- a/core/src/main/java/bisq/core/trade/Trade.java +++ b/core/src/main/java/bisq/core/trade/Trade.java @@ -317,6 +317,10 @@ public static protobuf.Trade.TradePeriodState toProtoMessage(Trade.TradePeriodSt @Getter @Setter private String payoutTxId; + @Nullable + @Getter + @Setter + private String withdrawalTxId; @Getter @Setter private long tradeAmountAsLong; @@ -554,6 +558,7 @@ public Message toProtoMessage() { Optional.ofNullable(takerFeeTxId).ifPresent(builder::setTakerFeeTxId); Optional.ofNullable(depositTxId).ifPresent(builder::setDepositTxId); Optional.ofNullable(payoutTxId).ifPresent(builder::setPayoutTxId); + Optional.ofNullable(withdrawalTxId).ifPresent(builder::setWithdrawalTxId); Optional.ofNullable(tradingPeerNodeAddress).ifPresent(e -> builder.setTradingPeerNodeAddress(tradingPeerNodeAddress.toProtoMessage())); Optional.ofNullable(contract).ifPresent(e -> builder.setContract(contract.toProtoMessage())); Optional.ofNullable(contractAsJson).ifPresent(builder::setContractAsJson); @@ -587,6 +592,7 @@ public static Trade fromProto(Trade trade, protobuf.Trade proto, CoreProtoResolv trade.setTakerFeeTxId(ProtoUtil.stringOrNullFromProto(proto.getTakerFeeTxId())); trade.setDepositTxId(ProtoUtil.stringOrNullFromProto(proto.getDepositTxId())); trade.setPayoutTxId(ProtoUtil.stringOrNullFromProto(proto.getPayoutTxId())); + trade.setWithdrawalTxId(ProtoUtil.stringOrNullFromProto(proto.getWithdrawalTxId())); trade.setContract(proto.hasContract() ? Contract.fromProto(proto.getContract(), coreProtoResolver) : null); trade.setContractAsJson(ProtoUtil.stringOrNullFromProto(proto.getContractAsJson())); trade.setContractHash(ProtoUtil.byteArrayOrNullFromProto(proto.getContractHash())); @@ -1124,6 +1130,7 @@ public String toString() { ",\n takerFeeTxId='" + takerFeeTxId + '\'' + ",\n depositTxId='" + depositTxId + '\'' + ",\n payoutTxId='" + payoutTxId + '\'' + + ",\n withdrawalTxId='" + withdrawalTxId + '\'' + ",\n tradeAmountAsLong=" + tradeAmountAsLong + ",\n tradePrice=" + tradePrice + ",\n tradingPeerNodeAddress=" + tradingPeerNodeAddress + diff --git a/core/src/main/java/bisq/core/trade/TradeManager.java b/core/src/main/java/bisq/core/trade/TradeManager.java index dc87d4a98cc..d6c99e28e3e 100644 --- a/core/src/main/java/bisq/core/trade/TradeManager.java +++ b/core/src/main/java/bisq/core/trade/TradeManager.java @@ -497,6 +497,7 @@ public void onSuccess(@javax.annotation.Nullable Transaction transaction) { onTradeCompleted(trade); trade.setState(Trade.State.WITHDRAW_COMPLETED); getTradeProtocol(trade).onWithdrawCompleted(); + trade.setWithdrawalTxId(transaction.getTxId().toString()); requestPersistence(); resultHandler.handleResult(); } diff --git a/proto/src/main/proto/pb.proto b/proto/src/main/proto/pb.proto index 22d81d40de9..a4579016f0c 100644 --- a/proto/src/main/proto/pb.proto +++ b/proto/src/main/proto/pb.proto @@ -1450,6 +1450,7 @@ message Trade { string counter_currency_extra_data = 37; string asset_tx_proof_result = 38; // name of AssetTxProofResult enum string uid = 39; + string withdrawal_tx_id = 40; } message BuyerAsMakerTrade { From 5522d0c53e96312a75dbb88da72bec8480c97bc8 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Mon, 14 Dec 2020 15:01:03 -0300 Subject: [PATCH 08/45] Add new api method gettransaction This change was prompted by the recent changes in the main branch to allow a tx memo field to be set from the UI and API. This and the prior PR address the API's need to be able to fetch a tx (with a memo). The API can now get a completed trade's withdrawal txid and pass it as a gettransaction parameter. See previous PR "Append nullable withdrawalTxId field to Trade". https://github.com/bisq-network/bisq/pull/4937 A summary of changes by file: grpc.proto - Added withdrawalTxId field to existing TradeInfo proto & wrapper. - Reordered fields in TradeInfo proto. - Added new fields to be displayed by TxInfo proto in CLI. - Fixed typo: unsetTxFeeRatePreference -> UnsetTxFeeRatePreference. - Added new GetTransaction rpc. GrpcWalletsService - Added new getTransaction gRPC boilerplate. CoreWalletsService - Added new getTransaction implementation. TxInfo - Added the new fields for displaying a tx summary from CLI. This is not intended to be more than a brief summary; a block explorer or bitcoin-core client should be used to see the complete definition. TradeInfo - Added the new withdrawalTxId field defined in grpc.proto. CliMain - Added new 'case gettransaction'. TransactionFormat - Formats a TxInfo sent from the server to CLI. ColumnHeaderConstants - Added console headers used by TransactionFormat. TradeFormat - Displays a completed trade's WithdrawalTxId if present. Apitest - Adjusted affected tests: assert tx memo is persisted and test gettransaction. --- .../java/bisq/apitest/method/MethodTest.java | 6 + .../payment/CreatePaymentAccountTest.java | 7 +- .../method/trade/TakeSellBTCOfferTest.java | 23 +++- .../apitest/method/wallet/BtcWalletTest.java | 18 ++- .../java/bisq/apitest/scenario/TradeTest.java | 1 + cli/src/main/java/bisq/cli/CliMain.java | 27 ++++- .../java/bisq/cli/ColumnHeaderConstants.java | 10 ++ cli/src/main/java/bisq/cli/TradeFormat.java | 33 ++--- .../main/java/bisq/cli/TransactionFormat.java | 59 +++++++++ core/src/main/java/bisq/core/api/CoreApi.java | 4 + .../bisq/core/api/CoreWalletsService.java | 20 +++ .../java/bisq/core/api/model/TradeInfo.java | 11 ++ .../main/java/bisq/core/api/model/TxInfo.java | 114 +++++++++++++++--- .../bisq/daemon/grpc/GrpcWalletsService.java | 19 +++ proto/src/main/proto/grpc.proto | 50 +++++--- 15 files changed, 341 insertions(+), 61 deletions(-) create mode 100644 cli/src/main/java/bisq/cli/TransactionFormat.java diff --git a/apitest/src/test/java/bisq/apitest/method/MethodTest.java b/apitest/src/test/java/bisq/apitest/method/MethodTest.java index 95918189424..791e22ef6c2 100644 --- a/apitest/src/test/java/bisq/apitest/method/MethodTest.java +++ b/apitest/src/test/java/bisq/apitest/method/MethodTest.java @@ -39,6 +39,7 @@ import bisq.proto.grpc.GetPaymentAccountsRequest; import bisq.proto.grpc.GetPaymentMethodsRequest; import bisq.proto.grpc.GetTradeRequest; +import bisq.proto.grpc.GetTransactionRequest; import bisq.proto.grpc.GetTxFeeRateRequest; import bisq.proto.grpc.GetUnusedBsqAddressRequest; import bisq.proto.grpc.KeepFundsRequest; @@ -432,6 +433,11 @@ protected final TxFeeRateInfo unsetTxFeeRate(BisqAppConfig bisqAppConfig) { grpcStubs(bisqAppConfig).walletsService.unsetTxFeeRatePreference(req).getTxFeeRateInfo()); } + protected final TxInfo getTransaction(BisqAppConfig bisqAppConfig, String txId) { + var req = GetTransactionRequest.newBuilder().setTxId(txId).build(); + return grpcStubs(bisqAppConfig).walletsService.getTransaction(req).getTxInfo(); + } + // Static conveniences for test methods and test case fixture setups. protected static RegisterDisputeAgentRequest createRegisterDisputeAgentRequest(String disputeAgentType) { diff --git a/apitest/src/test/java/bisq/apitest/method/payment/CreatePaymentAccountTest.java b/apitest/src/test/java/bisq/apitest/method/payment/CreatePaymentAccountTest.java index 9795eec28c4..ac66cc7993e 100644 --- a/apitest/src/test/java/bisq/apitest/method/payment/CreatePaymentAccountTest.java +++ b/apitest/src/test/java/bisq/apitest/method/payment/CreatePaymentAccountTest.java @@ -746,7 +746,12 @@ public void testCreateTransferwiseAccount(TestInfo testInfo) { String jsonString = getCompletedFormAsJsonString(); TransferwiseAccount paymentAccount = (TransferwiseAccount) createPaymentAccount(alicedaemon, jsonString); verifyUserPayloadHasPaymentAccountWithId(paymentAccount.getId()); - verifyAccountTradeCurrencies(getAllTransferwiseCurrencies(), paymentAccount); + verifyUserPayloadHasPaymentAccountWithId(paymentAccount.getId()); + // As per commit 88f26f93241af698ae689bf081205d0f9dc929fa + // Do not autofill all currencies by default but keep all unselected. + // verifyAccountTradeCurrencies(getAllTransferwiseCurrencies(), paymentAccount); + // TODO uncomment after master/merge + // assertEquals(0, paymentAccount.getTradeCurrencies().size()); verifyCommonFormEntries(paymentAccount); assertEquals(COMPLETED_FORM_MAP.get(PROPERTY_NAME_EMAIL), paymentAccount.getEmail()); log.debug("Deserialized {}: {}", paymentAccount.getClass().getSimpleName(), paymentAccount); diff --git a/apitest/src/test/java/bisq/apitest/method/trade/TakeSellBTCOfferTest.java b/apitest/src/test/java/bisq/apitest/method/trade/TakeSellBTCOfferTest.java index 69445657416..ce5bceff7a1 100644 --- a/apitest/src/test/java/bisq/apitest/method/trade/TakeSellBTCOfferTest.java +++ b/apitest/src/test/java/bisq/apitest/method/trade/TakeSellBTCOfferTest.java @@ -23,7 +23,6 @@ import lombok.extern.slf4j.Slf4j; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.MethodOrderer; import org.junit.jupiter.api.Order; import org.junit.jupiter.api.Test; @@ -33,6 +32,7 @@ import static bisq.apitest.config.BisqAppConfig.alicedaemon; import static bisq.apitest.config.BisqAppConfig.bobdaemon; import static bisq.cli.CurrencyFormat.formatSatoshis; +import static bisq.cli.TransactionFormat.format; import static bisq.core.trade.Trade.Phase.*; import static bisq.core.trade.Trade.State.*; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -42,7 +42,7 @@ import static protobuf.Offer.State.OFFER_FEE_PAID; import static protobuf.OpenOffer.State.AVAILABLE; -@Disabled +// @Disabled @Slf4j @TestMethodOrder(MethodOrderer.OrderAnnotation.class) public class TakeSellBTCOfferTest extends AbstractTradeTest { @@ -52,6 +52,8 @@ public class TakeSellBTCOfferTest extends AbstractTradeTest { // Maker and Taker fees are in BTC. private static final String TRADE_FEE_CURRENCY_CODE = "btc"; + private static final String WITHDRAWAL_TX_MEMO = "Bob's trade withdrawal"; + @Test @Order(1) public void testTakeAlicesSellOffer(final TestInfo testInfo) { @@ -147,7 +149,7 @@ public void testBobsBtcWithdrawalToExternalAddress(final TestInfo testInfo) { logTrade(log, testInfo, "Bob's view before withdrawing funds to external wallet", trade); String toAddress = bitcoinCli.getNewBtcAddress(); - withdrawFunds(bobdaemon, tradeId, toAddress, "to whom it may concern"); + withdrawFunds(bobdaemon, tradeId, toAddress, WITHDRAWAL_TX_MEMO); genBtcBlocksThenWait(1, 2250); @@ -162,4 +164,19 @@ public void testBobsBtcWithdrawalToExternalAddress(final TestInfo testInfo) { testName(testInfo), formatSatoshis(currentBalance.getAvailableBalance())); } + + @Test + @Order(5) + public void testGetTradeWithdrawalTx(final TestInfo testInfo) { + var trade = getTrade(bobdaemon, tradeId); + var withdrawalTxId = trade.getWithdrawalTxId(); + assertNotNull(withdrawalTxId); + + var txInfo = getTransaction(bobdaemon, withdrawalTxId); + assertEquals(WITHDRAWAL_TX_MEMO, txInfo.getMemo()); + + log.debug("{} Trade withdrawal Tx:\n{}", + testName(testInfo), + format(txInfo)); + } } diff --git a/apitest/src/test/java/bisq/apitest/method/wallet/BtcWalletTest.java b/apitest/src/test/java/bisq/apitest/method/wallet/BtcWalletTest.java index 7af31dcb3fc..90c46a3c814 100644 --- a/apitest/src/test/java/bisq/apitest/method/wallet/BtcWalletTest.java +++ b/apitest/src/test/java/bisq/apitest/method/wallet/BtcWalletTest.java @@ -1,6 +1,7 @@ package bisq.apitest.method.wallet; import bisq.proto.grpc.BtcBalanceInfo; +import bisq.proto.grpc.TxInfo; import lombok.extern.slf4j.Slf4j; @@ -20,6 +21,7 @@ import static bisq.cli.TableFormat.formatBtcBalanceInfoTbl; import static java.util.Collections.singletonList; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.MethodOrderer.OrderAnnotation; @@ -32,6 +34,8 @@ @TestMethodOrder(OrderAnnotation.class) public class BtcWalletTest extends MethodTest { + private static final String TX_MEMO = "tx memo"; + // All api tests depend on the DAO / regtest environment, and Bob & Alice's wallets // are initialized with 10 BTC during the scaffolding setup. private static final bisq.core.api.model.BtcBalanceInfo INITIAL_BTC_BALANCES = @@ -99,15 +103,23 @@ public void testAliceSendBTCToBob(TestInfo testInfo) { String bobsBtcAddress = getUnusedBtcAddress(bobdaemon); log.debug("Sending 5.5 BTC From Alice to Bob @ {}", bobsBtcAddress); - sendBtc(alicedaemon, + TxInfo txInfo = sendBtc(alicedaemon, bobsBtcAddress, "5.50", "100", - "to whom it may concern"); + TX_MEMO); + assertTrue(txInfo.getIsPending()); + + // Note that the memo is not set on the tx yet. + assertTrue(txInfo.getMemo().isEmpty()); genBtcBlocksThenWait(1, 3000); - BtcBalanceInfo alicesBalances = getBtcBalances(alicedaemon); + // Fetch the tx and check for confirmation and memo. + txInfo = getTransaction(alicedaemon, txInfo.getTxId()); + assertFalse(txInfo.getIsPending()); + assertEquals(TX_MEMO, txInfo.getMemo()); + BtcBalanceInfo alicesBalances = getBtcBalances(alicedaemon); log.debug("{} Alice's BTC Balances:\n{}", testName(testInfo), formatBtcBalanceInfoTbl(alicesBalances)); diff --git a/apitest/src/test/java/bisq/apitest/scenario/TradeTest.java b/apitest/src/test/java/bisq/apitest/scenario/TradeTest.java index 4c07452abc6..410b72ca6cb 100644 --- a/apitest/src/test/java/bisq/apitest/scenario/TradeTest.java +++ b/apitest/src/test/java/bisq/apitest/scenario/TradeTest.java @@ -60,5 +60,6 @@ public void testTakeSellBTCOffer(final TestInfo testInfo) { test.testBobsConfirmPaymentStarted(testInfo); test.testAlicesConfirmPaymentReceived(testInfo); test.testBobsBtcWithdrawalToExternalAddress(testInfo); + test.testGetTradeWithdrawalTx(testInfo); } } diff --git a/cli/src/main/java/bisq/cli/CliMain.java b/cli/src/main/java/bisq/cli/CliMain.java index b9cb4ede0fd..4853e3a76b3 100644 --- a/cli/src/main/java/bisq/cli/CliMain.java +++ b/cli/src/main/java/bisq/cli/CliMain.java @@ -31,6 +31,7 @@ import bisq.proto.grpc.GetPaymentAccountsRequest; import bisq.proto.grpc.GetPaymentMethodsRequest; import bisq.proto.grpc.GetTradeRequest; +import bisq.proto.grpc.GetTransactionRequest; import bisq.proto.grpc.GetTxFeeRateRequest; import bisq.proto.grpc.GetUnusedBsqAddressRequest; import bisq.proto.grpc.GetVersionRequest; @@ -116,6 +117,7 @@ private enum Method { gettxfeerate, settxfeerate, unsettxfeerate, + gettransaction, lockwallet, unlockwallet, removewalletpassword, @@ -275,7 +277,10 @@ public static void run(String[] args) { .build(); var reply = walletsService.sendBsq(request); TxInfo txInfo = reply.getTxInfo(); - out.printf("%s bsq sent to %s in tx %s%n", amount, address, txInfo.getId()); + out.printf("%s bsq sent to %s in tx %s%n", + amount, + address, + txInfo.getTxId()); return; } case sendbtc: { @@ -305,7 +310,10 @@ public static void run(String[] args) { .build(); var reply = walletsService.sendBtc(request); TxInfo txInfo = reply.getTxInfo(); - out.printf("%s btc sent to %s in tx %s%n", amount, address, txInfo.getId()); + out.printf("%s btc sent to %s in tx %s%n", + amount, + address, + txInfo.getTxId()); return; } case gettxfeerate: { @@ -332,6 +340,18 @@ public static void run(String[] args) { out.println(formatTxFeeRateInfo(reply.getTxFeeRateInfo())); return; } + case gettransaction: { + if (nonOptionArgs.size() < 2) + throw new IllegalArgumentException("no tx id specified"); + + var txId = nonOptionArgs.get(1); + var request = GetTransactionRequest.newBuilder() + .setTxId(txId) + .build(); + var reply = walletsService.getTransaction(request); + out.println(TransactionFormat.format(reply.getTxInfo())); + return; + } case createoffer: { if (nonOptionArgs.size() < 9) throw new IllegalArgumentException("incorrect parameter count," @@ -441,7 +461,7 @@ public static void run(String[] args) { return; } case gettrade: { - // TODO make short-id a valid argument + // TODO make short-id a valid argument? if (nonOptionArgs.size() < 2) throw new IllegalArgumentException("incorrect parameter count, " + " expecting trade id [,showcontract = true|false]"); @@ -720,6 +740,7 @@ private static void printHelp(OptionParser parser, PrintStream stream) { stream.format(rowFormat, "gettxfeerate", "", "Get current tx fee rate in sats/byte"); stream.format(rowFormat, "settxfeerate", "satoshis (per byte)", "Set custom tx fee rate in sats/byte"); stream.format(rowFormat, "unsettxfeerate", "", "Unset custom tx fee rate"); + stream.format(rowFormat, "gettransaction", "transaction id", "Get transaction with id"); stream.format(rowFormat, "createoffer", "payment acct id, buy | sell, currency code, \\", "Create and place an offer"); stream.format(rowFormat, "", "amount (btc), min amount, use mkt based price, \\", ""); stream.format(rowFormat, "", "fixed price (btc) | mkt price margin (%), security deposit (%) \\", ""); diff --git a/cli/src/main/java/bisq/cli/ColumnHeaderConstants.java b/cli/src/main/java/bisq/cli/ColumnHeaderConstants.java index 59b6230a2eb..e81e407d7b9 100644 --- a/cli/src/main/java/bisq/cli/ColumnHeaderConstants.java +++ b/cli/src/main/java/bisq/cli/ColumnHeaderConstants.java @@ -59,6 +59,16 @@ class ColumnHeaderConstants { static final String COL_HEADER_TRADE_SHORT_ID = "ID"; static final String COL_HEADER_TRADE_TX_FEE = "Tx Fee(%-3s)"; static final String COL_HEADER_TRADE_TAKER_FEE = "Taker Fee(%-3s)"; + static final String COL_HEADER_TRADE_WITHDRAWAL_TX_ID = "Withdrawal TX ID"; + + static final String COL_HEADER_TX_ID = "Tx ID"; + static final String COL_HEADER_TX_INPUT_SUM = "Tx Inputs (BTC)"; + static final String COL_HEADER_TX_OUTPUT_SUM = "Tx Outputs (BTC)"; + static final String COL_HEADER_TX_FEE = "Tx Fee (BTC)"; + static final String COL_HEADER_TX_SIZE = "Tx Size (Bytes)"; + static final String COL_HEADER_TX_IS_CONFIRMED = "Is Confirmed"; + static final String COL_HEADER_TX_MEMO = "Memo"; + static final String COL_HEADER_VOLUME = padEnd("%-3s(min - max)", 15, ' '); static final String COL_HEADER_UUID = padEnd("ID", 52, ' '); } diff --git a/cli/src/main/java/bisq/cli/TradeFormat.java b/cli/src/main/java/bisq/cli/TradeFormat.java index 2a28c1dccc4..0c4d4d29498 100644 --- a/cli/src/main/java/bisq/cli/TradeFormat.java +++ b/cli/src/main/java/bisq/cli/TradeFormat.java @@ -58,6 +58,7 @@ public static String format(TradeInfo tradeInfo) { + COL_HEADER_TRADE_FIAT_RECEIVED + COL_HEADER_DELIMITER + COL_HEADER_TRADE_PAYOUT_PUBLISHED + COL_HEADER_DELIMITER + COL_HEADER_TRADE_WITHDRAWN + COL_HEADER_DELIMITER + + (tradeInfo.getIsWithdrawn() ? COL_HEADER_TRADE_WITHDRAWAL_TX_ID + COL_HEADER_DELIMITER : "") + "%n"; String counterCurrencyCode = tradeInfo.getOffer().getCounterCurrencyCode(); @@ -66,18 +67,20 @@ public static String format(TradeInfo tradeInfo) { ? String.format(headersFormat, counterCurrencyCode, baseCurrencyCode, baseCurrencyCode, baseCurrencyCode) : String.format(headersFormat, counterCurrencyCode, baseCurrencyCode, baseCurrencyCode); - String colDataFormat = "%-" + shortIdColWidth + "s" // left justify - + " %-" + (roleColWidth + COL_HEADER_DELIMITER.length()) + "s" // left justify - + "%" + (COL_HEADER_PRICE.length() - 1) + "s" // right justify - + "%" + (COL_HEADER_TRADE_AMOUNT.length() + 1) + "s" // right justify - + "%" + (COL_HEADER_TRADE_TX_FEE.length() + 1) + "s" // right justify - + takerFeeHeader.get() // right justify - + " %-" + COL_HEADER_TRADE_DEPOSIT_PUBLISHED.length() + "s" // left justify - + " %-" + COL_HEADER_TRADE_DEPOSIT_CONFIRMED.length() + "s" // left justify - + " %-" + COL_HEADER_TRADE_FIAT_SENT.length() + "s" // left justify - + " %-" + COL_HEADER_TRADE_FIAT_RECEIVED.length() + "s" // left justify - + " %-" + COL_HEADER_TRADE_PAYOUT_PUBLISHED.length() + "s" // left justify - + " %-" + COL_HEADER_TRADE_WITHDRAWN.length() + "s"; // left justify + + String colDataFormat = "%-" + shortIdColWidth + "s" // lt justify + + " %-" + (roleColWidth + COL_HEADER_DELIMITER.length()) + "s" // left + + "%" + (COL_HEADER_PRICE.length() - 1) + "s" // rt justify + + "%" + (COL_HEADER_TRADE_AMOUNT.length() + 1) + "s" // rt justify + + "%" + (COL_HEADER_TRADE_TX_FEE.length() + 1) + "s" // rt justify + + takerFeeHeader.get() // rt justify + + " %-" + COL_HEADER_TRADE_DEPOSIT_PUBLISHED.length() + "s" // lt justify + + " %-" + COL_HEADER_TRADE_DEPOSIT_CONFIRMED.length() + "s" // lt justify + + " %-" + COL_HEADER_TRADE_FIAT_SENT.length() + "s" // lt justify + + " %-" + COL_HEADER_TRADE_FIAT_RECEIVED.length() + "s" // lt justify + + " %-" + COL_HEADER_TRADE_PAYOUT_PUBLISHED.length() + "s" // lt justify + + " %-" + COL_HEADER_TRADE_WITHDRAWN.length() + "s" // lt justify + + " %-" + COL_HEADER_TRADE_WITHDRAWAL_TX_ID.length() + "s"; // left return headerLine + (isTaker @@ -97,7 +100,8 @@ private static String formatTradeForMaker(String format, TradeInfo tradeInfo) { tradeInfo.getIsFiatSent() ? "YES" : "NO", tradeInfo.getIsFiatReceived() ? "YES" : "NO", tradeInfo.getIsPayoutPublished() ? "YES" : "NO", - tradeInfo.getIsWithdrawn() ? "YES" : "NO"); + tradeInfo.getIsWithdrawn() ? "YES" : "NO", + tradeInfo.getIsWithdrawn() ? tradeInfo.getWithdrawalTxId() : ""); } private static String formatTradeForTaker(String format, TradeInfo tradeInfo) { @@ -113,6 +117,7 @@ private static String formatTradeForTaker(String format, TradeInfo tradeInfo) { tradeInfo.getIsFiatSent() ? "YES" : "NO", tradeInfo.getIsFiatReceived() ? "YES" : "NO", tradeInfo.getIsPayoutPublished() ? "YES" : "NO", - tradeInfo.getIsWithdrawn() ? "YES" : "NO"); + tradeInfo.getIsWithdrawn() ? "YES" : "NO", + tradeInfo.getIsWithdrawn() ? tradeInfo.getWithdrawalTxId() : ""); } } diff --git a/cli/src/main/java/bisq/cli/TransactionFormat.java b/cli/src/main/java/bisq/cli/TransactionFormat.java new file mode 100644 index 00000000000..608c2fcb71f --- /dev/null +++ b/cli/src/main/java/bisq/cli/TransactionFormat.java @@ -0,0 +1,59 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + +package bisq.cli; + +import bisq.proto.grpc.TxInfo; + +import com.google.common.annotations.VisibleForTesting; + +import static bisq.cli.ColumnHeaderConstants.*; +import static bisq.cli.CurrencyFormat.formatSatoshis; +import static com.google.common.base.Strings.padEnd; + +@VisibleForTesting +public class TransactionFormat { + + public static String format(TxInfo txInfo) { + String headerLine = padEnd(COL_HEADER_TX_ID, txInfo.getTxId().length(), ' ') + COL_HEADER_DELIMITER + + COL_HEADER_TX_IS_CONFIRMED + COL_HEADER_DELIMITER + + COL_HEADER_TX_INPUT_SUM + COL_HEADER_DELIMITER + + COL_HEADER_TX_OUTPUT_SUM + COL_HEADER_DELIMITER + + COL_HEADER_TX_FEE + COL_HEADER_DELIMITER + + COL_HEADER_TX_SIZE + COL_HEADER_DELIMITER + + (txInfo.getMemo().isEmpty() ? "" : COL_HEADER_TX_MEMO + COL_HEADER_DELIMITER) + + "\n"; + + String colDataFormat = "%-" + txInfo.getTxId().length() + "s" + + " %" + COL_HEADER_TX_IS_CONFIRMED.length() + "s" + + " %" + COL_HEADER_TX_INPUT_SUM.length() + "s" + + " %" + COL_HEADER_TX_OUTPUT_SUM.length() + "s" + + " %" + COL_HEADER_TX_FEE.length() + "s" + + " %" + COL_HEADER_TX_SIZE.length() + "s" + + " %s"; + + return headerLine + + String.format(colDataFormat, + txInfo.getTxId(), + txInfo.getIsPending() ? "NO" : "YES", // pending=true means not confirmed + formatSatoshis(txInfo.getInputSum()), + formatSatoshis(txInfo.getOutputSum()), + formatSatoshis(txInfo.getFee()), + txInfo.getSize(), + txInfo.getMemo().isEmpty() ? "" : txInfo.getMemo()); + } +} diff --git a/core/src/main/java/bisq/core/api/CoreApi.java b/core/src/main/java/bisq/core/api/CoreApi.java index dbba310f5dd..6709bf42ff1 100644 --- a/core/src/main/java/bisq/core/api/CoreApi.java +++ b/core/src/main/java/bisq/core/api/CoreApi.java @@ -279,6 +279,10 @@ public TxFeeRateInfo getMostRecentTxFeeRateInfo() { return walletsService.getMostRecentTxFeeRateInfo(); } + public Transaction getTransaction(String txId) { + return walletsService.getTransaction(txId); + } + public void setWalletPassword(String password, String newPassword) { walletsService.setWalletPassword(password, newPassword); } diff --git a/core/src/main/java/bisq/core/api/CoreWalletsService.java b/core/src/main/java/bisq/core/api/CoreWalletsService.java index 2bbd585f8dd..c107259ff98 100644 --- a/core/src/main/java/bisq/core/api/CoreWalletsService.java +++ b/core/src/main/java/bisq/core/api/CoreWalletsService.java @@ -331,6 +331,26 @@ TxFeeRateInfo getMostRecentTxFeeRateInfo() { feeService.getLastRequest()); } + Transaction getTransaction(String txId) { + if (txId.length() != 64) + throw new IllegalArgumentException(format("%s is not a transaction id", txId)); + + try { + Transaction tx = btcWalletService.getTransaction(txId); + if (tx == null) + throw new IllegalArgumentException(format("tx with id %s not found", txId)); + else + return tx; + + } catch (IllegalArgumentException ex) { + log.error("", ex); + throw new IllegalArgumentException( + format("could not get transaction with id %s%ncause: %s", + txId, + ex.getMessage().toLowerCase())); + } + } + int getNumConfirmationsForMostRecentTransaction(String addressString) { Address address = getAddressEntry(addressString).getAddress(); TransactionConfidence confidence = btcWalletService.getConfidenceForAddress(address); diff --git a/core/src/main/java/bisq/core/api/model/TradeInfo.java b/core/src/main/java/bisq/core/api/model/TradeInfo.java index 1a717a7672e..ec9880d5c69 100644 --- a/core/src/main/java/bisq/core/api/model/TradeInfo.java +++ b/core/src/main/java/bisq/core/api/model/TradeInfo.java @@ -47,6 +47,7 @@ public class TradeInfo implements Payload { private final String takerFeeTxId; private final String depositTxId; private final String payoutTxId; + private final String withdrawalTxId; private final long tradeAmountAsLong; private final long tradePrice; private final String tradingPeerNodeAddress; @@ -73,6 +74,7 @@ public TradeInfo(TradeInfoBuilder builder) { this.takerFeeTxId = builder.takerFeeTxId; this.depositTxId = builder.depositTxId; this.payoutTxId = builder.payoutTxId; + this.withdrawalTxId = builder.withdrawalTxId; this.tradeAmountAsLong = builder.tradeAmountAsLong; this.tradePrice = builder.tradePrice; this.tradingPeerNodeAddress = builder.tradingPeerNodeAddress; @@ -106,6 +108,7 @@ public static TradeInfo toTradeInfo(Trade trade, String role) { .withTakerFeeTxId(trade.getTakerFeeTxId()) .withDepositTxId(trade.getDepositTxId()) .withPayoutTxId(trade.getPayoutTxId()) + .withWithdrawalTxId(trade.getWithdrawalTxId()) .withTradeAmountAsLong(trade.getTradeAmountAsLong()) .withTradePrice(trade.getTradePrice().getValue()) .withTradingPeerNodeAddress(Objects.requireNonNull( @@ -141,6 +144,7 @@ public bisq.proto.grpc.TradeInfo toProtoMessage() { .setTakerFeeTxId(takerFeeTxId == null ? "" : takerFeeTxId) .setDepositTxId(depositTxId == null ? "" : depositTxId) .setPayoutTxId(payoutTxId == null ? "" : payoutTxId) + .setWithdrawalTxId(withdrawalTxId == null ? "" : withdrawalTxId) .setTradeAmountAsLong(tradeAmountAsLong) .setTradePrice(tradePrice) .setTradingPeerNodeAddress(tradingPeerNodeAddress) @@ -180,6 +184,7 @@ public static class TradeInfoBuilder { private String takerFeeTxId; private String depositTxId; private String payoutTxId; + private String withdrawalTxId; private long tradeAmountAsLong; private long tradePrice; private String tradingPeerNodeAddress; @@ -249,6 +254,11 @@ public TradeInfoBuilder withPayoutTxId(String payoutTxId) { return this; } + public TradeInfoBuilder withWithdrawalTxId(String withdrawalTxId) { + this.withdrawalTxId = withdrawalTxId; + return this; + } + public TradeInfoBuilder withTradeAmountAsLong(long tradeAmountAsLong) { this.tradeAmountAsLong = tradeAmountAsLong; return this; @@ -332,6 +342,7 @@ public String toString() { ", takerFeeTxId='" + takerFeeTxId + '\'' + "\n" + ", depositTxId='" + depositTxId + '\'' + "\n" + ", payoutTxId='" + payoutTxId + '\'' + "\n" + + ", withdrawalTxId='" + withdrawalTxId + '\'' + "\n" + ", tradeAmountAsLong='" + tradeAmountAsLong + '\'' + "\n" + ", tradePrice='" + tradePrice + '\'' + "\n" + ", tradingPeerNodeAddress='" + tradingPeerNodeAddress + '\'' + "\n" + diff --git a/core/src/main/java/bisq/core/api/model/TxInfo.java b/core/src/main/java/bisq/core/api/model/TxInfo.java index f1b24f6b0fa..16d8f5fc108 100644 --- a/core/src/main/java/bisq/core/api/model/TxInfo.java +++ b/core/src/main/java/bisq/core/api/model/TxInfo.java @@ -28,26 +28,42 @@ @Getter public class TxInfo implements Payload { - private final String id; + // The client cannot see an instance of an org.bitcoinj.core.Transaction. We use the + // lighter weight TxInfo proto wrapper instead, containing just enough fields to + // view some transaction details. A block explorer or bitcoin-core client can be + // used to see more detail. + + private final String txId; + private final long inputSum; private final long outputSum; private final long fee; private final int size; + private final boolean isPending; + private final String memo; - public TxInfo(String id, long outputSum, long fee, int size) { - this.id = id; - this.outputSum = outputSum; - this.fee = fee; - this.size = size; + public TxInfo(TxInfo.TxInfoBuilder builder) { + this.txId = builder.txId; + this.inputSum = builder.inputSum; + this.outputSum = builder.outputSum; + this.fee = builder.fee; + this.size = builder.size; + this.isPending = builder.isPending; + this.memo = builder.memo; } public static TxInfo toTxInfo(Transaction transaction) { if (transaction == null) throw new IllegalStateException("server created a null transaction"); - return new TxInfo(transaction.getTxId().toString(), - transaction.getOutputSum().value, - transaction.getFee().value, - transaction.getMessageSize()); + return new TxInfo.TxInfoBuilder() + .withTxId(transaction.getTxId().toString()) + .withInputSum(transaction.getInputSum().value) + .withOutputSum(transaction.getOutputSum().value) + .withFee(transaction.getFee().value) + .withSize(transaction.getMessageSize()) + .withIsPending(transaction.isPending()) + .withMemo(transaction.getMemo()) + .build(); } ////////////////////////////////////////////////////////////////////////////////////// @@ -57,28 +73,88 @@ public static TxInfo toTxInfo(Transaction transaction) { @Override public bisq.proto.grpc.TxInfo toProtoMessage() { return bisq.proto.grpc.TxInfo.newBuilder() - .setId(id) + .setTxId(txId) + .setInputSum(inputSum) .setOutputSum(outputSum) .setFee(fee) .setSize(size) + .setIsPending(isPending) + .setMemo(memo == null ? "" : memo) .build(); } @SuppressWarnings("unused") public static TxInfo fromProto(bisq.proto.grpc.TxInfo proto) { - return new TxInfo(proto.getId(), - proto.getOutputSum(), - proto.getFee(), - proto.getSize()); + return new TxInfo.TxInfoBuilder() + .withTxId(proto.getTxId()) + .withInputSum(proto.getInputSum()) + .withOutputSum(proto.getOutputSum()) + .withFee(proto.getFee()) + .withSize(proto.getSize()) + .withIsPending(proto.getIsPending()) + .withMemo(proto.getMemo()) + .build(); + } + + public static class TxInfoBuilder { + private String txId; + private long inputSum; + private long outputSum; + private long fee; + private int size; + private boolean isPending; + private String memo; + + public TxInfo.TxInfoBuilder withTxId(String txId) { + this.txId = txId; + return this; + } + + public TxInfo.TxInfoBuilder withInputSum(long inputSum) { + this.inputSum = inputSum; + return this; + } + + public TxInfo.TxInfoBuilder withOutputSum(long outputSum) { + this.outputSum = outputSum; + return this; + } + + public TxInfo.TxInfoBuilder withFee(long fee) { + this.fee = fee; + return this; + } + + public TxInfo.TxInfoBuilder withSize(int size) { + this.size = size; + return this; + } + + public TxInfo.TxInfoBuilder withIsPending(boolean isPending) { + this.isPending = isPending; + return this; + } + + public TxInfo.TxInfoBuilder withMemo(String memo) { + this.memo = memo; + return this; + } + + public TxInfo build() { + return new TxInfo(this); + } } @Override public String toString() { return "TxInfo{" + "\n" + - " id='" + id + '\'' + "\n" + - ", outputSum=" + outputSum + " sats" + "\n" + - ", fee=" + fee + " sats" + "\n" + - ", size=" + size + " bytes" + "\n" + + " txId='" + txId + '\'' + "\n" + + ", inputSum=" + inputSum + "\n" + + ", outputSum=" + outputSum + "\n" + + ", fee=" + fee + "\n" + + ", size=" + size + "\n" + + ", isPending=" + isPending + "\n" + + ", memo='" + memo + '\'' + "\n" + '}'; } } diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java index c7bbb4623da..51b30cb1871 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java @@ -29,6 +29,8 @@ import bisq.proto.grpc.GetBalancesRequest; import bisq.proto.grpc.GetFundingAddressesReply; import bisq.proto.grpc.GetFundingAddressesRequest; +import bisq.proto.grpc.GetTransactionReply; +import bisq.proto.grpc.GetTransactionRequest; import bisq.proto.grpc.GetTxFeeRateReply; import bisq.proto.grpc.GetTxFeeRateRequest; import bisq.proto.grpc.GetUnusedBsqAddressReply; @@ -280,6 +282,23 @@ public void unsetTxFeeRatePreference(UnsetTxFeeRatePreferenceRequest req, } } + @Override + public void getTransaction(GetTransactionRequest req, + StreamObserver responseObserver) { + try { + Transaction tx = coreApi.getTransaction(req.getTxId()); + var reply = GetTransactionReply.newBuilder() + .setTxInfo(toTxInfo(tx).toProtoMessage()) + .build(); + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } catch (IllegalStateException | IllegalArgumentException cause) { + var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); + responseObserver.onError(ex); + throw ex; + } + } + @Override public void setWalletPassword(SetWalletPasswordRequest req, StreamObserver responseObserver) { diff --git a/proto/src/main/proto/grpc.proto b/proto/src/main/proto/grpc.proto index a6e30cdb473..81795112e55 100644 --- a/proto/src/main/proto/grpc.proto +++ b/proto/src/main/proto/grpc.proto @@ -273,19 +273,20 @@ message TradeInfo { string takerFeeTxId = 9; string depositTxId = 10; string payoutTxId = 11; - uint64 tradeAmountAsLong = 12; - uint64 tradePrice = 13; - string tradingPeerNodeAddress = 14; - string state = 15; - string phase = 16; - string tradePeriodState = 17; - bool isDepositPublished = 18; - bool isDepositConfirmed = 19; - bool isFiatSent = 20; - bool isFiatReceived = 21; - bool isPayoutPublished = 22; - bool isWithdrawn = 23; - string contractAsJson = 24; + string withdrawalTxId = 12; + uint64 tradeAmountAsLong = 13; + uint64 tradePrice = 14; + string tradingPeerNodeAddress = 15; + string state = 16; + string phase = 17; + string tradePeriodState = 18; + bool isDepositPublished = 19; + bool isDepositConfirmed = 20; + bool isFiatSent = 21; + bool isFiatReceived = 22; + bool isPayoutPublished = 23; + bool isWithdrawn = 24; + string contractAsJson = 25; } /////////////////////////////////////////////////////////////////////////////////////////// @@ -300,10 +301,13 @@ message TxFeeRateInfo { } message TxInfo { - string id = 1; - uint64 outputSum = 2; - uint64 fee = 3; - int32 size = 4; + string txId = 1; + uint64 inputSum = 2; + uint64 outputSum = 3; + uint64 fee = 4; + int32 size = 5; + bool isPending = 6; + string memo = 7; } /////////////////////////////////////////////////////////////////////////////////////////// @@ -325,7 +329,9 @@ service Wallets { } rpc SetTxFeeRatePreference (SetTxFeeRatePreferenceRequest) returns (SetTxFeeRatePreferenceReply) { } - rpc unsetTxFeeRatePreference (UnsetTxFeeRatePreferenceRequest) returns (UnsetTxFeeRatePreferenceReply) { + rpc UnsetTxFeeRatePreference (UnsetTxFeeRatePreferenceRequest) returns (UnsetTxFeeRatePreferenceReply) { + } + rpc GetTransaction (GetTransactionRequest) returns (GetTransactionReply) { } rpc GetFundingAddresses (GetFundingAddressesRequest) returns (GetFundingAddressesReply) { } @@ -405,6 +411,14 @@ message UnsetTxFeeRatePreferenceReply { TxFeeRateInfo txFeeRateInfo = 1; } +message GetTransactionRequest { + string txId = 1; +} + +message GetTransactionReply { + TxInfo txInfo = 1; +} + message GetFundingAddressesRequest { } From 0384642f32c8ffbdb8ec68c93cbc067e1de73008 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Mon, 14 Dec 2020 15:03:35 -0300 Subject: [PATCH 09/45] Adjust create TransferwiseAccount test As per commit 88f26f93241af698ae689bf081205d0f9dc929fa, do not autofill all currencies by default but keep all unselected. --- .../bisq/apitest/method/payment/CreatePaymentAccountTest.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/apitest/src/test/java/bisq/apitest/method/payment/CreatePaymentAccountTest.java b/apitest/src/test/java/bisq/apitest/method/payment/CreatePaymentAccountTest.java index ac66cc7993e..2fe5ed22abd 100644 --- a/apitest/src/test/java/bisq/apitest/method/payment/CreatePaymentAccountTest.java +++ b/apitest/src/test/java/bisq/apitest/method/payment/CreatePaymentAccountTest.java @@ -750,8 +750,7 @@ public void testCreateTransferwiseAccount(TestInfo testInfo) { // As per commit 88f26f93241af698ae689bf081205d0f9dc929fa // Do not autofill all currencies by default but keep all unselected. // verifyAccountTradeCurrencies(getAllTransferwiseCurrencies(), paymentAccount); - // TODO uncomment after master/merge - // assertEquals(0, paymentAccount.getTradeCurrencies().size()); + assertEquals(0, paymentAccount.getTradeCurrencies().size()); verifyCommonFormEntries(paymentAccount); assertEquals(COMPLETED_FORM_MAP.get(PROPERTY_NAME_EMAIL), paymentAccount.getEmail()); log.debug("Deserialized {}: {}", paymentAccount.getClass().getSimpleName(), paymentAccount); From 4be87a6281961242dcb9689db869bc4641840b40 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Mon, 14 Dec 2020 15:12:39 -0300 Subject: [PATCH 10/45] Disable method test to avoid repetition --- .../java/bisq/apitest/method/trade/TakeSellBTCOfferTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apitest/src/test/java/bisq/apitest/method/trade/TakeSellBTCOfferTest.java b/apitest/src/test/java/bisq/apitest/method/trade/TakeSellBTCOfferTest.java index ce5bceff7a1..cd500cdd091 100644 --- a/apitest/src/test/java/bisq/apitest/method/trade/TakeSellBTCOfferTest.java +++ b/apitest/src/test/java/bisq/apitest/method/trade/TakeSellBTCOfferTest.java @@ -23,6 +23,7 @@ import lombok.extern.slf4j.Slf4j; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.MethodOrderer; import org.junit.jupiter.api.Order; import org.junit.jupiter.api.Test; @@ -42,7 +43,7 @@ import static protobuf.Offer.State.OFFER_FEE_PAID; import static protobuf.OpenOffer.State.AVAILABLE; -// @Disabled +@Disabled @Slf4j @TestMethodOrder(MethodOrderer.OrderAnnotation.class) public class TakeSellBTCOfferTest extends AbstractTradeTest { From e6c6d3b8d3a5095690ea498980b71d19b7eb9252 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Wed, 16 Dec 2020 13:34:21 -0300 Subject: [PATCH 11/45] Add new CoreApiExceptionHandler to gRPC services This change reduces gRPC service error handling duplication by moving it into a @Singleton encapsulating everything needed to wrap an expected or unexpected core api exception into a gRPC StatusRuntimeException before sending it to the client. It also fixes some boilerpate classes were gRPC error handling was missing. --- .../daemon/grpc/CoreApiExceptionHandler.java | 93 +++++++++++++++++++ .../daemon/grpc/GrpcDisputeAgentsService.java | 16 +--- .../grpc/GrpcGetTradeStatisticsService.java | 23 +++-- .../bisq/daemon/grpc/GrpcOffersService.java | 48 +++++----- .../grpc/GrpcPaymentAccountsService.java | 46 +++------ .../bisq/daemon/grpc/GrpcPriceService.java | 12 +-- .../bisq/daemon/grpc/GrpcTradesService.java | 42 +++------ .../bisq/daemon/grpc/GrpcVersionService.java | 14 ++- .../bisq/daemon/grpc/GrpcWalletsService.java | 90 +++++++----------- 9 files changed, 207 insertions(+), 177 deletions(-) create mode 100644 daemon/src/main/java/bisq/daemon/grpc/CoreApiExceptionHandler.java diff --git a/daemon/src/main/java/bisq/daemon/grpc/CoreApiExceptionHandler.java b/daemon/src/main/java/bisq/daemon/grpc/CoreApiExceptionHandler.java new file mode 100644 index 00000000000..03cde748abd --- /dev/null +++ b/daemon/src/main/java/bisq/daemon/grpc/CoreApiExceptionHandler.java @@ -0,0 +1,93 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + +package bisq.daemon.grpc; + +import io.grpc.Status; +import io.grpc.StatusRuntimeException; +import io.grpc.stub.StreamObserver; + +import javax.inject.Inject; +import javax.inject.Singleton; + +import java.util.function.Predicate; + +import lombok.extern.slf4j.Slf4j; + +import static io.grpc.Status.INVALID_ARGUMENT; +import static io.grpc.Status.UNKNOWN; + +/** + * The singleton instance of this class wraps a RuntimeException from the core api + * in a gRPC StatusRuntimeException before putting it in the server's response, + * then throwing it. + */ +@Singleton +@Slf4j +class CoreApiExceptionHandler { + + private final Predicate isExpectedException = (t) -> + t instanceof IllegalStateException || t instanceof IllegalArgumentException; + + @Inject + public CoreApiExceptionHandler() { + } + + public void handleException(Throwable t, StreamObserver responseObserver) { + // Log the core api error (this is last chance to do that), wrap it in a new + // gRPC StatusRuntimeException, then send it to the client in the gRPC response. + log.error("", t); + var grpcStatusRuntimeException = wrapException(t); + responseObserver.onError(grpcStatusRuntimeException); + throw grpcStatusRuntimeException; + } + + private StatusRuntimeException wrapException(Throwable t) { + // We want to be careful about what kinds of exception messages we send to the + // client. Expected core exceptions should be wrapped in an IllegalStateException + // or IllegalArgumentException, with a consistently styled and worded error + // message. But only a small number of the expected error types are currently + // handled this way; there is much work to do to handle the variety of errors + // that can occur in the api. In the meantime, we take care to not pass full, + // unexpected error messages to the client. If the exception type is unexpected, + // we omit details from the gRPC exception sent to the client. + if (isExpectedException.test(t)) { + if (t.getCause() != null) + return new StatusRuntimeException(mapGrpcErrorStatus(t.getCause(), t.getCause().getMessage())); + else + return new StatusRuntimeException(mapGrpcErrorStatus(t, t.getMessage())); + } else { + return new StatusRuntimeException(mapGrpcErrorStatus(t, "unexpected error on server")); + } + } + + private Status mapGrpcErrorStatus(Throwable t, String description) { + // We default to the UNKNOWN status, except were the mapping of a core api + // exception to a gRPC Status is obvious. If we ever use a gRPC reverse-proxy + // to support RESTful clients, we will need to have more specific mappings + // to support correct HTTP 1.1. status codes. + //noinspection SwitchStatementWithTooFewBranches + switch (t.getClass().getSimpleName()) { + // We go ahead and use a switch statement instead of if, in anticipation + // of more, specific exception mappings. + case "IllegalArgumentException": + return INVALID_ARGUMENT.withDescription(description); + default: + return UNKNOWN.withDescription(description); + } + } +} diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcDisputeAgentsService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcDisputeAgentsService.java index 24fd192fea8..1b5268bb81d 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcDisputeAgentsService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcDisputeAgentsService.java @@ -6,8 +6,6 @@ import bisq.proto.grpc.RegisterDisputeAgentReply; import bisq.proto.grpc.RegisterDisputeAgentRequest; -import io.grpc.Status; -import io.grpc.StatusRuntimeException; import io.grpc.stub.StreamObserver; import javax.inject.Inject; @@ -18,10 +16,12 @@ class GrpcDisputeAgentsService extends DisputeAgentsGrpc.DisputeAgentsImplBase { private final CoreApi coreApi; + private final CoreApiExceptionHandler exceptionHandler; @Inject - public GrpcDisputeAgentsService(CoreApi coreApi) { + public GrpcDisputeAgentsService(CoreApi coreApi, CoreApiExceptionHandler exceptionHandler) { this.coreApi = coreApi; + this.exceptionHandler = exceptionHandler; } @Override @@ -32,14 +32,8 @@ public void registerDisputeAgent(RegisterDisputeAgentRequest req, var reply = RegisterDisputeAgentReply.newBuilder().build(); responseObserver.onNext(reply); responseObserver.onCompleted(); - } catch (IllegalArgumentException cause) { - var ex = new StatusRuntimeException(Status.INVALID_ARGUMENT.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; - } catch (IllegalStateException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } } diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcGetTradeStatisticsService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcGetTradeStatisticsService.java index 4c98e939af3..d50d7eb477a 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcGetTradeStatisticsService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcGetTradeStatisticsService.java @@ -16,22 +16,27 @@ class GrpcGetTradeStatisticsService extends GetTradeStatisticsGrpc.GetTradeStatisticsImplBase { private final CoreApi coreApi; + private final CoreApiExceptionHandler exceptionHandler; @Inject - public GrpcGetTradeStatisticsService(CoreApi coreApi) { + public GrpcGetTradeStatisticsService(CoreApi coreApi, CoreApiExceptionHandler exceptionHandler) { this.coreApi = coreApi; + this.exceptionHandler = exceptionHandler; } @Override public void getTradeStatistics(GetTradeStatisticsRequest req, StreamObserver responseObserver) { - - var tradeStatistics = coreApi.getTradeStatistics().stream() - .map(TradeStatistics3::toProtoTradeStatistics3) - .collect(Collectors.toList()); - - var reply = GetTradeStatisticsReply.newBuilder().addAllTradeStatistics(tradeStatistics).build(); - responseObserver.onNext(reply); - responseObserver.onCompleted(); + try { + var tradeStatistics = coreApi.getTradeStatistics().stream() + .map(TradeStatistics3::toProtoTradeStatistics3) + .collect(Collectors.toList()); + + var reply = GetTradeStatisticsReply.newBuilder().addAllTradeStatistics(tradeStatistics).build(); + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); + } } } diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcOffersService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcOffersService.java index 1efed108ca6..2bc99ee4039 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcOffersService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcOffersService.java @@ -31,8 +31,6 @@ import bisq.proto.grpc.GetOffersRequest; import bisq.proto.grpc.OffersGrpc; -import io.grpc.Status; -import io.grpc.StatusRuntimeException; import io.grpc.stub.StreamObserver; import javax.inject.Inject; @@ -48,10 +46,12 @@ class GrpcOffersService extends OffersGrpc.OffersImplBase { private final CoreApi coreApi; + private final CoreApiExceptionHandler exceptionHandler; @Inject - public GrpcOffersService(CoreApi coreApi) { + public GrpcOffersService(CoreApi coreApi, CoreApiExceptionHandler exceptionHandler) { this.coreApi = coreApi; + this.exceptionHandler = exceptionHandler; } @Override @@ -64,26 +64,28 @@ public void getOffer(GetOfferRequest req, .build(); responseObserver.onNext(reply); responseObserver.onCompleted(); - } catch (IllegalStateException | IllegalArgumentException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @Override public void getOffers(GetOffersRequest req, StreamObserver responseObserver) { - List result = coreApi.getOffers(req.getDirection(), req.getCurrencyCode()) - .stream().map(OfferInfo::toOfferInfo) - .collect(Collectors.toList()); - var reply = GetOffersReply.newBuilder() - .addAllOffers(result.stream() - .map(OfferInfo::toProtoMessage) - .collect(Collectors.toList())) - .build(); - responseObserver.onNext(reply); - responseObserver.onCompleted(); + try { + List result = coreApi.getOffers(req.getDirection(), req.getCurrencyCode()) + .stream().map(OfferInfo::toOfferInfo) + .collect(Collectors.toList()); + var reply = GetOffersReply.newBuilder() + .addAllOffers(result.stream() + .map(OfferInfo::toProtoMessage) + .collect(Collectors.toList())) + .build(); + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); + } } @Override @@ -111,10 +113,8 @@ public void createOffer(CreateOfferRequest req, responseObserver.onNext(reply); responseObserver.onCompleted(); }); - } catch (IllegalStateException | IllegalArgumentException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @@ -126,10 +126,8 @@ public void cancelOffer(CancelOfferRequest req, var reply = CancelOfferReply.newBuilder().build(); responseObserver.onNext(reply); responseObserver.onCompleted(); - } catch (IllegalStateException | IllegalArgumentException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } } diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcPaymentAccountsService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcPaymentAccountsService.java index 0438d33655d..864ecc55e16 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcPaymentAccountsService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcPaymentAccountsService.java @@ -31,8 +31,6 @@ import bisq.proto.grpc.GetPaymentMethodsRequest; import bisq.proto.grpc.PaymentAccountsGrpc; -import io.grpc.Status; -import io.grpc.StatusRuntimeException; import io.grpc.stub.StreamObserver; import javax.inject.Inject; @@ -43,10 +41,12 @@ class GrpcPaymentAccountsService extends PaymentAccountsGrpc.PaymentAccountsImplBase { private final CoreApi coreApi; + private final CoreApiExceptionHandler exceptionHandler; @Inject - public GrpcPaymentAccountsService(CoreApi coreApi) { + public GrpcPaymentAccountsService(CoreApi coreApi, CoreApiExceptionHandler exceptionHandler) { this.coreApi = coreApi; + this.exceptionHandler = exceptionHandler; } @Override @@ -59,14 +59,8 @@ public void createPaymentAccount(CreatePaymentAccountRequest req, .build(); responseObserver.onNext(reply); responseObserver.onCompleted(); - } catch (IllegalArgumentException cause) { - var ex = new StatusRuntimeException(Status.INVALID_ARGUMENT.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; - } catch (IllegalStateException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @@ -81,14 +75,8 @@ public void getPaymentAccounts(GetPaymentAccountsRequest req, .addAllPaymentAccounts(paymentAccounts).build(); responseObserver.onNext(reply); responseObserver.onCompleted(); - } catch (IllegalArgumentException cause) { - var ex = new StatusRuntimeException(Status.INVALID_ARGUMENT.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; - } catch (IllegalStateException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @@ -103,14 +91,8 @@ public void getPaymentMethods(GetPaymentMethodsRequest req, .addAllPaymentMethods(paymentMethods).build(); responseObserver.onNext(reply); responseObserver.onCompleted(); - } catch (IllegalArgumentException cause) { - var ex = new StatusRuntimeException(Status.INVALID_ARGUMENT.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; - } catch (IllegalStateException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @@ -124,14 +106,8 @@ public void getPaymentAccountForm(GetPaymentAccountFormRequest req, .build(); responseObserver.onNext(reply); responseObserver.onCompleted(); - } catch (IllegalArgumentException cause) { - var ex = new StatusRuntimeException(Status.INVALID_ARGUMENT.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; - } catch (IllegalStateException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } } diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcPriceService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcPriceService.java index 11410140fdc..5687aa554c3 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcPriceService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcPriceService.java @@ -23,8 +23,6 @@ import bisq.proto.grpc.MarketPriceRequest; import bisq.proto.grpc.PriceGrpc; -import io.grpc.Status; -import io.grpc.StatusRuntimeException; import io.grpc.stub.StreamObserver; import javax.inject.Inject; @@ -35,10 +33,12 @@ class GrpcPriceService extends PriceGrpc.PriceImplBase { private final CoreApi coreApi; + private final CoreApiExceptionHandler exceptionHandler; @Inject - public GrpcPriceService(CoreApi coreApi) { + public GrpcPriceService(CoreApi coreApi, CoreApiExceptionHandler exceptionHandler) { this.coreApi = coreApi; + this.exceptionHandler = exceptionHandler; } @Override @@ -49,10 +49,8 @@ public void getMarketPrice(MarketPriceRequest req, var reply = MarketPriceReply.newBuilder().setPrice(price).build(); responseObserver.onNext(reply); responseObserver.onCompleted(); - } catch (IllegalStateException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } } diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcTradesService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcTradesService.java index 1028c0cf9d7..3bf2b183cdd 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcTradesService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcTradesService.java @@ -35,8 +35,6 @@ import bisq.proto.grpc.WithdrawFundsReply; import bisq.proto.grpc.WithdrawFundsRequest; -import io.grpc.Status; -import io.grpc.StatusRuntimeException; import io.grpc.stub.StreamObserver; import javax.inject.Inject; @@ -49,10 +47,12 @@ class GrpcTradesService extends TradesGrpc.TradesImplBase { private final CoreApi coreApi; + private final CoreApiExceptionHandler exceptionHandler; @Inject - public GrpcTradesService(CoreApi coreApi) { + public GrpcTradesService(CoreApi coreApi, CoreApiExceptionHandler exceptionHandler) { this.coreApi = coreApi; + this.exceptionHandler = exceptionHandler; } @Override @@ -66,10 +66,8 @@ public void getTrade(GetTradeRequest req, .build(); responseObserver.onNext(reply); responseObserver.onCompleted(); - } catch (IllegalStateException | IllegalArgumentException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @@ -88,10 +86,8 @@ public void takeOffer(TakeOfferRequest req, responseObserver.onNext(reply); responseObserver.onCompleted(); }); - } catch (IllegalStateException | IllegalArgumentException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @@ -103,10 +99,8 @@ public void confirmPaymentStarted(ConfirmPaymentStartedRequest req, var reply = ConfirmPaymentStartedReply.newBuilder().build(); responseObserver.onNext(reply); responseObserver.onCompleted(); - } catch (IllegalStateException | IllegalArgumentException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @@ -118,10 +112,8 @@ public void confirmPaymentReceived(ConfirmPaymentReceivedRequest req, var reply = ConfirmPaymentReceivedReply.newBuilder().build(); responseObserver.onNext(reply); responseObserver.onCompleted(); - } catch (IllegalStateException | IllegalArgumentException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @@ -133,10 +125,8 @@ public void keepFunds(KeepFundsRequest req, var reply = KeepFundsReply.newBuilder().build(); responseObserver.onNext(reply); responseObserver.onCompleted(); - } catch (IllegalStateException | IllegalArgumentException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @@ -148,10 +138,8 @@ public void withdrawFunds(WithdrawFundsRequest req, var reply = WithdrawFundsReply.newBuilder().build(); responseObserver.onNext(reply); responseObserver.onCompleted(); - } catch (IllegalStateException | IllegalArgumentException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } } diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcVersionService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcVersionService.java index 658ef10e27f..6414a6ceb44 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcVersionService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcVersionService.java @@ -13,16 +13,22 @@ class GrpcVersionService extends GetVersionGrpc.GetVersionImplBase { private final CoreApi coreApi; + private final CoreApiExceptionHandler exceptionHandler; @Inject - public GrpcVersionService(CoreApi coreApi) { + public GrpcVersionService(CoreApi coreApi, CoreApiExceptionHandler exceptionHandler) { this.coreApi = coreApi; + this.exceptionHandler = exceptionHandler; } @Override public void getVersion(GetVersionRequest req, StreamObserver responseObserver) { - var reply = GetVersionReply.newBuilder().setVersion(coreApi.getVersion()).build(); - responseObserver.onNext(reply); - responseObserver.onCompleted(); + try { + var reply = GetVersionReply.newBuilder().setVersion(coreApi.getVersion()).build(); + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); + } } } diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java index 51b30cb1871..5cf74d38dab 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java @@ -53,8 +53,6 @@ import bisq.proto.grpc.UnsetTxFeeRatePreferenceRequest; import bisq.proto.grpc.WalletsGrpc; -import io.grpc.Status; -import io.grpc.StatusRuntimeException; import io.grpc.stub.StreamObserver; import org.bitcoinj.core.Transaction; @@ -76,10 +74,12 @@ class GrpcWalletsService extends WalletsGrpc.WalletsImplBase { private final CoreApi coreApi; + private final CoreApiExceptionHandler exceptionHandler; @Inject - public GrpcWalletsService(CoreApi coreApi) { + public GrpcWalletsService(CoreApi coreApi, CoreApiExceptionHandler exceptionHandler) { this.coreApi = coreApi; + this.exceptionHandler = exceptionHandler; } @Override @@ -91,10 +91,8 @@ public void getBalances(GetBalancesRequest req, StreamObserver .build(); responseObserver.onNext(reply); responseObserver.onCompleted(); - } catch (IllegalStateException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @@ -107,10 +105,8 @@ public void getAddressBalance(GetAddressBalanceRequest req, .setAddressBalanceInfo(balanceInfo.toProtoMessage()).build(); responseObserver.onNext(reply); responseObserver.onCompleted(); - } catch (IllegalStateException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @@ -127,10 +123,8 @@ public void getFundingAddresses(GetFundingAddressesRequest req, .build(); responseObserver.onNext(reply); responseObserver.onCompleted(); - } catch (IllegalStateException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @@ -144,10 +138,8 @@ public void getUnusedBsqAddress(GetUnusedBsqAddressRequest req, .build(); responseObserver.onNext(reply); responseObserver.onCompleted(); - } catch (IllegalStateException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @@ -178,10 +170,8 @@ public void onFailure(TxBroadcastException ex) { throw new IllegalStateException(ex); } }); - } catch (IllegalStateException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @@ -218,10 +208,8 @@ public void onFailure(@NotNull Throwable t) { throw new IllegalStateException(t); } }); - } catch (IllegalStateException | IllegalArgumentException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @@ -237,10 +225,8 @@ public void getTxFeeRate(GetTxFeeRateRequest req, responseObserver.onNext(reply); responseObserver.onCompleted(); }); - } catch (IllegalStateException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @@ -256,10 +242,8 @@ public void setTxFeeRatePreference(SetTxFeeRatePreferenceRequest req, responseObserver.onNext(reply); responseObserver.onCompleted(); }); - } catch (IllegalStateException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @@ -275,10 +259,8 @@ public void unsetTxFeeRatePreference(UnsetTxFeeRatePreferenceRequest req, responseObserver.onNext(reply); responseObserver.onCompleted(); }); - } catch (IllegalStateException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @@ -292,10 +274,8 @@ public void getTransaction(GetTransactionRequest req, .build(); responseObserver.onNext(reply); responseObserver.onCompleted(); - } catch (IllegalStateException | IllegalArgumentException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @@ -307,10 +287,8 @@ public void setWalletPassword(SetWalletPasswordRequest req, var reply = SetWalletPasswordReply.newBuilder().build(); responseObserver.onNext(reply); responseObserver.onCompleted(); - } catch (IllegalStateException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @@ -322,10 +300,8 @@ public void removeWalletPassword(RemoveWalletPasswordRequest req, var reply = RemoveWalletPasswordReply.newBuilder().build(); responseObserver.onNext(reply); responseObserver.onCompleted(); - } catch (IllegalStateException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @@ -337,10 +313,8 @@ public void lockWallet(LockWalletRequest req, var reply = LockWalletReply.newBuilder().build(); responseObserver.onNext(reply); responseObserver.onCompleted(); - } catch (IllegalStateException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } @@ -352,10 +326,8 @@ public void unlockWallet(UnlockWalletRequest req, var reply = UnlockWalletReply.newBuilder().build(); responseObserver.onNext(reply); responseObserver.onCompleted(); - } catch (IllegalStateException cause) { - var ex = new StatusRuntimeException(Status.UNKNOWN.withDescription(cause.getMessage())); - responseObserver.onError(ex); - throw ex; + } catch (Throwable cause) { + exceptionHandler.handleException(cause, responseObserver); } } } From c60605f75c98e2632c2c80e30f5a824b9eb1c0ad Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Wed, 16 Dec 2020 14:26:12 -0300 Subject: [PATCH 12/45] Fix class level comment --- .../main/java/bisq/daemon/grpc/CoreApiExceptionHandler.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/daemon/src/main/java/bisq/daemon/grpc/CoreApiExceptionHandler.java b/daemon/src/main/java/bisq/daemon/grpc/CoreApiExceptionHandler.java index 03cde748abd..8f9a54a40b3 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/CoreApiExceptionHandler.java +++ b/daemon/src/main/java/bisq/daemon/grpc/CoreApiExceptionHandler.java @@ -32,9 +32,9 @@ import static io.grpc.Status.UNKNOWN; /** - * The singleton instance of this class wraps a RuntimeException from the core api - * in a gRPC StatusRuntimeException before putting it in the server's response, - * then throwing it. + * The singleton instance of this class handles any expected core api Throwable by + * wrapping its message in a gRPC StatusRuntimeException and sending it to the client. + * An unexpected Throwable's message will be replaced with an 'unexpected' error message. */ @Singleton @Slf4j From f7c1103848f33d1fada78a030c221f0f5fb4b01a Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Wed, 16 Dec 2020 14:42:23 -0300 Subject: [PATCH 13/45] Rename gRPC exception handler class --- .../main/java/bisq/daemon/grpc/GrpcDisputeAgentsService.java | 4 ++-- ...CoreApiExceptionHandler.java => GrpcExceptionHandler.java} | 4 ++-- .../java/bisq/daemon/grpc/GrpcGetTradeStatisticsService.java | 4 ++-- daemon/src/main/java/bisq/daemon/grpc/GrpcOffersService.java | 4 ++-- .../java/bisq/daemon/grpc/GrpcPaymentAccountsService.java | 4 ++-- daemon/src/main/java/bisq/daemon/grpc/GrpcPriceService.java | 4 ++-- daemon/src/main/java/bisq/daemon/grpc/GrpcTradesService.java | 4 ++-- daemon/src/main/java/bisq/daemon/grpc/GrpcVersionService.java | 4 ++-- daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java | 4 ++-- 9 files changed, 18 insertions(+), 18 deletions(-) rename daemon/src/main/java/bisq/daemon/grpc/{CoreApiExceptionHandler.java => GrpcExceptionHandler.java} (98%) diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcDisputeAgentsService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcDisputeAgentsService.java index 1b5268bb81d..cd336da436a 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcDisputeAgentsService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcDisputeAgentsService.java @@ -16,10 +16,10 @@ class GrpcDisputeAgentsService extends DisputeAgentsGrpc.DisputeAgentsImplBase { private final CoreApi coreApi; - private final CoreApiExceptionHandler exceptionHandler; + private final GrpcExceptionHandler exceptionHandler; @Inject - public GrpcDisputeAgentsService(CoreApi coreApi, CoreApiExceptionHandler exceptionHandler) { + public GrpcDisputeAgentsService(CoreApi coreApi, GrpcExceptionHandler exceptionHandler) { this.coreApi = coreApi; this.exceptionHandler = exceptionHandler; } diff --git a/daemon/src/main/java/bisq/daemon/grpc/CoreApiExceptionHandler.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcExceptionHandler.java similarity index 98% rename from daemon/src/main/java/bisq/daemon/grpc/CoreApiExceptionHandler.java rename to daemon/src/main/java/bisq/daemon/grpc/GrpcExceptionHandler.java index 8f9a54a40b3..8a1c4c2836e 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/CoreApiExceptionHandler.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcExceptionHandler.java @@ -38,13 +38,13 @@ */ @Singleton @Slf4j -class CoreApiExceptionHandler { +class GrpcExceptionHandler { private final Predicate isExpectedException = (t) -> t instanceof IllegalStateException || t instanceof IllegalArgumentException; @Inject - public CoreApiExceptionHandler() { + public GrpcExceptionHandler() { } public void handleException(Throwable t, StreamObserver responseObserver) { diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcGetTradeStatisticsService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcGetTradeStatisticsService.java index d50d7eb477a..18cd5ef1c7c 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcGetTradeStatisticsService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcGetTradeStatisticsService.java @@ -16,10 +16,10 @@ class GrpcGetTradeStatisticsService extends GetTradeStatisticsGrpc.GetTradeStatisticsImplBase { private final CoreApi coreApi; - private final CoreApiExceptionHandler exceptionHandler; + private final GrpcExceptionHandler exceptionHandler; @Inject - public GrpcGetTradeStatisticsService(CoreApi coreApi, CoreApiExceptionHandler exceptionHandler) { + public GrpcGetTradeStatisticsService(CoreApi coreApi, GrpcExceptionHandler exceptionHandler) { this.coreApi = coreApi; this.exceptionHandler = exceptionHandler; } diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcOffersService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcOffersService.java index 2bc99ee4039..9b6690864f2 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcOffersService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcOffersService.java @@ -46,10 +46,10 @@ class GrpcOffersService extends OffersGrpc.OffersImplBase { private final CoreApi coreApi; - private final CoreApiExceptionHandler exceptionHandler; + private final GrpcExceptionHandler exceptionHandler; @Inject - public GrpcOffersService(CoreApi coreApi, CoreApiExceptionHandler exceptionHandler) { + public GrpcOffersService(CoreApi coreApi, GrpcExceptionHandler exceptionHandler) { this.coreApi = coreApi; this.exceptionHandler = exceptionHandler; } diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcPaymentAccountsService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcPaymentAccountsService.java index 864ecc55e16..4decb5b1bae 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcPaymentAccountsService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcPaymentAccountsService.java @@ -41,10 +41,10 @@ class GrpcPaymentAccountsService extends PaymentAccountsGrpc.PaymentAccountsImplBase { private final CoreApi coreApi; - private final CoreApiExceptionHandler exceptionHandler; + private final GrpcExceptionHandler exceptionHandler; @Inject - public GrpcPaymentAccountsService(CoreApi coreApi, CoreApiExceptionHandler exceptionHandler) { + public GrpcPaymentAccountsService(CoreApi coreApi, GrpcExceptionHandler exceptionHandler) { this.coreApi = coreApi; this.exceptionHandler = exceptionHandler; } diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcPriceService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcPriceService.java index 5687aa554c3..c8d20fcf29b 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcPriceService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcPriceService.java @@ -33,10 +33,10 @@ class GrpcPriceService extends PriceGrpc.PriceImplBase { private final CoreApi coreApi; - private final CoreApiExceptionHandler exceptionHandler; + private final GrpcExceptionHandler exceptionHandler; @Inject - public GrpcPriceService(CoreApi coreApi, CoreApiExceptionHandler exceptionHandler) { + public GrpcPriceService(CoreApi coreApi, GrpcExceptionHandler exceptionHandler) { this.coreApi = coreApi; this.exceptionHandler = exceptionHandler; } diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcTradesService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcTradesService.java index 3bf2b183cdd..b862e04ac95 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcTradesService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcTradesService.java @@ -47,10 +47,10 @@ class GrpcTradesService extends TradesGrpc.TradesImplBase { private final CoreApi coreApi; - private final CoreApiExceptionHandler exceptionHandler; + private final GrpcExceptionHandler exceptionHandler; @Inject - public GrpcTradesService(CoreApi coreApi, CoreApiExceptionHandler exceptionHandler) { + public GrpcTradesService(CoreApi coreApi, GrpcExceptionHandler exceptionHandler) { this.coreApi = coreApi; this.exceptionHandler = exceptionHandler; } diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcVersionService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcVersionService.java index 6414a6ceb44..48c4d8a6b12 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcVersionService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcVersionService.java @@ -13,10 +13,10 @@ class GrpcVersionService extends GetVersionGrpc.GetVersionImplBase { private final CoreApi coreApi; - private final CoreApiExceptionHandler exceptionHandler; + private final GrpcExceptionHandler exceptionHandler; @Inject - public GrpcVersionService(CoreApi coreApi, CoreApiExceptionHandler exceptionHandler) { + public GrpcVersionService(CoreApi coreApi, GrpcExceptionHandler exceptionHandler) { this.coreApi = coreApi; this.exceptionHandler = exceptionHandler; } diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java index 5cf74d38dab..f8b06758070 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java @@ -74,10 +74,10 @@ class GrpcWalletsService extends WalletsGrpc.WalletsImplBase { private final CoreApi coreApi; - private final CoreApiExceptionHandler exceptionHandler; + private final GrpcExceptionHandler exceptionHandler; @Inject - public GrpcWalletsService(CoreApi coreApi, CoreApiExceptionHandler exceptionHandler) { + public GrpcWalletsService(CoreApi coreApi, GrpcExceptionHandler exceptionHandler) { this.coreApi = coreApi; this.exceptionHandler = exceptionHandler; } From 2572e8641dfd252845df1665c6a2a8c37d4dbe45 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Wed, 16 Dec 2020 15:30:40 -0300 Subject: [PATCH 14/45] Create grpc interceptor pkg, move auth interceptor into it --- daemon/src/main/java/bisq/daemon/grpc/GrpcServer.java | 4 ++++ .../grpc/{ => interceptor}/PasswordAuthInterceptor.java | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) rename daemon/src/main/java/bisq/daemon/grpc/{ => interceptor}/PasswordAuthInterceptor.java (95%) diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcServer.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcServer.java index 40efdd07ac2..24fc267573b 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcServer.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcServer.java @@ -31,6 +31,10 @@ import lombok.extern.slf4j.Slf4j; + + +import bisq.daemon.grpc.interceptor.PasswordAuthInterceptor; + @Singleton @Slf4j public class GrpcServer { diff --git a/daemon/src/main/java/bisq/daemon/grpc/PasswordAuthInterceptor.java b/daemon/src/main/java/bisq/daemon/grpc/interceptor/PasswordAuthInterceptor.java similarity index 95% rename from daemon/src/main/java/bisq/daemon/grpc/PasswordAuthInterceptor.java rename to daemon/src/main/java/bisq/daemon/grpc/interceptor/PasswordAuthInterceptor.java index 1bf542d7fe7..df1b544b6a0 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/PasswordAuthInterceptor.java +++ b/daemon/src/main/java/bisq/daemon/grpc/interceptor/PasswordAuthInterceptor.java @@ -15,7 +15,7 @@ * along with Bisq. If not, see . */ -package bisq.daemon.grpc; +package bisq.daemon.grpc.interceptor; import bisq.common.config.Config; @@ -38,7 +38,7 @@ * * @see bisq.common.config.Config#apiPassword */ -class PasswordAuthInterceptor implements ServerInterceptor { +public class PasswordAuthInterceptor implements ServerInterceptor { private static final String PASSWORD_KEY = "password"; From fa9ffa1fb2c10d40f9107cff84bac9c40e982800 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Wed, 16 Dec 2020 15:35:12 -0300 Subject: [PATCH 15/45] Put arguments on separate lines --- .../bisq/daemon/grpc/interceptor/PasswordAuthInterceptor.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/daemon/src/main/java/bisq/daemon/grpc/interceptor/PasswordAuthInterceptor.java b/daemon/src/main/java/bisq/daemon/grpc/interceptor/PasswordAuthInterceptor.java index df1b544b6a0..4ed1af4f3db 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/interceptor/PasswordAuthInterceptor.java +++ b/daemon/src/main/java/bisq/daemon/grpc/interceptor/PasswordAuthInterceptor.java @@ -50,7 +50,8 @@ public PasswordAuthInterceptor(Config config) { } @Override - public ServerCall.Listener interceptCall(ServerCall serverCall, Metadata headers, + public ServerCall.Listener interceptCall(ServerCall serverCall, + Metadata headers, ServerCallHandler serverCallHandler) { var actualPasswordValue = headers.get(Key.of(PASSWORD_KEY, ASCII_STRING_MARSHALLER)); From 2148a4d9587f5da8e2d965182c5b4a86c6bc34fa Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Thu, 17 Dec 2020 12:33:45 -0300 Subject: [PATCH 16/45] Prevent excessive api calls This change provides a gRPC CallRateMeteringInterceptor to help protect the server and network against being overloaded by CLI scripting mistakes. An interceptor instance can be configured on a gRPC service to set individual method call rate limits on one or more of the the service's methods. For example, the GrpcOffersService could be configured with this interceptor to set the createoffer rate limit to 5/hour, and the takeoffer call rate limit could be set to 20/day. Whenever a call rate limit is exceeded, the gRPC call is aborted and the client recieves a "rate limit exceeded" error. Below is a simple example showing how to set rate limits for one method in GrpcVersionService. final ServerInterceptor[] interceptors() { return new ServerInterceptor[]{ new CallRateMeteringInterceptor(new HashMap<>() {{ put("getVersion", new GrpcCallRateMeter(2, SECONDS)); }}) }; } It specifies a CLI can execute getversion 2 times / second. This is not a throttling mechansim, there is no blocking nor locking to slow call rates. When call rates are exceeded, calls are simply aborted. --- .../CallRateMeteringInterceptor.java | 107 ++++++++++++++++++ .../grpc/interceptor/GrpcCallRateMeter.java | 65 +++++++++++ 2 files changed, 172 insertions(+) create mode 100644 daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java create mode 100644 daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java diff --git a/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java b/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java new file mode 100644 index 00000000000..2f727e9cdc6 --- /dev/null +++ b/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java @@ -0,0 +1,107 @@ +package bisq.daemon.grpc.interceptor; + +import io.grpc.Metadata; +import io.grpc.ServerCall; +import io.grpc.ServerCallHandler; +import io.grpc.ServerInterceptor; +import io.grpc.StatusRuntimeException; + +import org.apache.commons.lang3.StringUtils; + +import java.util.Map; +import java.util.Objects; +import java.util.Optional; + +import lombok.extern.slf4j.Slf4j; + +import static io.grpc.Status.FAILED_PRECONDITION; +import static io.grpc.Status.PERMISSION_DENIED; +import static java.lang.String.format; + +@Slf4j +public final class CallRateMeteringInterceptor implements ServerInterceptor { + + // Maps the gRPC server method names to rate meters. This allows one interceptor + // instance to handle rate metering for any or all the methods in a Grpc*Service. + protected final Map serviceCallRateMeters; + + public CallRateMeteringInterceptor(Map serviceCallRateMeters) { + this.serviceCallRateMeters = serviceCallRateMeters; + } + + @Override + public ServerCall.Listener interceptCall(ServerCall serverCall, + Metadata headers, + ServerCallHandler serverCallHandler) { + Optional> rateMeterKV = getRateMeterKV(serverCall); + rateMeterKV.ifPresentOrElse( + (kv) -> checkRateMeterAndMaybeCloseCall(kv, serverCall), + () -> handleInterceptorConfigErrorAndCloseCall(serverCall)); + + // We leave it to the gRPC framework to clean up if the server call was closed + // above. But we still have to invoke startCall here because the method must + // return a ServerCall.Listener. + return serverCallHandler.startCall(serverCall, headers); + } + + private void checkRateMeterAndMaybeCloseCall(Map.Entry rateMeterKV, + ServerCall serverCall) { + String methodName = rateMeterKV.getKey(); + GrpcCallRateMeter rateMeter = rateMeterKV.getValue(); + + // The service method's rate meter doesn't start running until the 1st call. + if (!rateMeter.isRunning()) + rateMeter.start(); + + rateMeter.incrementCallsCount(); + + if (rateMeter.isCallRateExceeded()) + handlePermissionDeniedErrorAndCloseCall(methodName, rateMeter, serverCall); + else + log.info(rateMeter.getCallsCountProgress(methodName)); + } + + private void handleInterceptorConfigErrorAndCloseCall(ServerCall serverCall) + throws StatusRuntimeException { + String methodName = getRateMeterKey(serverCall); + String msg = format("%s's rate metering interceptor is incorrectly configured;" + + " its rate meter cannot be found ", + methodName); + log.error(StringUtils.capitalize(msg) + "."); + serverCall.close(FAILED_PRECONDITION.withDescription(msg), new Metadata()); + } + + private void handlePermissionDeniedErrorAndCloseCall(String methodName, + GrpcCallRateMeter rateMeter, + ServerCall serverCall) + throws StatusRuntimeException { + String msg = getDefaultRateExceededError(methodName, rateMeter); + log.error(StringUtils.capitalize(msg) + "."); + serverCall.close(PERMISSION_DENIED.withDescription(msg), new Metadata()); + } + + private String getDefaultRateExceededError(String methodName, + GrpcCallRateMeter rateMeter) { + // The derived method name may not be an exact match to CLI's method name. + String timeUnitName = StringUtils.chop(rateMeter.getTimeUnit().name().toLowerCase()); + return format("the maximum allowed number of %s calls (%d/%s) has been exceeded by %d calls", + methodName.toLowerCase(), + rateMeter.getAllowedCallsPerTimeUnit(), + timeUnitName, + rateMeter.getCallsCount() - rateMeter.getAllowedCallsPerTimeUnit()); + } + + private Optional> getRateMeterKV(ServerCall serverCall) { + String rateMeterKey = getRateMeterKey(serverCall); + return serviceCallRateMeters.entrySet().stream() + .filter((e) -> e.getKey().equals(rateMeterKey)).findFirst(); + } + + private String getRateMeterKey(ServerCall serverCall) { + // Get the rate meter map key from the full rpc service name. The key name + // is hard coded in the Grpc*Service interceptors() method. + String fullServiceName = serverCall.getMethodDescriptor().getServiceName(); + return StringUtils.uncapitalize(Objects.requireNonNull(fullServiceName) + .substring("io.bisq.protobuffer.".length())); + } +} diff --git a/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java b/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java new file mode 100644 index 00000000000..62dbdc9a784 --- /dev/null +++ b/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java @@ -0,0 +1,65 @@ +package bisq.daemon.grpc.interceptor; + +import bisq.common.Timer; +import bisq.common.UserThread; + +import org.apache.commons.lang3.StringUtils; + +import java.util.concurrent.TimeUnit; + +import lombok.Getter; +import lombok.extern.slf4j.Slf4j; + +import javax.annotation.Nullable; + +import static java.lang.String.format; + +@Slf4j +public final class GrpcCallRateMeter { + + @Getter + private final long allowedCallsPerTimeUnit; + @Getter + private final TimeUnit timeUnit; + + @Getter + private long callsCount = 0; + + @Getter + private boolean isRunning; + + @Nullable + private Timer timer; + + public GrpcCallRateMeter(long allowedCallsPerTimeUnit, TimeUnit timeUnit) { + this.allowedCallsPerTimeUnit = allowedCallsPerTimeUnit; + this.timeUnit = timeUnit; + } + + public void start() { + if (timer != null) + timer.stop(); + + timer = UserThread.runPeriodically(() -> callsCount = 0, 1, timeUnit); + isRunning = true; + } + + public void incrementCallsCount() { + callsCount++; + } + + public boolean isCallRateExceeded() { + return callsCount > allowedCallsPerTimeUnit; + } + + public String getCallsCountProgress(String calledMethodName) { + String shortTimeUnitName = StringUtils.chop(timeUnit.name().toLowerCase()); + return format("%s has been called %d time%s in the last %s; the rate limit is %d/%s.", + calledMethodName, + callsCount, + callsCount == 1 ? "" : "s", + shortTimeUnitName, + allowedCallsPerTimeUnit, + shortTimeUnitName); + } +} From 89e218787893e0a975d7b7b084a2e82da7e95633 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Thu, 17 Dec 2020 14:12:04 -0300 Subject: [PATCH 17/45] Change long to int, tidy up error msg --- .../grpc/interceptor/CallRateMeteringInterceptor.java | 6 ++++-- .../bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java | 6 +++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java b/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java index 2f727e9cdc6..ab82978f016 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java +++ b/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java @@ -84,11 +84,13 @@ private String getDefaultRateExceededError(String methodName, GrpcCallRateMeter rateMeter) { // The derived method name may not be an exact match to CLI's method name. String timeUnitName = StringUtils.chop(rateMeter.getTimeUnit().name().toLowerCase()); - return format("the maximum allowed number of %s calls (%d/%s) has been exceeded by %d calls", + int callCountAboveLimit = rateMeter.getCallsCount() - rateMeter.getAllowedCallsPerTimeUnit(); + return format("the maximum allowed number of %s calls (%d/%s) has been exceeded by %d call%s", methodName.toLowerCase(), rateMeter.getAllowedCallsPerTimeUnit(), timeUnitName, - rateMeter.getCallsCount() - rateMeter.getAllowedCallsPerTimeUnit()); + callCountAboveLimit, + callCountAboveLimit == 1 ? "" : "s"); } private Optional> getRateMeterKV(ServerCall serverCall) { diff --git a/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java b/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java index 62dbdc9a784..45a07f88978 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java +++ b/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java @@ -18,12 +18,12 @@ public final class GrpcCallRateMeter { @Getter - private final long allowedCallsPerTimeUnit; + private final int allowedCallsPerTimeUnit; @Getter private final TimeUnit timeUnit; @Getter - private long callsCount = 0; + private int callsCount = 0; @Getter private boolean isRunning; @@ -31,7 +31,7 @@ public final class GrpcCallRateMeter { @Nullable private Timer timer; - public GrpcCallRateMeter(long allowedCallsPerTimeUnit, TimeUnit timeUnit) { + public GrpcCallRateMeter(int allowedCallsPerTimeUnit, TimeUnit timeUnit) { this.allowedCallsPerTimeUnit = allowedCallsPerTimeUnit; this.timeUnit = timeUnit; } From a5ed17e43f541fc80f5a873460379263ff52d538 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Sat, 19 Dec 2020 13:14:06 -0300 Subject: [PATCH 18/45] Add license comment, stop & toString methods, and make isRunning transient --- .../grpc/interceptor/GrpcCallRateMeter.java | 42 ++++++++++++++++--- 1 file changed, 36 insertions(+), 6 deletions(-) diff --git a/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java b/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java index 45a07f88978..ea70dcc16dd 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java +++ b/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java @@ -1,3 +1,20 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + package bisq.daemon.grpc.interceptor; import bisq.common.Timer; @@ -21,12 +38,10 @@ public final class GrpcCallRateMeter { private final int allowedCallsPerTimeUnit; @Getter private final TimeUnit timeUnit; - @Getter private int callsCount = 0; - @Getter - private boolean isRunning; + private transient boolean isRunning; @Nullable private Timer timer; @@ -37,11 +52,16 @@ public GrpcCallRateMeter(int allowedCallsPerTimeUnit, TimeUnit timeUnit) { } public void start() { + stop(); + timer = UserThread.runPeriodically(() -> callsCount = 0, 1, timeUnit); + isRunning = true; + } + + public void stop() { if (timer != null) timer.stop(); - timer = UserThread.runPeriodically(() -> callsCount = 0, 1, timeUnit); - isRunning = true; + isRunning = false; } public void incrementCallsCount() { @@ -54,7 +74,7 @@ public boolean isCallRateExceeded() { public String getCallsCountProgress(String calledMethodName) { String shortTimeUnitName = StringUtils.chop(timeUnit.name().toLowerCase()); - return format("%s has been called %d time%s in the last %s; the rate limit is %d/%s.", + return format("%s has been called %d time%s in the last %s, rate limit is %d/%s", calledMethodName, callsCount, callsCount == 1 ? "" : "s", @@ -62,4 +82,14 @@ public String getCallsCountProgress(String calledMethodName) { allowedCallsPerTimeUnit, shortTimeUnitName); } + + @Override + public String toString() { + return "GrpcCallRateMeter{" + + "allowedCallsPerTimeUnit=" + allowedCallsPerTimeUnit + + ", timeUnit=" + timeUnit.name() + + ", callsCount=" + callsCount + + ", isRunning=" + isRunning + + '}'; + } } From b307593c82a86c047b9b3c21551af5ed59c088c1 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Sat, 19 Dec 2020 15:32:40 -0300 Subject: [PATCH 19/45] Make CallRateMeteringInterceptor configurable via json This adds a GrpcServiceRateMeteringConfig class that can read and write rate metering interceptor config files, and configure a gRPC rate metering service interceptor at startup. This seems excessive, but we need to be able to test and tune method rate metering without having to change hard coded, default interceptor rate meters. --- .../GrpcServiceRateMeteringConfig.java | 268 ++++++++++++++++++ 1 file changed, 268 insertions(+) create mode 100644 daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfig.java diff --git a/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfig.java b/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfig.java new file mode 100644 index 00000000000..151d1e3d6bb --- /dev/null +++ b/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfig.java @@ -0,0 +1,268 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + +package bisq.daemon.grpc.interceptor; + +import io.grpc.ServerInterceptor; + +import com.google.gson.Gson; +import com.google.gson.GsonBuilder; + +import com.google.common.annotations.VisibleForTesting; + +import org.apache.commons.lang3.StringUtils; + +import java.nio.file.Paths; + +import java.io.File; +import java.io.FileOutputStream; +import java.io.IOException; +import java.io.OutputStreamWriter; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.TimeUnit; + +import lombok.extern.slf4j.Slf4j; + +import static bisq.common.file.FileUtil.deleteFileIfExists; +import static bisq.common.file.FileUtil.renameFile; +import static com.google.common.base.Preconditions.checkNotNull; +import static java.lang.String.format; +import static java.lang.System.getProperty; +import static java.nio.charset.StandardCharsets.UTF_8; +import static java.nio.file.Files.readAllBytes; + +@VisibleForTesting +@Slf4j +public class GrpcServiceRateMeteringConfig { + + public static final String RATE_METERS_CONFIG_FILENAME = "ratemeters.json"; + + private static final String KEY_GRPC_SERVICE_CLASS_NAME = "grpcServiceClassName"; + private static final String KEY_METHOD_RATE_METERS = "methodRateMeters"; + private static final String KEY_ALLOWED_CALL_PER_TIME_UNIT = "allowedCallsPerTimeUnit"; + private static final String KEY_TIME_UNIT = "timeUnit"; + + private static final Gson gson = new GsonBuilder().setPrettyPrinting().create(); + + private final List> methodRateMeters; + private final String grpcServiceClassName; + + public GrpcServiceRateMeteringConfig(String grpcServiceClassName) { + this(grpcServiceClassName, new ArrayList<>()); + } + + public GrpcServiceRateMeteringConfig(String grpcServiceClassName, + List> methodRateMeters) { + this.grpcServiceClassName = grpcServiceClassName; + this.methodRateMeters = methodRateMeters; + } + + public GrpcServiceRateMeteringConfig addMethodCallRateMeter(String methodName, + int maxCalls, + TimeUnit timeUnit) { + methodRateMeters.add(new LinkedHashMap<>() {{ + put(methodName, new GrpcCallRateMeter(maxCalls, timeUnit)); + }}); + return this; + } + + public boolean isConfigForGrpcService(Class clazz) { + return isConfigForGrpcService(clazz.getSimpleName()); + } + + public boolean isConfigForGrpcService(String grpcServiceClassSimpleName) { + return this.grpcServiceClassName.equals(grpcServiceClassSimpleName); + } + + @Override + public String toString() { + return "GrpcServiceRateMeteringConfig{" + "\n" + + " grpcServiceClassName='" + grpcServiceClassName + '\'' + "\n" + + ", methodRateMeters=" + methodRateMeters + "\n" + + '}'; + } + + public static Optional getCustomRateMeteringInterceptor(File installationDir, + Class grpcServiceClass) { + File configFile = new File(installationDir, RATE_METERS_CONFIG_FILENAME); + return configFile.exists() + ? toServerInterceptor(configFile, grpcServiceClass) + : Optional.empty(); + } + + public static Optional toServerInterceptor(File configFile, Class grpcServiceClass) { + // From a global rate metering config file, create a specific gRPC service + // interceptor configuration in the form of an interceptor constructor argument, + // a map. + // Transforming json into the List> is a bit + // convoluted due to Gson's loss of generic type information during deserialization. + Optional grpcServiceConfig = getAllDeserializedConfigs(configFile) + .stream().filter(x -> x.isConfigForGrpcService(grpcServiceClass)).findFirst(); + if (grpcServiceConfig.isPresent()) { + Map serviceCallRateMeters = new HashMap<>(); + for (Map methodToRateMeterMap : grpcServiceConfig.get().methodRateMeters) { + Map.Entry entry = methodToRateMeterMap.entrySet().stream().findFirst().orElseThrow(() + -> new IllegalStateException("Gson deserialized a method rate meter configuration into an empty map.")); + serviceCallRateMeters.put(entry.getKey(), entry.getValue()); + } + return Optional.of(new CallRateMeteringInterceptor(serviceCallRateMeters)); + } else { + return Optional.empty(); + } + } + + @SuppressWarnings("unchecked") + private static List> getMethodRateMetersMap(Map gsonMap) { + List> rateMeters = new ArrayList<>(); + // Each gsonMap is a Map with a single entry: + // {getVersion={allowedCallsPerTimeUnit=8.0, timeUnit=SECONDS, callsCount=0.0, isRunning=false}} + // Convert it to a multiple entry Map, where the key + // is a method name. + for (Map singleEntryRateMeterMap : (List>) gsonMap.get(KEY_METHOD_RATE_METERS)) { + log.debug("Gson's single entry {} {} = {}", + gsonMap.get(KEY_GRPC_SERVICE_CLASS_NAME), + singleEntryRateMeterMap.getClass().getSimpleName(), + singleEntryRateMeterMap); + Map.Entry entry = singleEntryRateMeterMap.entrySet().stream().findFirst().orElseThrow(() + -> new IllegalStateException("Gson deserialized a method rate meter configuration into an empty map.")); + String methodName = entry.getKey(); + GrpcCallRateMeter rateMeter = getGrpcCallRateMeter(entry); + rateMeters.add(new LinkedHashMap<>() {{ + put(methodName, rateMeter); + }}); + } + return rateMeters; + } + + @SuppressWarnings({"rawtypes", "unchecked"}) + public static List deserialize(File configFile) { + verifyConfigFile(configFile); + List serviceMethodConfigurations = new ArrayList<>(); + // Gson cannot deserialize a json string to List + // so easily for us, so we do it here before returning the list of configurations. + List rawConfigList = gson.fromJson(toJson(configFile), ArrayList.class); + // Gson gave us a list of maps with keys grpcServiceClassName, methodRateMeters: + // String grpcServiceClassName + // List methodRateMeters + for (Object rawConfig : rawConfigList) { + Map gsonMap = (Map) rawConfig; + String grpcServiceClassName = (String) gsonMap.get(KEY_GRPC_SERVICE_CLASS_NAME); + List> rateMeters = getMethodRateMetersMap(gsonMap); + serviceMethodConfigurations.add(new GrpcServiceRateMeteringConfig(grpcServiceClassName, rateMeters)); + } + return serviceMethodConfigurations; + } + + @SuppressWarnings("unchecked") + private static GrpcCallRateMeter getGrpcCallRateMeter(Map.Entry gsonEntry) { + Map valueMap = (Map) gsonEntry.getValue(); + int allowedCallsPerTimeUnit = ((Number) valueMap.get(KEY_ALLOWED_CALL_PER_TIME_UNIT)).intValue(); + TimeUnit timeUnit = TimeUnit.valueOf((String) valueMap.get(KEY_TIME_UNIT)); + return new GrpcCallRateMeter(allowedCallsPerTimeUnit, timeUnit); + } + + private static void verifyConfigFile(File configFile) { + if (configFile == null) + throw new IllegalStateException("Cannot read null json config file."); + + if (!configFile.exists()) + throw new IllegalStateException(format("cannot find json config file %s", configFile.getAbsolutePath())); + } + + private static String toJson(File configFile) { + try { + return new String(readAllBytes(Paths.get(configFile.getAbsolutePath()))); + } catch (IOException ex) { + throw new IllegalStateException(format("Cannot read json string from file %s.", + configFile.getAbsolutePath())); + } + } + + private static List allDeserializedConfigs; + + private static List getAllDeserializedConfigs(File configFile) { + // We deserialize once, not for each gRPC service wanting an interceptor. + if (allDeserializedConfigs == null) + allDeserializedConfigs = deserialize(configFile); + + return allDeserializedConfigs; + } + + @VisibleForTesting + public static class Builder { + private final List rateMeterConfigs = new ArrayList<>(); + + public void addCallRateMeter(String grpcServiceClassName, + String methodName, + int maxCalls, + TimeUnit timeUnit) { + log.info("Adding call rate metering definition {}.{} ({}/{}).", + grpcServiceClassName, + methodName, + maxCalls, + StringUtils.chop(timeUnit.name().toLowerCase())); + rateMeterConfigs.stream().filter(c -> c.isConfigForGrpcService(grpcServiceClassName)) + .findFirst().ifPresentOrElse( + (config) -> config.addMethodCallRateMeter(methodName, maxCalls, timeUnit), + () -> rateMeterConfigs.add(new GrpcServiceRateMeteringConfig(grpcServiceClassName) + .addMethodCallRateMeter(methodName, maxCalls, timeUnit))); + } + + public File build() { + File tmpFile = serializeRateMeterDefinitions(); + File configFile = new File("/tmp/ratemeters.json"); + try { + deleteFileIfExists(configFile); + renameFile(tmpFile, configFile); + } catch (IOException ex) { + throw new IllegalStateException(format("Could not create config file %s.", + configFile.getAbsolutePath()), ex); + } + return configFile; + } + + private File serializeRateMeterDefinitions() { + String json = gson.toJson(rateMeterConfigs); + File file = createTmpFile(); + try (OutputStreamWriter outputStreamWriter = + new OutputStreamWriter(new FileOutputStream(checkNotNull(file), false), UTF_8)) { + outputStreamWriter.write(json); + } catch (Exception ex) { + throw new IllegalStateException(format("Cannot write file for json string %s.", json), ex); + } + return file; + } + + private File createTmpFile() { + File file; + try { + file = File.createTempFile("ratemeters_", + ".tmp", + Paths.get(getProperty("java.io.tmpdir")).toFile()); + } catch (IOException ex) { + throw new IllegalStateException("Cannot create tmp ratemeters json file.", ex); + } + return file; + } + } +} From 5de910a03d73ad9e17c36ec3a952bfcff2786407 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Sat, 19 Dec 2020 15:40:27 -0300 Subject: [PATCH 20/45] Add unit test dependencies to daemon subproject --- build.gradle | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/build.gradle b/build.gradle index 8b1004db5be..edf54f30132 100644 --- a/build.gradle +++ b/build.gradle @@ -592,6 +592,12 @@ configure(project(':daemon')) { compileOnly "org.projectlombok:lombok:$lombokVersion" compileOnly "javax.annotation:javax.annotation-api:$javaxAnnotationVersion" annotationProcessor "org.projectlombok:lombok:$lombokVersion" + + testCompile "org.junit.jupiter:junit-jupiter-api:$jupiterVersion" + testCompile "org.junit.jupiter:junit-jupiter-params:$jupiterVersion" + testCompileOnly "org.projectlombok:lombok:$lombokVersion" + testAnnotationProcessor "org.projectlombok:lombok:$lombokVersion" + testRuntime("org.junit.jupiter:junit-jupiter-engine:$jupiterVersion") } } From 455ed67f9bc4d1ec94a736c83adcf4e813d30bd6 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Sat, 19 Dec 2020 15:41:55 -0300 Subject: [PATCH 21/45] Add GrpcServiceRateMeteringConfigTest --- .../GrpcServiceRateMeteringConfigTest.java | 149 ++++++++++++++++++ 1 file changed, 149 insertions(+) create mode 100644 daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java diff --git a/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java b/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java new file mode 100644 index 00000000000..ea352858c36 --- /dev/null +++ b/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java @@ -0,0 +1,149 @@ +package bisq.daemon.grpc.interceptor; + +import io.grpc.ServerInterceptor; + +import java.nio.file.Paths; + +import java.io.File; + +import java.util.Optional; +import java.util.concurrent.TimeUnit; + +import lombok.extern.slf4j.Slf4j; + +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; + +import static bisq.daemon.grpc.interceptor.GrpcServiceRateMeteringConfig.getCustomRateMeteringInterceptor; +import static java.lang.System.getProperty; +import static java.util.concurrent.TimeUnit.DAYS; +import static java.util.concurrent.TimeUnit.HOURS; +import static java.util.concurrent.TimeUnit.MINUTES; +import static java.util.concurrent.TimeUnit.SECONDS; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + + + +import bisq.daemon.grpc.GrpcVersionService; + +@Slf4j +public class GrpcServiceRateMeteringConfigTest { + + private static final GrpcServiceRateMeteringConfig.Builder builder = new GrpcServiceRateMeteringConfig.Builder(); + private static File configFile; + private static Optional versionServiceInterceptor; + + @BeforeClass + public static void setup() { + builder.addCallRateMeter(GrpcVersionService.class.getSimpleName(), + "getVersion", + 2, + SECONDS); + builder.addCallRateMeter(GrpcVersionService.class.getSimpleName(), + "getNadaButDoNotBreakAnything", + 100, + DAYS); + + // The other Grpc*Service classes are not @VisibleForTesting, so we hardcode + // the simple class name. + builder.addCallRateMeter("GrpcOffersService", + "createOffer", + 5, + MINUTES); + builder.addCallRateMeter("GrpcOffersService", + "takeOffer", + 10, + DAYS); + builder.addCallRateMeter("GrpcWalletsService", + "sendBtc", + 3, + HOURS); + } + + @Before + public void buildConfigFile() { + if (configFile == null) + configFile = builder.build(); + } + + @Test + public void testConfigFileBuild() { + assertNotNull(configFile); + assertTrue(configFile.exists()); + assertTrue(configFile.length() > 0); + String expectedConfigFilePath = Paths.get(getProperty("java.io.tmpdir")) + File.separator + "ratemeters.json"; + assertEquals(expectedConfigFilePath, configFile.getAbsolutePath()); + } + + @Test + public void testGrpcVersionServiceRateMeteringConfig() { + CallRateMeteringInterceptor versionServiceInterceptor = buildInterceptor(); + assertEquals(2, versionServiceInterceptor.serviceCallRateMeters.size()); + + GrpcCallRateMeter versionCallRateMeter = versionServiceInterceptor.serviceCallRateMeters.get("getVersion"); + assertFalse(versionCallRateMeter.isRunning()); + assertEquals(2, versionCallRateMeter.getAllowedCallsPerTimeUnit()); + assertEquals(SECONDS, versionCallRateMeter.getTimeUnit()); + assertFalse(versionCallRateMeter.isCallRateExceeded()); + for (int i = 1; i <= 3; i++) { + versionCallRateMeter.incrementCallsCount(); + } + assertEquals(3, versionCallRateMeter.getCallsCount()); + assertTrue(versionCallRateMeter.isCallRateExceeded()); + } + + @Test + public void testRunningRateMetering() { + CallRateMeteringInterceptor versionServiceInterceptor = buildInterceptor(); + GrpcCallRateMeter versionCallRateMeter = versionServiceInterceptor.serviceCallRateMeters.get("getVersion"); + + versionCallRateMeter.start(); + assertTrue(versionCallRateMeter.isRunning()); + + // The timer resets the call count to 0 every 1s (the meter's configured timeunit). + // Wait 1.1s to let it do that before bumping the call count and checking state. + rest(1100); + + assertEquals(0, versionCallRateMeter.getCallsCount()); + assertFalse(versionCallRateMeter.isCallRateExceeded()); + + // Simulate calling 'getVersion' three times. + for (int i = 1; i <= 3; i++) { + versionCallRateMeter.incrementCallsCount(); + } + + assertEquals(3, versionCallRateMeter.getCallsCount()); + assertTrue(versionCallRateMeter.isCallRateExceeded()); + versionCallRateMeter.stop(); + assertFalse(versionCallRateMeter.isRunning()); + log.debug("Configured {}", versionServiceInterceptor); + } + + @AfterClass + public static void teardown() { + if (configFile != null) + configFile.deleteOnExit(); + } + + private void rest(long milliseconds) { + try { + TimeUnit.MILLISECONDS.sleep(milliseconds); + } catch (InterruptedException ignored) { + } + } + + private CallRateMeteringInterceptor buildInterceptor() { + if (versionServiceInterceptor == null) { + versionServiceInterceptor = getCustomRateMeteringInterceptor( + configFile.getParentFile(), + GrpcVersionService.class); + } + assertTrue(versionServiceInterceptor.isPresent()); + return (CallRateMeteringInterceptor) versionServiceInterceptor.get(); + } +} From 830a5f009a78c9eb59e29cb8ba5cb585241de867 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Sat, 19 Dec 2020 15:43:44 -0300 Subject: [PATCH 22/45] Add license note --- .../GrpcServiceRateMeteringConfigTest.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java b/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java index ea352858c36..42e98ca1de6 100644 --- a/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java +++ b/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java @@ -1,3 +1,20 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + package bisq.daemon.grpc.interceptor; import io.grpc.ServerInterceptor; From 9f679deb085e827dbc1aba08c81136830457efc0 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Sat, 19 Dec 2020 15:47:20 -0300 Subject: [PATCH 23/45] Add license note and toString method --- .../CallRateMeteringInterceptor.java | 33 ++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java b/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java index ab82978f016..33db3dc907b 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java +++ b/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java @@ -1,3 +1,20 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + package bisq.daemon.grpc.interceptor; import io.grpc.Metadata; @@ -17,6 +34,7 @@ import static io.grpc.Status.FAILED_PRECONDITION; import static io.grpc.Status.PERMISSION_DENIED; import static java.lang.String.format; +import static java.util.stream.Collectors.joining; @Slf4j public final class CallRateMeteringInterceptor implements ServerInterceptor { @@ -65,7 +83,7 @@ private void handleInterceptorConfigErrorAndCloseCall(ServerCall serverCal throws StatusRuntimeException { String methodName = getRateMeterKey(serverCall); String msg = format("%s's rate metering interceptor is incorrectly configured;" - + " its rate meter cannot be found ", + + " its rate meter cannot be found", methodName); log.error(StringUtils.capitalize(msg) + "."); serverCall.close(FAILED_PRECONDITION.withDescription(msg), new Metadata()); @@ -106,4 +124,17 @@ private String getRateMeterKey(ServerCall serverCall) { return StringUtils.uncapitalize(Objects.requireNonNull(fullServiceName) .substring("io.bisq.protobuffer.".length())); } + + @Override + public String toString() { + String rateMetersString = + serviceCallRateMeters.entrySet() + .stream() + .map(Object::toString) + .collect(joining("\n\t\t")); + return "CallRateMeteringInterceptor {" + "\n\t" + + "serviceCallRateMeters {" + "\n\t\t" + + rateMetersString + "\n\t" + "}" + "\n" + + "}"; + } } From bb8d2ae7c42a1533612101479abd85b007dd4e5f Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Sat, 19 Dec 2020 15:50:00 -0300 Subject: [PATCH 24/45] Inject Config into CoreApi --- core/src/main/java/bisq/core/api/CoreApi.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/bisq/core/api/CoreApi.java b/core/src/main/java/bisq/core/api/CoreApi.java index 6709bf42ff1..bc2fb53122b 100644 --- a/core/src/main/java/bisq/core/api/CoreApi.java +++ b/core/src/main/java/bisq/core/api/CoreApi.java @@ -31,6 +31,7 @@ import bisq.core.trade.statistics.TradeStatisticsManager; import bisq.common.app.Version; +import bisq.common.config.Config; import bisq.common.handlers.ResultHandler; import org.bitcoinj.core.Coin; @@ -46,6 +47,8 @@ import java.util.Set; import java.util.function.Consumer; +import lombok.Getter; +import lombok.extern.flogger.Flogger; import lombok.extern.slf4j.Slf4j; /** @@ -56,6 +59,8 @@ @Slf4j public class CoreApi { + @Getter + private final Config config; private final CoreDisputeAgentsService coreDisputeAgentsService; private final CoreOffersService coreOffersService; private final CorePaymentAccountsService paymentAccountsService; @@ -65,13 +70,15 @@ public class CoreApi { private final TradeStatisticsManager tradeStatisticsManager; @Inject - public CoreApi(CoreDisputeAgentsService coreDisputeAgentsService, + public CoreApi(Config config, + CoreDisputeAgentsService coreDisputeAgentsService, CoreOffersService coreOffersService, CorePaymentAccountsService paymentAccountsService, CorePriceService corePriceService, CoreTradesService coreTradesService, CoreWalletsService walletsService, TradeStatisticsManager tradeStatisticsManager) { + this.config = config; this.coreDisputeAgentsService = coreDisputeAgentsService; this.coreOffersService = coreOffersService; this.paymentAccountsService = paymentAccountsService; From ea97a801e5c40706c394923497c160166413e592 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Sat, 19 Dec 2020 16:31:41 -0300 Subject: [PATCH 25/45] Don't cancel gRPC call if an interceptor does not meter all methods --- .../interceptor/CallRateMeteringInterceptor.java | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java b/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java index 33db3dc907b..562f27fb031 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java +++ b/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java @@ -31,7 +31,6 @@ import lombok.extern.slf4j.Slf4j; -import static io.grpc.Status.FAILED_PRECONDITION; import static io.grpc.Status.PERMISSION_DENIED; import static java.lang.String.format; import static java.util.stream.Collectors.joining; @@ -54,7 +53,7 @@ public ServerCall.Listener interceptCall(ServerCall> rateMeterKV = getRateMeterKV(serverCall); rateMeterKV.ifPresentOrElse( (kv) -> checkRateMeterAndMaybeCloseCall(kv, serverCall), - () -> handleInterceptorConfigErrorAndCloseCall(serverCall)); + () -> handleMissingRateMeterConfiguration(serverCall)); // We leave it to the gRPC framework to clean up if the server call was closed // above. But we still have to invoke startCall here because the method must @@ -79,14 +78,11 @@ private void checkRateMeterAndMaybeCloseCall(Map.Entry serverCall) + private void handleMissingRateMeterConfiguration(ServerCall serverCall) throws StatusRuntimeException { - String methodName = getRateMeterKey(serverCall); - String msg = format("%s's rate metering interceptor is incorrectly configured;" - + " its rate meter cannot be found", - methodName); - log.error(StringUtils.capitalize(msg) + "."); - serverCall.close(FAILED_PRECONDITION.withDescription(msg), new Metadata()); + log.debug("The gRPC service's call rate metering interceptor does not" + + " meter the {} method.", + getRateMeterKey(serverCall)); } private void handlePermissionDeniedErrorAndCloseCall(String methodName, From 87f75ee10ca4caec5387c64102359eec8a38fd59 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Sat, 19 Dec 2020 16:41:26 -0300 Subject: [PATCH 26/45] Configure GrpcVersionService's rate metering interceptor This change demonstrates how a method call rate metering interceptor is configured for a gRPC service. GrpcVersionService uses a custom rate metering interceptor, or none. A commented out, 'default' interceptor is defined as a usage example. --- .../java/bisq/daemon/grpc/GrpcServer.java | 4 +- .../bisq/daemon/grpc/GrpcVersionService.java | 54 ++++++++++++++++++- 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcServer.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcServer.java index 24fc267573b..b72f0eafe08 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcServer.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcServer.java @@ -31,6 +31,8 @@ import lombok.extern.slf4j.Slf4j; +import static io.grpc.ServerInterceptors.interceptForward; + import bisq.daemon.grpc.interceptor.PasswordAuthInterceptor; @@ -60,7 +62,7 @@ public GrpcServer(Config config, .addService(priceService) .addService(tradeStatisticsService) .addService(tradesService) - .addService(versionService) + .addService(interceptForward(versionService, versionService.interceptors())) .addService(walletsService) .intercept(passwordAuthInterceptor) .build(); diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcVersionService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcVersionService.java index 48c4d8a6b12..9fdba3b1654 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcVersionService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcVersionService.java @@ -1,3 +1,20 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + package bisq.daemon.grpc; import bisq.core.api.CoreApi; @@ -6,11 +23,29 @@ import bisq.proto.grpc.GetVersionReply; import bisq.proto.grpc.GetVersionRequest; +import io.grpc.ServerInterceptor; import io.grpc.stub.StreamObserver; import javax.inject.Inject; -class GrpcVersionService extends GetVersionGrpc.GetVersionImplBase { +import com.google.common.annotations.VisibleForTesting; + +import java.util.HashMap; +import java.util.Optional; + +import lombok.extern.slf4j.Slf4j; + +import static bisq.daemon.grpc.interceptor.GrpcServiceRateMeteringConfig.getCustomRateMeteringInterceptor; +import static java.util.concurrent.TimeUnit.SECONDS; + + + +import bisq.daemon.grpc.interceptor.CallRateMeteringInterceptor; +import bisq.daemon.grpc.interceptor.GrpcCallRateMeter; + +@VisibleForTesting +@Slf4j +public class GrpcVersionService extends GetVersionGrpc.GetVersionImplBase { private final CoreApi coreApi; private final GrpcExceptionHandler exceptionHandler; @@ -31,4 +66,21 @@ public void getVersion(GetVersionRequest req, StreamObserver re exceptionHandler.handleException(cause, responseObserver); } } + + final ServerInterceptor[] interceptors() { + Optional rateMeteringInterceptor = rateMeteringInterceptor(); + return rateMeteringInterceptor.map(serverInterceptor -> + new ServerInterceptor[]{serverInterceptor}).orElseGet(() -> new ServerInterceptor[0]); + } + + final Optional rateMeteringInterceptor() { + @SuppressWarnings("unused") // Defined as a usage example. + CallRateMeteringInterceptor defaultCallRateMeteringInterceptor = + new CallRateMeteringInterceptor(new HashMap<>() {{ + put("getVersion", new GrpcCallRateMeter(100, SECONDS)); + }}); + + return getCustomRateMeteringInterceptor(coreApi.getConfig().appDataDir, this.getClass()) + .or(Optional::empty /* Optional.of(defaultCallRateMeteringInterceptor) */); + } } From 56a5c7938df2f63b166e7c7eba509abdc19ad576 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Sat, 19 Dec 2020 16:54:44 -0300 Subject: [PATCH 27/45] Add ApiTestConfig option --callRateMeteringConfigPath Points to a call rate metering interceptor configuration file. Test cases can build a config file, and the test harness will install it into a daemon's appDataDir before startup. The installed config file will be used to configure gRPC service rate metering interceptors. --- .../src/main/java/bisq/apitest/config/ApiTestConfig.java | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/apitest/src/main/java/bisq/apitest/config/ApiTestConfig.java b/apitest/src/main/java/bisq/apitest/config/ApiTestConfig.java index b0ce2c548e6..bffa009935c 100644 --- a/apitest/src/main/java/bisq/apitest/config/ApiTestConfig.java +++ b/apitest/src/main/java/bisq/apitest/config/ApiTestConfig.java @@ -71,6 +71,7 @@ public class ApiTestConfig { static final String SKIP_TESTS = "skipTests"; static final String SHUTDOWN_AFTER_TESTS = "shutdownAfterTests"; static final String SUPPORTING_APPS = "supportingApps"; + static final String CALL_RATE_METERING_CONFIG_PATH = "callRateMeteringConfigPath"; static final String ENABLE_BISQ_DEBUGGING = "enableBisqDebugging"; // Default values for certain options @@ -102,6 +103,7 @@ public class ApiTestConfig { public final boolean skipTests; public final boolean shutdownAfterTests; public final List supportingApps; + public final String callRateMeteringConfigPath; public final boolean enableBisqDebugging; // Immutable system configurations set in the constructor. @@ -228,6 +230,12 @@ public ApiTestConfig(String... args) { .ofType(String.class) .defaultsTo("bitcoind,seednode,arbdaemon,alicedaemon,bobdaemon"); + ArgumentAcceptingOptionSpec callRateMeteringConfigPathOpt = + parser.accepts(CALL_RATE_METERING_CONFIG_PATH, + "Install a ratemeters.json file to configure call rate metering interceptors") + .withRequiredArg() + .defaultsTo(EMPTY); + ArgumentAcceptingOptionSpec enableBisqDebuggingOpt = parser.accepts(ENABLE_BISQ_DEBUGGING, "Start Bisq apps with remote debug options") @@ -289,6 +297,7 @@ public ApiTestConfig(String... args) { this.skipTests = options.valueOf(skipTestsOpt); this.shutdownAfterTests = options.valueOf(shutdownAfterTestsOpt); this.supportingApps = asList(options.valueOf(supportingAppsOpt).split(",")); + this.callRateMeteringConfigPath = options.valueOf(callRateMeteringConfigPathOpt); this.enableBisqDebugging = options.valueOf(enableBisqDebuggingOpt); // Assign values to special-case static fields. From d5657e97604d8766905f6b05b64e8d9f83c78c2a Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Sat, 19 Dec 2020 17:04:40 -0300 Subject: [PATCH 28/45] Install call rate metering config file before startup Copy the config file at --callRateMeteringConfigPath to each daemon's appDataDir, where it will be detected at server startup. --- .../src/main/java/bisq/apitest/Scaffold.java | 35 ++++++++++++++++--- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/apitest/src/main/java/bisq/apitest/Scaffold.java b/apitest/src/main/java/bisq/apitest/Scaffold.java index 333f59285a3..79075f044d5 100644 --- a/apitest/src/main/java/bisq/apitest/Scaffold.java +++ b/apitest/src/main/java/bisq/apitest/Scaffold.java @@ -154,7 +154,8 @@ public void tearDown() { try { log.info("Shutting down executor service ..."); executor.shutdownNow(); - executor.awaitTermination(config.supportingApps.size() * 2000, MILLISECONDS); + //noinspection ResultOfMethodCallIgnored + executor.awaitTermination(config.supportingApps.size() * 2000L, MILLISECONDS); SetupTask[] orderedTasks = new SetupTask[]{ bobNodeTask, aliceNodeTask, arbNodeTask, seedNodeTask, bitcoindTask}; @@ -218,20 +219,25 @@ public void installDaoSetupDirectories() { if (copyBitcoinRegtestDir.run().getExitStatus() != 0) throw new IllegalStateException("Could not install bitcoin regtest dir"); + String aliceDataDir = daoSetupDir + "/" + alicedaemon.appName; BashCommand copyAliceDataDir = new BashCommand( - "cp -rf " + daoSetupDir + "/" + alicedaemon.appName - + " " + config.rootAppDataDir); + "cp -rf " + aliceDataDir + " " + config.rootAppDataDir); if (copyAliceDataDir.run().getExitStatus() != 0) throw new IllegalStateException("Could not install alice data dir"); + String bobDataDir = daoSetupDir + "/" + bobdaemon.appName; BashCommand copyBobDataDir = new BashCommand( - "cp -rf " + daoSetupDir + "/" + bobdaemon.appName - + " " + config.rootAppDataDir); + "cp -rf " + bobDataDir + " " + config.rootAppDataDir); if (copyBobDataDir.run().getExitStatus() != 0) throw new IllegalStateException("Could not install bob data dir"); log.info("Installed dao-setup files into {}", buildDataDir); + if (!config.callRateMeteringConfigPath.isEmpty()) { + installCallRateMeteringConfiguration(aliceDataDir); + installCallRateMeteringConfiguration(bobDataDir); + } + // Copy the blocknotify script from the src resources dir to the build // resources dir. Users may want to edit comment out some lines when all // of the default block notifcation ports being will not be used (to avoid @@ -287,6 +293,25 @@ private void installBitcoinBlocknotify() { } } + private void installCallRateMeteringConfiguration(String dataDir) throws IOException, InterruptedException { + File testRateMeteringFile = new File(config.callRateMeteringConfigPath); + if (!testRateMeteringFile.exists()) + throw new FileNotFoundException( + format("Call rate metering config file '%s' not found", config.callRateMeteringConfigPath)); + + BashCommand copyRateMeteringConfigFile = new BashCommand( + "cp -rf " + config.callRateMeteringConfigPath + " " + dataDir); + if (copyRateMeteringConfigFile.run().getExitStatus() != 0) + throw new IllegalStateException( + format("Could not install %s file in %s", + testRateMeteringFile.getAbsolutePath(), dataDir)); + + Path destPath = Paths.get(dataDir, testRateMeteringFile.getName()); + String chmod700Perms = "rwx------"; + Files.setPosixFilePermissions(destPath, PosixFilePermissions.fromString(chmod700Perms)); + log.info("Installed {} with perms {}.", destPath.toString(), chmod700Perms); + } + private void installShutdownHook() { // Background apps can be left running until the jvm is manually shutdown, // so we add a shutdown hook for that use case. From fabd7c8776437eb1adaaf372cefe755ff92c89fd Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Sat, 19 Dec 2020 17:08:53 -0300 Subject: [PATCH 29/45] Refactor testcase superclasses to support rate metering configs --- .../test/java/bisq/apitest/ApiTestCase.java | 11 +++ .../java/bisq/apitest/method/MethodTest.java | 70 +++++++++++++------ 2 files changed, 59 insertions(+), 22 deletions(-) diff --git a/apitest/src/test/java/bisq/apitest/ApiTestCase.java b/apitest/src/test/java/bisq/apitest/ApiTestCase.java index 7f84772f543..7d1f4ddf016 100644 --- a/apitest/src/test/java/bisq/apitest/ApiTestCase.java +++ b/apitest/src/test/java/bisq/apitest/ApiTestCase.java @@ -19,6 +19,7 @@ import java.net.InetAddress; +import java.io.File; import java.io.IOException; import java.util.HashMap; @@ -72,6 +73,16 @@ public class ApiTestCase { // gRPC service stubs are used by method & scenario tests, but not e2e tests. private static final Map grpcStubsCache = new HashMap<>(); + public static void setUpScaffold(File callRateMeteringConfigFile, + Enum... supportingApps) + throws InterruptedException, ExecutionException, IOException { + scaffold = new Scaffold(stream(supportingApps).map(Enum::name) + .collect(Collectors.joining(","))) + .setUp(); + config = scaffold.config; + bitcoinCli = new BitcoinCliHelper((config)); + } + public static void setUpScaffold(Enum... supportingApps) throws InterruptedException, ExecutionException, IOException { scaffold = new Scaffold(stream(supportingApps).map(Enum::name) diff --git a/apitest/src/test/java/bisq/apitest/method/MethodTest.java b/apitest/src/test/java/bisq/apitest/method/MethodTest.java index 791e22ef6c2..da078e1efc2 100644 --- a/apitest/src/test/java/bisq/apitest/method/MethodTest.java +++ b/apitest/src/test/java/bisq/apitest/method/MethodTest.java @@ -67,6 +67,7 @@ import java.io.PrintWriter; import java.util.List; +import java.util.function.Function; import java.util.stream.Collectors; import static bisq.apitest.config.BisqAppConfig.alicedaemon; @@ -99,37 +100,62 @@ public class MethodTest extends ApiTestCase { private static final CoreProtoResolver CORE_PROTO_RESOLVER = new CoreProtoResolver(); + private static final Function[], String> toNameList = (enums) -> + stream(enums).map(Enum::name).collect(Collectors.joining(",")); + + public static void startSupportingApps(File callRateMeteringConfigFile, + boolean registerDisputeAgents, + boolean generateBtcBlock, + Enum... supportingApps) { + try { + setUpScaffold(new String[]{ + "--supportingApps", toNameList.apply(supportingApps), + "--callRateMeteringConfigPath", callRateMeteringConfigFile.getAbsolutePath(), + "--enableBisqDebugging", "false" + }); + doPostStartup(registerDisputeAgents, generateBtcBlock, supportingApps); + } catch (Exception ex) { + fail(ex); + } + } + public static void startSupportingApps(boolean registerDisputeAgents, boolean generateBtcBlock, Enum... supportingApps) { try { - // To run Bisq apps in debug mode, use the other setUpScaffold method: - // setUpScaffold(new String[]{"--supportingApps", "bitcoind,seednode,arbdaemon,alicedaemon,bobdaemon", - // "--enableBisqDebugging", "true"}); - setUpScaffold(supportingApps); - if (registerDisputeAgents) { - registerDisputeAgents(arbdaemon); - } - - if (stream(supportingApps).map(Enum::name).anyMatch(name -> name.equals(alicedaemon.name()))) { - aliceStubs = grpcStubs(alicedaemon); - alicesDummyAcct = getDefaultPerfectDummyPaymentAccount(alicedaemon); - } - - if (stream(supportingApps).map(Enum::name).anyMatch(name -> name.equals(bobdaemon.name()))) { - bobStubs = grpcStubs(bobdaemon); - bobsDummyAcct = getDefaultPerfectDummyPaymentAccount(bobdaemon); - } - - // Generate 1 regtest block for alice's and/or bob's wallet to - // show 10 BTC balance, and allow time for daemons parse the new block. - if (generateBtcBlock) - genBtcBlocksThenWait(1, 1500); + setUpScaffold(new String[]{ + "--supportingApps", toNameList.apply(supportingApps), + "--enableBisqDebugging", "false" + }); + doPostStartup(registerDisputeAgents, generateBtcBlock, supportingApps); } catch (Exception ex) { fail(ex); } } + private static void doPostStartup(boolean registerDisputeAgents, + boolean generateBtcBlock, + Enum... supportingApps) { + if (registerDisputeAgents) { + registerDisputeAgents(arbdaemon); + } + + if (stream(supportingApps).map(Enum::name).anyMatch(name -> name.equals(alicedaemon.name()))) { + aliceStubs = grpcStubs(alicedaemon); + alicesDummyAcct = getDefaultPerfectDummyPaymentAccount(alicedaemon); + } + + if (stream(supportingApps).map(Enum::name).anyMatch(name -> name.equals(bobdaemon.name()))) { + bobStubs = grpcStubs(bobdaemon); + bobsDummyAcct = getDefaultPerfectDummyPaymentAccount(bobdaemon); + } + + // Generate 1 regtest block for alice's and/or bob's wallet to + // show 10 BTC balance, and allow time for daemons parse the new block. + if (generateBtcBlock) + genBtcBlocksThenWait(1, 1500); + } + // Convenience methods for building gRPC request objects protected final GetBalancesRequest createGetBalancesRequest(String currencyCode) { return GetBalancesRequest.newBuilder().setCurrencyCode(currencyCode).build(); From abc39402b5490e84040f6bc984d782bc67fafd35 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Sat, 19 Dec 2020 17:31:02 -0300 Subject: [PATCH 30/45] Test CallRateMeteringInterceptor --- .../CallRateMeteringInterceptorTest.java | 130 ++++++++++++++++++ 1 file changed, 130 insertions(+) create mode 100644 apitest/src/test/java/bisq/apitest/scenario/CallRateMeteringInterceptorTest.java diff --git a/apitest/src/test/java/bisq/apitest/scenario/CallRateMeteringInterceptorTest.java b/apitest/src/test/java/bisq/apitest/scenario/CallRateMeteringInterceptorTest.java new file mode 100644 index 00000000000..f2013133673 --- /dev/null +++ b/apitest/src/test/java/bisq/apitest/scenario/CallRateMeteringInterceptorTest.java @@ -0,0 +1,130 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + +package bisq.apitest.scenario; + +import io.grpc.StatusRuntimeException; + +import java.io.File; + +import lombok.extern.slf4j.Slf4j; + +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.MethodOrderer; +import org.junit.jupiter.api.Order; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestMethodOrder; + +import static bisq.apitest.Scaffold.BitcoinCoreApp.bitcoind; +import static bisq.apitest.config.BisqAppConfig.alicedaemon; +import static java.util.concurrent.TimeUnit.DAYS; +import static java.util.concurrent.TimeUnit.HOURS; +import static java.util.concurrent.TimeUnit.MINUTES; +import static java.util.concurrent.TimeUnit.SECONDS; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + + + +import bisq.apitest.method.GetVersionTest; +import bisq.apitest.method.MethodTest; +import bisq.daemon.grpc.GrpcVersionService; +import bisq.daemon.grpc.interceptor.GrpcServiceRateMeteringConfig; + + +@Slf4j +@TestMethodOrder(MethodOrderer.OrderAnnotation.class) +public class CallRateMeteringInterceptorTest extends MethodTest { + + private static final GetVersionTest getVersionTest = new GetVersionTest(); + + @BeforeAll + public static void setUp() { + File callRateMeteringConfigFile = buildInterceptorConfigFile(); + startSupportingApps(callRateMeteringConfigFile, + false, + false, + bitcoind, alicedaemon); + } + + @BeforeEach + public void sleep200Milliseconds() { + sleep(200); + } + + @Test + @Order(1) + public void testGetVersionCall1IsAllowed() { + getVersionTest.testGetVersion(); + } + + @Test + @Order(2) + public void testGetVersionCall2ShouldThrowException() { + Throwable exception = assertThrows(StatusRuntimeException.class, getVersionTest::testGetVersion); + assertEquals("PERMISSION_DENIED: the maximum allowed number of getversion calls (1/second) has been exceeded by 1 call", + exception.getMessage()); + } + + @Test + @Order(3) + public void testGetVersionCall3ShouldThrowException() { + Throwable exception = assertThrows(StatusRuntimeException.class, getVersionTest::testGetVersion); + assertEquals("PERMISSION_DENIED: the maximum allowed number of getversion calls (1/second) has been exceeded by 2 calls", + exception.getMessage()); + } + + @Test + @Order(4) + public void testGetVersionCall4IsAllowed() { + sleep(1100); // Let the server's rate meter reset the call count. + getVersionTest.testGetVersion(); + } + + @AfterAll + public static void tearDown() { + tearDownScaffold(); + } + + private static File buildInterceptorConfigFile() { + GrpcServiceRateMeteringConfig.Builder builder = new GrpcServiceRateMeteringConfig.Builder(); + builder.addCallRateMeter(GrpcVersionService.class.getSimpleName(), + "getVersion", + 1, + SECONDS); + builder.addCallRateMeter(GrpcVersionService.class.getSimpleName(), + "shouldNotBreakAnything", + 1000, + DAYS); + // Only GrpcVersionService is @VisibleForTesting, so we hardcode the class names. + builder.addCallRateMeter("GrpcOffersService", + "createOffer", + 5, + MINUTES); + builder.addCallRateMeter("GrpcOffersService", + "takeOffer", + 10, + DAYS); + builder.addCallRateMeter("GrpcTradesService", + "withdrawFunds", + 3, + HOURS); + return builder.build(); + } +} From a3eb4ed59acc88cf46e306bb3a50f7b79cc4aa14 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Sat, 19 Dec 2020 17:37:24 -0300 Subject: [PATCH 31/45] Remove unused import --- core/src/main/java/bisq/core/api/CoreApi.java | 1 - 1 file changed, 1 deletion(-) diff --git a/core/src/main/java/bisq/core/api/CoreApi.java b/core/src/main/java/bisq/core/api/CoreApi.java index bc2fb53122b..4dffaee9760 100644 --- a/core/src/main/java/bisq/core/api/CoreApi.java +++ b/core/src/main/java/bisq/core/api/CoreApi.java @@ -48,7 +48,6 @@ import java.util.function.Consumer; import lombok.Getter; -import lombok.extern.flogger.Flogger; import lombok.extern.slf4j.Slf4j; /** From 672eb79f95147fcb1dcae83148c9a081e717ac2a Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Mon, 21 Dec 2020 15:05:54 -0300 Subject: [PATCH 32/45] Revert "Append nullable withdrawalTxId field to Trade proto message" This reverts commit 6aa385e494eb8fa870257c76e078108607503d03. --- core/src/main/java/bisq/core/trade/Trade.java | 7 ------- core/src/main/java/bisq/core/trade/TradeManager.java | 1 - proto/src/main/proto/pb.proto | 1 - 3 files changed, 9 deletions(-) diff --git a/core/src/main/java/bisq/core/trade/Trade.java b/core/src/main/java/bisq/core/trade/Trade.java index b25587eba97..de5c1949bb7 100644 --- a/core/src/main/java/bisq/core/trade/Trade.java +++ b/core/src/main/java/bisq/core/trade/Trade.java @@ -317,10 +317,6 @@ public static protobuf.Trade.TradePeriodState toProtoMessage(Trade.TradePeriodSt @Getter @Setter private String payoutTxId; - @Nullable - @Getter - @Setter - private String withdrawalTxId; @Getter @Setter private long tradeAmountAsLong; @@ -558,7 +554,6 @@ public Message toProtoMessage() { Optional.ofNullable(takerFeeTxId).ifPresent(builder::setTakerFeeTxId); Optional.ofNullable(depositTxId).ifPresent(builder::setDepositTxId); Optional.ofNullable(payoutTxId).ifPresent(builder::setPayoutTxId); - Optional.ofNullable(withdrawalTxId).ifPresent(builder::setWithdrawalTxId); Optional.ofNullable(tradingPeerNodeAddress).ifPresent(e -> builder.setTradingPeerNodeAddress(tradingPeerNodeAddress.toProtoMessage())); Optional.ofNullable(contract).ifPresent(e -> builder.setContract(contract.toProtoMessage())); Optional.ofNullable(contractAsJson).ifPresent(builder::setContractAsJson); @@ -592,7 +587,6 @@ public static Trade fromProto(Trade trade, protobuf.Trade proto, CoreProtoResolv trade.setTakerFeeTxId(ProtoUtil.stringOrNullFromProto(proto.getTakerFeeTxId())); trade.setDepositTxId(ProtoUtil.stringOrNullFromProto(proto.getDepositTxId())); trade.setPayoutTxId(ProtoUtil.stringOrNullFromProto(proto.getPayoutTxId())); - trade.setWithdrawalTxId(ProtoUtil.stringOrNullFromProto(proto.getWithdrawalTxId())); trade.setContract(proto.hasContract() ? Contract.fromProto(proto.getContract(), coreProtoResolver) : null); trade.setContractAsJson(ProtoUtil.stringOrNullFromProto(proto.getContractAsJson())); trade.setContractHash(ProtoUtil.byteArrayOrNullFromProto(proto.getContractHash())); @@ -1130,7 +1124,6 @@ public String toString() { ",\n takerFeeTxId='" + takerFeeTxId + '\'' + ",\n depositTxId='" + depositTxId + '\'' + ",\n payoutTxId='" + payoutTxId + '\'' + - ",\n withdrawalTxId='" + withdrawalTxId + '\'' + ",\n tradeAmountAsLong=" + tradeAmountAsLong + ",\n tradePrice=" + tradePrice + ",\n tradingPeerNodeAddress=" + tradingPeerNodeAddress + diff --git a/core/src/main/java/bisq/core/trade/TradeManager.java b/core/src/main/java/bisq/core/trade/TradeManager.java index d6c99e28e3e..dc87d4a98cc 100644 --- a/core/src/main/java/bisq/core/trade/TradeManager.java +++ b/core/src/main/java/bisq/core/trade/TradeManager.java @@ -497,7 +497,6 @@ public void onSuccess(@javax.annotation.Nullable Transaction transaction) { onTradeCompleted(trade); trade.setState(Trade.State.WITHDRAW_COMPLETED); getTradeProtocol(trade).onWithdrawCompleted(); - trade.setWithdrawalTxId(transaction.getTxId().toString()); requestPersistence(); resultHandler.handleResult(); } diff --git a/proto/src/main/proto/pb.proto b/proto/src/main/proto/pb.proto index a4579016f0c..22d81d40de9 100644 --- a/proto/src/main/proto/pb.proto +++ b/proto/src/main/proto/pb.proto @@ -1450,7 +1450,6 @@ message Trade { string counter_currency_extra_data = 37; string asset_tx_proof_result = 38; // name of AssetTxProofResult enum string uid = 39; - string withdrawal_tx_id = 40; } message BuyerAsMakerTrade { From bdde24a46322e4c55561ef46fb8bce295d960c35 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Mon, 21 Dec 2020 15:10:43 -0300 Subject: [PATCH 33/45] Ajust TradeInfo to reverting 6aa385e494eb8fa870257c76e078108607503d03 --- core/src/main/java/bisq/core/api/model/TradeInfo.java | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/core/src/main/java/bisq/core/api/model/TradeInfo.java b/core/src/main/java/bisq/core/api/model/TradeInfo.java index ec9880d5c69..1a717a7672e 100644 --- a/core/src/main/java/bisq/core/api/model/TradeInfo.java +++ b/core/src/main/java/bisq/core/api/model/TradeInfo.java @@ -47,7 +47,6 @@ public class TradeInfo implements Payload { private final String takerFeeTxId; private final String depositTxId; private final String payoutTxId; - private final String withdrawalTxId; private final long tradeAmountAsLong; private final long tradePrice; private final String tradingPeerNodeAddress; @@ -74,7 +73,6 @@ public TradeInfo(TradeInfoBuilder builder) { this.takerFeeTxId = builder.takerFeeTxId; this.depositTxId = builder.depositTxId; this.payoutTxId = builder.payoutTxId; - this.withdrawalTxId = builder.withdrawalTxId; this.tradeAmountAsLong = builder.tradeAmountAsLong; this.tradePrice = builder.tradePrice; this.tradingPeerNodeAddress = builder.tradingPeerNodeAddress; @@ -108,7 +106,6 @@ public static TradeInfo toTradeInfo(Trade trade, String role) { .withTakerFeeTxId(trade.getTakerFeeTxId()) .withDepositTxId(trade.getDepositTxId()) .withPayoutTxId(trade.getPayoutTxId()) - .withWithdrawalTxId(trade.getWithdrawalTxId()) .withTradeAmountAsLong(trade.getTradeAmountAsLong()) .withTradePrice(trade.getTradePrice().getValue()) .withTradingPeerNodeAddress(Objects.requireNonNull( @@ -144,7 +141,6 @@ public bisq.proto.grpc.TradeInfo toProtoMessage() { .setTakerFeeTxId(takerFeeTxId == null ? "" : takerFeeTxId) .setDepositTxId(depositTxId == null ? "" : depositTxId) .setPayoutTxId(payoutTxId == null ? "" : payoutTxId) - .setWithdrawalTxId(withdrawalTxId == null ? "" : withdrawalTxId) .setTradeAmountAsLong(tradeAmountAsLong) .setTradePrice(tradePrice) .setTradingPeerNodeAddress(tradingPeerNodeAddress) @@ -184,7 +180,6 @@ public static class TradeInfoBuilder { private String takerFeeTxId; private String depositTxId; private String payoutTxId; - private String withdrawalTxId; private long tradeAmountAsLong; private long tradePrice; private String tradingPeerNodeAddress; @@ -254,11 +249,6 @@ public TradeInfoBuilder withPayoutTxId(String payoutTxId) { return this; } - public TradeInfoBuilder withWithdrawalTxId(String withdrawalTxId) { - this.withdrawalTxId = withdrawalTxId; - return this; - } - public TradeInfoBuilder withTradeAmountAsLong(long tradeAmountAsLong) { this.tradeAmountAsLong = tradeAmountAsLong; return this; @@ -342,7 +332,6 @@ public String toString() { ", takerFeeTxId='" + takerFeeTxId + '\'' + "\n" + ", depositTxId='" + depositTxId + '\'' + "\n" + ", payoutTxId='" + payoutTxId + '\'' + "\n" + - ", withdrawalTxId='" + withdrawalTxId + '\'' + "\n" + ", tradeAmountAsLong='" + tradeAmountAsLong + '\'' + "\n" + ", tradePrice='" + tradePrice + '\'' + "\n" + ", tradingPeerNodeAddress='" + tradingPeerNodeAddress + '\'' + "\n" + From 64c2ac51699461c4ce5af65b4a93b62a06bc4b2d Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Mon, 21 Dec 2020 15:17:52 -0300 Subject: [PATCH 34/45] Adjust grpc.proto to reverting 6aa385e494eb8fa870257c76e078108607503d03 --- proto/src/main/proto/grpc.proto | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/proto/src/main/proto/grpc.proto b/proto/src/main/proto/grpc.proto index 81795112e55..df37f08fd82 100644 --- a/proto/src/main/proto/grpc.proto +++ b/proto/src/main/proto/grpc.proto @@ -273,20 +273,19 @@ message TradeInfo { string takerFeeTxId = 9; string depositTxId = 10; string payoutTxId = 11; - string withdrawalTxId = 12; - uint64 tradeAmountAsLong = 13; - uint64 tradePrice = 14; - string tradingPeerNodeAddress = 15; - string state = 16; - string phase = 17; - string tradePeriodState = 18; - bool isDepositPublished = 19; - bool isDepositConfirmed = 20; - bool isFiatSent = 21; - bool isFiatReceived = 22; - bool isPayoutPublished = 23; - bool isWithdrawn = 24; - string contractAsJson = 25; + uint64 tradeAmountAsLong = 12; + uint64 tradePrice = 13; + string tradingPeerNodeAddress = 14; + string state = 15; + string phase = 16; + string tradePeriodState = 17; + bool isDepositPublished = 18; + bool isDepositConfirmed = 19; + bool isFiatSent = 20; + bool isFiatReceived = 21; + bool isPayoutPublished = 22; + bool isWithdrawn = 23; + string contractAsJson = 24; } /////////////////////////////////////////////////////////////////////////////////////////// From 97dcac2a2d94ce768933c433c5ccfe8d966871f2 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Mon, 21 Dec 2020 15:19:10 -0300 Subject: [PATCH 35/45] Adjust TradeFormat to reverting 6aa385e494eb8fa870257c76e078108607503d03 --- cli/src/main/java/bisq/cli/TradeFormat.java | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/cli/src/main/java/bisq/cli/TradeFormat.java b/cli/src/main/java/bisq/cli/TradeFormat.java index 0c4d4d29498..1c286f03772 100644 --- a/cli/src/main/java/bisq/cli/TradeFormat.java +++ b/cli/src/main/java/bisq/cli/TradeFormat.java @@ -58,7 +58,6 @@ public static String format(TradeInfo tradeInfo) { + COL_HEADER_TRADE_FIAT_RECEIVED + COL_HEADER_DELIMITER + COL_HEADER_TRADE_PAYOUT_PUBLISHED + COL_HEADER_DELIMITER + COL_HEADER_TRADE_WITHDRAWN + COL_HEADER_DELIMITER - + (tradeInfo.getIsWithdrawn() ? COL_HEADER_TRADE_WITHDRAWAL_TX_ID + COL_HEADER_DELIMITER : "") + "%n"; String counterCurrencyCode = tradeInfo.getOffer().getCounterCurrencyCode(); @@ -79,8 +78,7 @@ public static String format(TradeInfo tradeInfo) { + " %-" + COL_HEADER_TRADE_FIAT_SENT.length() + "s" // lt justify + " %-" + COL_HEADER_TRADE_FIAT_RECEIVED.length() + "s" // lt justify + " %-" + COL_HEADER_TRADE_PAYOUT_PUBLISHED.length() + "s" // lt justify - + " %-" + COL_HEADER_TRADE_WITHDRAWN.length() + "s" // lt justify - + " %-" + COL_HEADER_TRADE_WITHDRAWAL_TX_ID.length() + "s"; // left + + " %-" + COL_HEADER_TRADE_WITHDRAWN.length() + "s"; // lt justify return headerLine + (isTaker @@ -100,8 +98,7 @@ private static String formatTradeForMaker(String format, TradeInfo tradeInfo) { tradeInfo.getIsFiatSent() ? "YES" : "NO", tradeInfo.getIsFiatReceived() ? "YES" : "NO", tradeInfo.getIsPayoutPublished() ? "YES" : "NO", - tradeInfo.getIsWithdrawn() ? "YES" : "NO", - tradeInfo.getIsWithdrawn() ? tradeInfo.getWithdrawalTxId() : ""); + tradeInfo.getIsWithdrawn() ? "YES" : "NO"); } private static String formatTradeForTaker(String format, TradeInfo tradeInfo) { @@ -117,7 +114,6 @@ private static String formatTradeForTaker(String format, TradeInfo tradeInfo) { tradeInfo.getIsFiatSent() ? "YES" : "NO", tradeInfo.getIsFiatReceived() ? "YES" : "NO", tradeInfo.getIsPayoutPublished() ? "YES" : "NO", - tradeInfo.getIsWithdrawn() ? "YES" : "NO", - tradeInfo.getIsWithdrawn() ? tradeInfo.getWithdrawalTxId() : ""); + tradeInfo.getIsWithdrawn() ? "YES" : "NO"); } } From 3a770f4bc0ba94297ba03712c06a0e0206f59f19 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Mon, 21 Dec 2020 15:22:05 -0300 Subject: [PATCH 36/45] Adjust TakeSellBTCOfferTest to reverting 6aa385e494eb8fa870257c76e078108607503d03 --- .../method/trade/TakeSellBTCOfferTest.java | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/apitest/src/test/java/bisq/apitest/method/trade/TakeSellBTCOfferTest.java b/apitest/src/test/java/bisq/apitest/method/trade/TakeSellBTCOfferTest.java index cd500cdd091..673792c4f55 100644 --- a/apitest/src/test/java/bisq/apitest/method/trade/TakeSellBTCOfferTest.java +++ b/apitest/src/test/java/bisq/apitest/method/trade/TakeSellBTCOfferTest.java @@ -33,7 +33,6 @@ import static bisq.apitest.config.BisqAppConfig.alicedaemon; import static bisq.apitest.config.BisqAppConfig.bobdaemon; import static bisq.cli.CurrencyFormat.formatSatoshis; -import static bisq.cli.TransactionFormat.format; import static bisq.core.trade.Trade.Phase.*; import static bisq.core.trade.Trade.State.*; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -165,19 +164,4 @@ public void testBobsBtcWithdrawalToExternalAddress(final TestInfo testInfo) { testName(testInfo), formatSatoshis(currentBalance.getAvailableBalance())); } - - @Test - @Order(5) - public void testGetTradeWithdrawalTx(final TestInfo testInfo) { - var trade = getTrade(bobdaemon, tradeId); - var withdrawalTxId = trade.getWithdrawalTxId(); - assertNotNull(withdrawalTxId); - - var txInfo = getTransaction(bobdaemon, withdrawalTxId); - assertEquals(WITHDRAWAL_TX_MEMO, txInfo.getMemo()); - - log.debug("{} Trade withdrawal Tx:\n{}", - testName(testInfo), - format(txInfo)); - } } From 4aa4270ed925e59a09608d4ef411408246c0d406 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Mon, 21 Dec 2020 15:23:23 -0300 Subject: [PATCH 37/45] Adjust TradeTest to reverting 6aa385e494eb8fa870257c76e078108607503d03 --- apitest/src/test/java/bisq/apitest/scenario/TradeTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/apitest/src/test/java/bisq/apitest/scenario/TradeTest.java b/apitest/src/test/java/bisq/apitest/scenario/TradeTest.java index 410b72ca6cb..4c07452abc6 100644 --- a/apitest/src/test/java/bisq/apitest/scenario/TradeTest.java +++ b/apitest/src/test/java/bisq/apitest/scenario/TradeTest.java @@ -60,6 +60,5 @@ public void testTakeSellBTCOffer(final TestInfo testInfo) { test.testBobsConfirmPaymentStarted(testInfo); test.testAlicesConfirmPaymentReceived(testInfo); test.testBobsBtcWithdrawalToExternalAddress(testInfo); - test.testGetTradeWithdrawalTx(testInfo); } } From 1507a2c791294f3e8efaf39f12a62af8f5724f4c Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Mon, 21 Dec 2020 15:31:07 -0300 Subject: [PATCH 38/45] Resolve file conflict w/ master --- .../apitest/method/payment/CreatePaymentAccountTest.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/apitest/src/test/java/bisq/apitest/method/payment/CreatePaymentAccountTest.java b/apitest/src/test/java/bisq/apitest/method/payment/CreatePaymentAccountTest.java index 2fe5ed22abd..fe9daf27df0 100644 --- a/apitest/src/test/java/bisq/apitest/method/payment/CreatePaymentAccountTest.java +++ b/apitest/src/test/java/bisq/apitest/method/payment/CreatePaymentAccountTest.java @@ -65,9 +65,12 @@ import static bisq.apitest.Scaffold.BitcoinCoreApp.bitcoind; import static bisq.apitest.config.BisqAppConfig.alicedaemon; -import static bisq.core.locale.CurrencyUtil.*; +import static bisq.core.locale.CurrencyUtil.getAllAdvancedCashCurrencies; +import static bisq.core.locale.CurrencyUtil.getAllMoneyGramCurrencies; +import static bisq.core.locale.CurrencyUtil.getAllRevolutCurrencies; +import static bisq.core.locale.CurrencyUtil.getAllUpholdCurrencies; import static bisq.core.payment.payload.PaymentMethod.*; -import static org.junit.Assert.assertEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.fail; import static org.junit.jupiter.api.MethodOrderer.OrderAnnotation; @@ -746,7 +749,6 @@ public void testCreateTransferwiseAccount(TestInfo testInfo) { String jsonString = getCompletedFormAsJsonString(); TransferwiseAccount paymentAccount = (TransferwiseAccount) createPaymentAccount(alicedaemon, jsonString); verifyUserPayloadHasPaymentAccountWithId(paymentAccount.getId()); - verifyUserPayloadHasPaymentAccountWithId(paymentAccount.getId()); // As per commit 88f26f93241af698ae689bf081205d0f9dc929fa // Do not autofill all currencies by default but keep all unselected. // verifyAccountTradeCurrencies(getAllTransferwiseCurrencies(), paymentAccount); From 01546ad11d97ca19f283c80a881f073ef40e55bf Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Tue, 22 Dec 2020 16:14:49 -0300 Subject: [PATCH 39/45] Use a simpler, time windowing call rate meter Rewrote the GrpcCallRateMeter class and adjusted afected classes. These changes were requested in PR review https://github.com/bisq-network/bisq/pull/4966#pullrequestreview-557040093 --- .../CallRateMeteringInterceptorTest.java | 4 +- .../CallRateMeteringInterceptor.java | 24 ++--- .../grpc/interceptor/GrpcCallRateMeter.java | 86 +++++++++--------- .../GrpcServiceRateMeteringConfig.java | 2 +- .../GrpcServiceRateMeteringConfigTest.java | 88 +++++++++++-------- 5 files changed, 104 insertions(+), 100 deletions(-) diff --git a/apitest/src/test/java/bisq/apitest/scenario/CallRateMeteringInterceptorTest.java b/apitest/src/test/java/bisq/apitest/scenario/CallRateMeteringInterceptorTest.java index f2013133673..8f85b35dcf8 100644 --- a/apitest/src/test/java/bisq/apitest/scenario/CallRateMeteringInterceptorTest.java +++ b/apitest/src/test/java/bisq/apitest/scenario/CallRateMeteringInterceptorTest.java @@ -78,7 +78,7 @@ public void testGetVersionCall1IsAllowed() { @Order(2) public void testGetVersionCall2ShouldThrowException() { Throwable exception = assertThrows(StatusRuntimeException.class, getVersionTest::testGetVersion); - assertEquals("PERMISSION_DENIED: the maximum allowed number of getversion calls (1/second) has been exceeded by 1 call", + assertEquals("PERMISSION_DENIED: the maximum allowed number of getversion calls (1/second) has been exceeded", exception.getMessage()); } @@ -86,7 +86,7 @@ public void testGetVersionCall2ShouldThrowException() { @Order(3) public void testGetVersionCall3ShouldThrowException() { Throwable exception = assertThrows(StatusRuntimeException.class, getVersionTest::testGetVersion); - assertEquals("PERMISSION_DENIED: the maximum allowed number of getversion calls (1/second) has been exceeded by 2 calls", + assertEquals("PERMISSION_DENIED: the maximum allowed number of getversion calls (1/second) has been exceeded", exception.getMessage()); } diff --git a/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java b/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java index 562f27fb031..917536c525f 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java +++ b/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java @@ -66,14 +66,8 @@ private void checkRateMeterAndMaybeCloseCall(Map.Entry serverCall) getRateMeterKey(serverCall)); } - private void handlePermissionDeniedErrorAndCloseCall(String methodName, - GrpcCallRateMeter rateMeter, - ServerCall serverCall) + private void handlePermissionDeniedWarningAndCloseCall(String methodName, + GrpcCallRateMeter rateMeter, + ServerCall serverCall) throws StatusRuntimeException { String msg = getDefaultRateExceededError(methodName, rateMeter); - log.error(StringUtils.capitalize(msg) + "."); + log.warn(StringUtils.capitalize(msg) + "."); serverCall.close(PERMISSION_DENIED.withDescription(msg), new Metadata()); } @@ -99,12 +93,10 @@ private String getDefaultRateExceededError(String methodName, // The derived method name may not be an exact match to CLI's method name. String timeUnitName = StringUtils.chop(rateMeter.getTimeUnit().name().toLowerCase()); int callCountAboveLimit = rateMeter.getCallsCount() - rateMeter.getAllowedCallsPerTimeUnit(); - return format("the maximum allowed number of %s calls (%d/%s) has been exceeded by %d call%s", + return format("the maximum allowed number of %s calls (%d/%s) has been exceeded", methodName.toLowerCase(), rateMeter.getAllowedCallsPerTimeUnit(), - timeUnitName, - callCountAboveLimit, - callCountAboveLimit == 1 ? "" : "s"); + timeUnitName); } private Optional> getRateMeterKV(ServerCall serverCall) { diff --git a/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java b/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java index ea70dcc16dd..a2c77b80598 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java +++ b/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java @@ -1,75 +1,52 @@ -/* - * This file is part of Bisq. - * - * Bisq is free software: you can redistribute it and/or modify it - * under the terms of the GNU Affero General Public License as published by - * the Free Software Foundation, either version 3 of the License, or (at - * your option) any later version. - * - * Bisq is distributed in the hope that it will be useful, but WITHOUT - * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or - * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public - * License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with Bisq. If not, see . - */ - package bisq.daemon.grpc.interceptor; -import bisq.common.Timer; -import bisq.common.UserThread; - import org.apache.commons.lang3.StringUtils; +import java.util.ArrayDeque; import java.util.concurrent.TimeUnit; +import java.util.function.Predicate; import lombok.Getter; import lombok.extern.slf4j.Slf4j; -import javax.annotation.Nullable; - import static java.lang.String.format; +import static java.lang.System.currentTimeMillis; @Slf4j -public final class GrpcCallRateMeter { +public class GrpcCallRateMeter { @Getter private final int allowedCallsPerTimeUnit; @Getter private final TimeUnit timeUnit; @Getter - private int callsCount = 0; - @Getter - private transient boolean isRunning; + private final long timeUnitIntervalInMilliseconds; + private final ArrayDeque callTimestamps; - @Nullable - private Timer timer; + // The total number of calls made within the current time window. + private int callsCount; public GrpcCallRateMeter(int allowedCallsPerTimeUnit, TimeUnit timeUnit) { this.allowedCallsPerTimeUnit = allowedCallsPerTimeUnit; this.timeUnit = timeUnit; + this.timeUnitIntervalInMilliseconds = timeUnit.toMillis(1); + this.callsCount = 0; + this.callTimestamps = new ArrayDeque<>(); } - public void start() { - stop(); - timer = UserThread.runPeriodically(() -> callsCount = 0, 1, timeUnit); - isRunning = true; - } - - public void stop() { - if (timer != null) - timer.stop(); - - isRunning = false; - } - - public void incrementCallsCount() { - callsCount++; + public boolean isAllowed() { + removeStaleCallTimestamps(); + if (callsCount < allowedCallsPerTimeUnit) { + incrementCallsCount(); + return true; + } else { + return false; + } } - public boolean isCallRateExceeded() { - return callsCount > allowedCallsPerTimeUnit; + public int getCallsCount() { + removeStaleCallTimestamps(); + return callsCount; } public String getCallsCountProgress(String calledMethodName) { @@ -83,13 +60,30 @@ public String getCallsCountProgress(String calledMethodName) { shortTimeUnitName); } + private void incrementCallsCount() { + callTimestamps.add(currentTimeMillis()); + callsCount++; + } + + private void removeStaleCallTimestamps() { + while (!callTimestamps.isEmpty() && isStale.test(callTimestamps.peek())) { + callTimestamps.remove(); + callsCount--; // updates the current time window's call count + } + } + + private final Predicate isStale = (t) -> { + long stale = currentTimeMillis() - this.getTimeUnitIntervalInMilliseconds(); + // Is the given timestamp before the current time minus 1 timeUnit in millis? + return t < stale; + }; + @Override public String toString() { return "GrpcCallRateMeter{" + "allowedCallsPerTimeUnit=" + allowedCallsPerTimeUnit + ", timeUnit=" + timeUnit.name() + ", callsCount=" + callsCount + - ", isRunning=" + isRunning + '}'; } } diff --git a/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfig.java b/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfig.java index 151d1e3d6bb..bde0d42a1e8 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfig.java +++ b/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfig.java @@ -230,7 +230,7 @@ public void addCallRateMeter(String grpcServiceClassName, public File build() { File tmpFile = serializeRateMeterDefinitions(); - File configFile = new File("/tmp/ratemeters.json"); + File configFile = Paths.get(getProperty("java.io.tmpdir"), "ratemeters.json").toFile(); try { deleteFileIfExists(configFile); renameFile(tmpFile, configFile); diff --git a/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java b/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java index 42e98ca1de6..992256fd7fb 100644 --- a/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java +++ b/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java @@ -53,13 +53,14 @@ public class GrpcServiceRateMeteringConfigTest { private static final GrpcServiceRateMeteringConfig.Builder builder = new GrpcServiceRateMeteringConfig.Builder(); private static File configFile; + @SuppressWarnings("OptionalUsedAsFieldOrParameterType") private static Optional versionServiceInterceptor; @BeforeClass public static void setup() { builder.addCallRateMeter(GrpcVersionService.class.getSimpleName(), "getVersion", - 2, + 3, SECONDS); builder.addCallRateMeter(GrpcVersionService.class.getSimpleName(), "getNadaButDoNotBreakAnything", @@ -93,52 +94,68 @@ public void testConfigFileBuild() { assertNotNull(configFile); assertTrue(configFile.exists()); assertTrue(configFile.length() > 0); - String expectedConfigFilePath = Paths.get(getProperty("java.io.tmpdir")) + File.separator + "ratemeters.json"; + String expectedConfigFilePath = Paths.get(getProperty("java.io.tmpdir"), "ratemeters.json").toString(); assertEquals(expectedConfigFilePath, configFile.getAbsolutePath()); } @Test - public void testGrpcVersionServiceRateMeteringConfig() { + public void testGetVersionCallRateMeter() { CallRateMeteringInterceptor versionServiceInterceptor = buildInterceptor(); assertEquals(2, versionServiceInterceptor.serviceCallRateMeters.size()); - GrpcCallRateMeter versionCallRateMeter = versionServiceInterceptor.serviceCallRateMeters.get("getVersion"); - assertFalse(versionCallRateMeter.isRunning()); - assertEquals(2, versionCallRateMeter.getAllowedCallsPerTimeUnit()); - assertEquals(SECONDS, versionCallRateMeter.getTimeUnit()); - assertFalse(versionCallRateMeter.isCallRateExceeded()); - for (int i = 1; i <= 3; i++) { - versionCallRateMeter.incrementCallsCount(); + GrpcCallRateMeter rateMeter = versionServiceInterceptor.serviceCallRateMeters.get("getVersion"); + assertEquals(3, rateMeter.getAllowedCallsPerTimeUnit()); + assertEquals(SECONDS, rateMeter.getTimeUnit()); + + doMaxIsAllowedChecks(true, + rateMeter.getAllowedCallsPerTimeUnit(), + rateMeter); + + // The next 3 getversion calls will be blocked because we've exceeded the limit. + doMaxIsAllowedChecks(false, + rateMeter.getAllowedCallsPerTimeUnit(), + rateMeter); + + // Wait: let all of the rate meter's cached call timestamps to become stale, + // then we can call getversion another 'allowedCallsPerTimeUnit' times. + rest(1 + rateMeter.getTimeUnitIntervalInMilliseconds()); + // All the stale call timestamps are gone and the call count is back to zero. + assertEquals(0, rateMeter.getCallsCount()); + + doMaxIsAllowedChecks(true, + rateMeter.getAllowedCallsPerTimeUnit(), + rateMeter); + // We've exceeded the call/second limit. + assertFalse(rateMeter.isAllowed()); + + // Let all of the call timestamps to go stale again. + rest(1 + rateMeter.getTimeUnitIntervalInMilliseconds()); + + // Call 2x, resting 0.25s after each call. + for (int i = 0; i < 2; i++) { + assertTrue(rateMeter.isAllowed()); + rest(250); } - assertEquals(3, versionCallRateMeter.getCallsCount()); - assertTrue(versionCallRateMeter.isCallRateExceeded()); - } - - @Test - public void testRunningRateMetering() { - CallRateMeteringInterceptor versionServiceInterceptor = buildInterceptor(); - GrpcCallRateMeter versionCallRateMeter = versionServiceInterceptor.serviceCallRateMeters.get("getVersion"); - - versionCallRateMeter.start(); - assertTrue(versionCallRateMeter.isRunning()); + // Call the 3rd time, then let one of the rate meter's timestamps to go stale. + assertTrue(rateMeter.isAllowed()); + rest(510); - // The timer resets the call count to 0 every 1s (the meter's configured timeunit). - // Wait 1.1s to let it do that before bumping the call count and checking state. - rest(1100); + // The call count was decremented by one because one timestamp went stale. + assertEquals(2, rateMeter.getCallsCount()); + assertTrue(rateMeter.isAllowed()); + assertEquals(rateMeter.getAllowedCallsPerTimeUnit(), rateMeter.getCallsCount()); - assertEquals(0, versionCallRateMeter.getCallsCount()); - assertFalse(versionCallRateMeter.isCallRateExceeded()); + // We've exceeded the call limit again. + assertFalse(rateMeter.isAllowed()); + } - // Simulate calling 'getVersion' three times. - for (int i = 1; i <= 3; i++) { - versionCallRateMeter.incrementCallsCount(); + private void doMaxIsAllowedChecks(boolean expectedIsAllowed, + int expectedCallsCount, + GrpcCallRateMeter rateMeter) { + for (int i = 1; i <= rateMeter.getAllowedCallsPerTimeUnit(); i++) { + assertEquals(expectedIsAllowed, rateMeter.isAllowed()); } - - assertEquals(3, versionCallRateMeter.getCallsCount()); - assertTrue(versionCallRateMeter.isCallRateExceeded()); - versionCallRateMeter.stop(); - assertFalse(versionCallRateMeter.isRunning()); - log.debug("Configured {}", versionServiceInterceptor); + assertEquals(expectedCallsCount, rateMeter.getCallsCount()); } @AfterClass @@ -155,6 +172,7 @@ private void rest(long milliseconds) { } private CallRateMeteringInterceptor buildInterceptor() { + //noinspection OptionalAssignedToNull if (versionServiceInterceptor == null) { versionServiceInterceptor = getCustomRateMeteringInterceptor( configFile.getParentFile(), From d61521276fde4aaf50cd9995c5c5a069c7d74a2b Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Tue, 22 Dec 2020 16:30:37 -0300 Subject: [PATCH 40/45] Remove unused local var --- .../daemon/grpc/interceptor/CallRateMeteringInterceptor.java | 1 - 1 file changed, 1 deletion(-) diff --git a/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java b/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java index 917536c525f..191ac0cf6e1 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java +++ b/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java @@ -92,7 +92,6 @@ private String getDefaultRateExceededError(String methodName, GrpcCallRateMeter rateMeter) { // The derived method name may not be an exact match to CLI's method name. String timeUnitName = StringUtils.chop(rateMeter.getTimeUnit().name().toLowerCase()); - int callCountAboveLimit = rateMeter.getCallsCount() - rateMeter.getAllowedCallsPerTimeUnit(); return format("the maximum allowed number of %s calls (%d/%s) has been exceeded", methodName.toLowerCase(), rateMeter.getAllowedCallsPerTimeUnit(), From 0d4ed952e727a2bb86d91c341050c9ac78f2e458 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Tue, 22 Dec 2020 17:00:01 -0300 Subject: [PATCH 41/45] Remove redundant callCount field The size of the timestamp queue is the call count --- .../grpc/interceptor/GrpcCallRateMeter.java | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java b/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java index a2c77b80598..240dcda2ecd 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java +++ b/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java @@ -23,20 +23,16 @@ public class GrpcCallRateMeter { private final long timeUnitIntervalInMilliseconds; private final ArrayDeque callTimestamps; - // The total number of calls made within the current time window. - private int callsCount; - public GrpcCallRateMeter(int allowedCallsPerTimeUnit, TimeUnit timeUnit) { this.allowedCallsPerTimeUnit = allowedCallsPerTimeUnit; this.timeUnit = timeUnit; this.timeUnitIntervalInMilliseconds = timeUnit.toMillis(1); - this.callsCount = 0; this.callTimestamps = new ArrayDeque<>(); } public boolean isAllowed() { removeStaleCallTimestamps(); - if (callsCount < allowedCallsPerTimeUnit) { + if (callTimestamps.size() < allowedCallsPerTimeUnit) { incrementCallsCount(); return true; } else { @@ -46,15 +42,15 @@ public boolean isAllowed() { public int getCallsCount() { removeStaleCallTimestamps(); - return callsCount; + return callTimestamps.size(); } public String getCallsCountProgress(String calledMethodName) { String shortTimeUnitName = StringUtils.chop(timeUnit.name().toLowerCase()); return format("%s has been called %d time%s in the last %s, rate limit is %d/%s", calledMethodName, - callsCount, - callsCount == 1 ? "" : "s", + callTimestamps.size(), + callTimestamps.size() == 1 ? "" : "s", shortTimeUnitName, allowedCallsPerTimeUnit, shortTimeUnitName); @@ -62,13 +58,11 @@ public String getCallsCountProgress(String calledMethodName) { private void incrementCallsCount() { callTimestamps.add(currentTimeMillis()); - callsCount++; } private void removeStaleCallTimestamps() { while (!callTimestamps.isEmpty() && isStale.test(callTimestamps.peek())) { callTimestamps.remove(); - callsCount--; // updates the current time window's call count } } @@ -83,7 +77,7 @@ public String toString() { return "GrpcCallRateMeter{" + "allowedCallsPerTimeUnit=" + allowedCallsPerTimeUnit + ", timeUnit=" + timeUnit.name() + - ", callsCount=" + callsCount + + ", callsCount=" + callTimestamps.size() + '}'; } } From c8ef4141e3b0e47a4ae595632d9aa615efe41e8a Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Tue, 22 Dec 2020 18:33:18 -0300 Subject: [PATCH 42/45] Fix comment --- .../grpc/interceptor/GrpcServiceRateMeteringConfigTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java b/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java index 992256fd7fb..3e179958cc5 100644 --- a/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java +++ b/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java @@ -128,7 +128,7 @@ public void testGetVersionCallRateMeter() { // We've exceeded the call/second limit. assertFalse(rateMeter.isAllowed()); - // Let all of the call timestamps to go stale again. + // Let all of the call timestamps go stale again. rest(1 + rateMeter.getTimeUnitIntervalInMilliseconds()); // Call 2x, resting 0.25s after each call. @@ -136,7 +136,7 @@ public void testGetVersionCallRateMeter() { assertTrue(rateMeter.isAllowed()); rest(250); } - // Call the 3rd time, then let one of the rate meter's timestamps to go stale. + // Call the 3rd time, then let one of the rate meter's timestamps go stale. assertTrue(rateMeter.isAllowed()); rest(510); From 63564760a8bb8be0ec94a423901ee5f4d72887c4 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Tue, 22 Dec 2020 21:11:04 -0300 Subject: [PATCH 43/45] Support more fine grained rate metering We need to be able to define call rate meters for time spans not limited to TimeUnit intervals of 1 SECOND, 1 HOUR, or 1 DAY. This change allows more flexibility, e.g., 10 per 5 seconds, 10 per 5 hrs, 100 per 7 days. --- .../grpc/interceptor/GrpcCallRateMeter.java | 19 +++++++--- .../GrpcServiceRateMeteringConfig.java | 35 ++++++++++++++----- .../GrpcServiceRateMeteringConfigTest.java | 26 ++++++++------ 3 files changed, 57 insertions(+), 23 deletions(-) diff --git a/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java b/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java index 240dcda2ecd..15503f5cfbe 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java +++ b/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcCallRateMeter.java @@ -20,19 +20,27 @@ public class GrpcCallRateMeter { @Getter private final TimeUnit timeUnit; @Getter - private final long timeUnitIntervalInMilliseconds; - private final ArrayDeque callTimestamps; + private final int numTimeUnits; + + @Getter + private transient final long timeUnitIntervalInMilliseconds; + + private transient final ArrayDeque callTimestamps; public GrpcCallRateMeter(int allowedCallsPerTimeUnit, TimeUnit timeUnit) { + this(allowedCallsPerTimeUnit, timeUnit, 1); + } + + public GrpcCallRateMeter(int allowedCallsPerTimeUnit, TimeUnit timeUnit, int numTimeUnits) { this.allowedCallsPerTimeUnit = allowedCallsPerTimeUnit; this.timeUnit = timeUnit; - this.timeUnitIntervalInMilliseconds = timeUnit.toMillis(1); + this.numTimeUnits = numTimeUnits; + this.timeUnitIntervalInMilliseconds = timeUnit.toMillis(1) * numTimeUnits; this.callTimestamps = new ArrayDeque<>(); } public boolean isAllowed() { - removeStaleCallTimestamps(); - if (callTimestamps.size() < allowedCallsPerTimeUnit) { + if (getCallsCount() < allowedCallsPerTimeUnit) { incrementCallsCount(); return true; } else { @@ -77,6 +85,7 @@ public String toString() { return "GrpcCallRateMeter{" + "allowedCallsPerTimeUnit=" + allowedCallsPerTimeUnit + ", timeUnit=" + timeUnit.name() + + ", timeUnitIntervalInMilliseconds=" + timeUnitIntervalInMilliseconds + ", callsCount=" + callTimestamps.size() + '}'; } diff --git a/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfig.java b/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfig.java index bde0d42a1e8..245feacaa29 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfig.java +++ b/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfig.java @@ -24,8 +24,6 @@ import com.google.common.annotations.VisibleForTesting; -import org.apache.commons.lang3.StringUtils; - import java.nio.file.Paths; import java.io.File; @@ -61,6 +59,7 @@ public class GrpcServiceRateMeteringConfig { private static final String KEY_METHOD_RATE_METERS = "methodRateMeters"; private static final String KEY_ALLOWED_CALL_PER_TIME_UNIT = "allowedCallsPerTimeUnit"; private static final String KEY_TIME_UNIT = "timeUnit"; + private static final String KEY_NUM_TIME_UNITS = "numTimeUnits"; private static final Gson gson = new GsonBuilder().setPrettyPrinting().create(); @@ -80,8 +79,15 @@ public GrpcServiceRateMeteringConfig(String grpcServiceClassName, public GrpcServiceRateMeteringConfig addMethodCallRateMeter(String methodName, int maxCalls, TimeUnit timeUnit) { + return addMethodCallRateMeter(methodName, maxCalls, timeUnit, 1); + } + + public GrpcServiceRateMeteringConfig addMethodCallRateMeter(String methodName, + int maxCalls, + TimeUnit timeUnit, + int numTimeUnits) { methodRateMeters.add(new LinkedHashMap<>() {{ - put(methodName, new GrpcCallRateMeter(maxCalls, timeUnit)); + put(methodName, new GrpcCallRateMeter(maxCalls, timeUnit, numTimeUnits)); }}); return this; } @@ -178,7 +184,8 @@ private static GrpcCallRateMeter getGrpcCallRateMeter(Map.Entry Map valueMap = (Map) gsonEntry.getValue(); int allowedCallsPerTimeUnit = ((Number) valueMap.get(KEY_ALLOWED_CALL_PER_TIME_UNIT)).intValue(); TimeUnit timeUnit = TimeUnit.valueOf((String) valueMap.get(KEY_TIME_UNIT)); - return new GrpcCallRateMeter(allowedCallsPerTimeUnit, timeUnit); + int numTimeUnits = ((Number) valueMap.get(KEY_NUM_TIME_UNITS)).intValue(); + return new GrpcCallRateMeter(allowedCallsPerTimeUnit, timeUnit, numTimeUnits); } private static void verifyConfigFile(File configFile) { @@ -216,16 +223,28 @@ public void addCallRateMeter(String grpcServiceClassName, String methodName, int maxCalls, TimeUnit timeUnit) { - log.info("Adding call rate metering definition {}.{} ({}/{}).", + addCallRateMeter(grpcServiceClassName, + methodName, + maxCalls, + timeUnit, + 1); + } + + public void addCallRateMeter(String grpcServiceClassName, + String methodName, + int maxCalls, + TimeUnit timeUnit, + int numTimeUnits) { + log.info("Adding call rate metering definition {}.{} ({}/{}ms).", grpcServiceClassName, methodName, maxCalls, - StringUtils.chop(timeUnit.name().toLowerCase())); + timeUnit.toMillis(1) * numTimeUnits); rateMeterConfigs.stream().filter(c -> c.isConfigForGrpcService(grpcServiceClassName)) .findFirst().ifPresentOrElse( - (config) -> config.addMethodCallRateMeter(methodName, maxCalls, timeUnit), + (config) -> config.addMethodCallRateMeter(methodName, maxCalls, timeUnit, numTimeUnits), () -> rateMeterConfigs.add(new GrpcServiceRateMeteringConfig(grpcServiceClassName) - .addMethodCallRateMeter(methodName, maxCalls, timeUnit))); + .addMethodCallRateMeter(methodName, maxCalls, timeUnit, numTimeUnits))); } public File build() { diff --git a/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java b/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java index 3e179958cc5..36d5d48f563 100644 --- a/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java +++ b/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java @@ -58,15 +58,16 @@ public class GrpcServiceRateMeteringConfigTest { @BeforeClass public static void setup() { + // This is the tested rate meter, it allows 3 calls every 2 seconds. builder.addCallRateMeter(GrpcVersionService.class.getSimpleName(), "getVersion", 3, - SECONDS); + SECONDS, + 2); builder.addCallRateMeter(GrpcVersionService.class.getSimpleName(), - "getNadaButDoNotBreakAnything", + "badMethodNameDoesNotBreakAnything", 100, DAYS); - // The other Grpc*Service classes are not @VisibleForTesting, so we hardcode // the simple class name. builder.addCallRateMeter("GrpcOffersService", @@ -100,24 +101,29 @@ public void testConfigFileBuild() { @Test public void testGetVersionCallRateMeter() { + // Check the interceptor has 2 rate meters, for getVersion and badMethodNameDoesNotBreakAnything. CallRateMeteringInterceptor versionServiceInterceptor = buildInterceptor(); assertEquals(2, versionServiceInterceptor.serviceCallRateMeters.size()); + // Check the rate meter config. GrpcCallRateMeter rateMeter = versionServiceInterceptor.serviceCallRateMeters.get("getVersion"); assertEquals(3, rateMeter.getAllowedCallsPerTimeUnit()); assertEquals(SECONDS, rateMeter.getTimeUnit()); + assertEquals(2, rateMeter.getNumTimeUnits()); + assertEquals(2 * 1000, rateMeter.getTimeUnitIntervalInMilliseconds()); + // Do as many calls as allowed within rateMeter.getTimeUnitIntervalInMilliseconds(). doMaxIsAllowedChecks(true, rateMeter.getAllowedCallsPerTimeUnit(), rateMeter); - // The next 3 getversion calls will be blocked because we've exceeded the limit. + // The next 3 calls are blocked because we've exceeded the 3calls/2s limit. doMaxIsAllowedChecks(false, rateMeter.getAllowedCallsPerTimeUnit(), rateMeter); - // Wait: let all of the rate meter's cached call timestamps to become stale, - // then we can call getversion another 'allowedCallsPerTimeUnit' times. + // Let all of the rate meter's cached call timestamps become stale by waiting for + // 2001 ms, then we can call getversion another 'allowedCallsPerTimeUnit' times. rest(1 + rateMeter.getTimeUnitIntervalInMilliseconds()); // All the stale call timestamps are gone and the call count is back to zero. assertEquals(0, rateMeter.getCallsCount()); @@ -128,17 +134,17 @@ public void testGetVersionCallRateMeter() { // We've exceeded the call/second limit. assertFalse(rateMeter.isAllowed()); - // Let all of the call timestamps go stale again. + // Let all of the call timestamps go stale again by waiting for 2001 ms. rest(1 + rateMeter.getTimeUnitIntervalInMilliseconds()); - // Call 2x, resting 0.25s after each call. + // Call twice, resting 0.5s after each call. for (int i = 0; i < 2; i++) { assertTrue(rateMeter.isAllowed()); - rest(250); + rest(500); } // Call the 3rd time, then let one of the rate meter's timestamps go stale. assertTrue(rateMeter.isAllowed()); - rest(510); + rest(1001); // The call count was decremented by one because one timestamp went stale. assertEquals(2, rateMeter.getCallsCount()); From b8c5a2965938096dd25cca64457e4528f3e3d92c Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Tue, 22 Dec 2020 22:03:06 -0300 Subject: [PATCH 44/45] Disable CallRateMeteringInterceptorTest and run it from test suite This will reduce the entire apitest suite's exec time --- .../CallRateMeteringInterceptorTest.java | 9 +++---- .../bisq/apitest/scenario/StartupTest.java | 26 +++++++++++++++++-- 2 files changed, 28 insertions(+), 7 deletions(-) rename apitest/src/test/java/bisq/apitest/{scenario => method}/CallRateMeteringInterceptorTest.java (96%) diff --git a/apitest/src/test/java/bisq/apitest/scenario/CallRateMeteringInterceptorTest.java b/apitest/src/test/java/bisq/apitest/method/CallRateMeteringInterceptorTest.java similarity index 96% rename from apitest/src/test/java/bisq/apitest/scenario/CallRateMeteringInterceptorTest.java rename to apitest/src/test/java/bisq/apitest/method/CallRateMeteringInterceptorTest.java index 8f85b35dcf8..b73806ca57d 100644 --- a/apitest/src/test/java/bisq/apitest/scenario/CallRateMeteringInterceptorTest.java +++ b/apitest/src/test/java/bisq/apitest/method/CallRateMeteringInterceptorTest.java @@ -15,7 +15,7 @@ * along with Bisq. If not, see . */ -package bisq.apitest.scenario; +package bisq.apitest.method; import io.grpc.StatusRuntimeException; @@ -26,6 +26,7 @@ import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.MethodOrderer; import org.junit.jupiter.api.Order; import org.junit.jupiter.api.Test; @@ -42,12 +43,10 @@ -import bisq.apitest.method.GetVersionTest; -import bisq.apitest.method.MethodTest; import bisq.daemon.grpc.GrpcVersionService; import bisq.daemon.grpc.interceptor.GrpcServiceRateMeteringConfig; - +@Disabled @Slf4j @TestMethodOrder(MethodOrderer.OrderAnnotation.class) public class CallRateMeteringInterceptorTest extends MethodTest { @@ -102,7 +101,7 @@ public static void tearDown() { tearDownScaffold(); } - private static File buildInterceptorConfigFile() { + public static File buildInterceptorConfigFile() { GrpcServiceRateMeteringConfig.Builder builder = new GrpcServiceRateMeteringConfig.Builder(); builder.addCallRateMeter(GrpcVersionService.class.getSimpleName(), "getVersion", diff --git a/apitest/src/test/java/bisq/apitest/scenario/StartupTest.java b/apitest/src/test/java/bisq/apitest/scenario/StartupTest.java index 26a95f3c773..5c017e8e4f8 100644 --- a/apitest/src/test/java/bisq/apitest/scenario/StartupTest.java +++ b/apitest/src/test/java/bisq/apitest/scenario/StartupTest.java @@ -17,6 +17,8 @@ package bisq.apitest.scenario; +import java.io.File; + import lombok.extern.slf4j.Slf4j; import org.junit.jupiter.api.AfterAll; @@ -30,10 +32,12 @@ import static bisq.apitest.config.BisqAppConfig.alicedaemon; import static bisq.apitest.config.BisqAppConfig.arbdaemon; import static bisq.apitest.config.BisqAppConfig.seednode; +import static bisq.apitest.method.CallRateMeteringInterceptorTest.buildInterceptorConfigFile; import static org.junit.jupiter.api.Assertions.fail; +import bisq.apitest.method.CallRateMeteringInterceptorTest; import bisq.apitest.method.GetVersionTest; import bisq.apitest.method.MethodTest; import bisq.apitest.method.RegisterDisputeAgentsTest; @@ -46,7 +50,11 @@ public class StartupTest extends MethodTest { @BeforeAll public static void setUp() { try { - setUpScaffold(bitcoind, seednode, arbdaemon, alicedaemon); + File callRateMeteringConfigFile = buildInterceptorConfigFile(); + startSupportingApps(callRateMeteringConfigFile, + false, + false, + bitcoind, seednode, arbdaemon, alicedaemon); } catch (Exception ex) { fail(ex); } @@ -54,13 +62,27 @@ public static void setUp() { @Test @Order(1) + public void testCallRateMeteringInterceptor() { + CallRateMeteringInterceptorTest test = new CallRateMeteringInterceptorTest(); + test.testGetVersionCall1IsAllowed(); + test.sleep200Milliseconds(); + test.testGetVersionCall2ShouldThrowException(); + test.sleep200Milliseconds(); + test.testGetVersionCall3ShouldThrowException(); + test.sleep200Milliseconds(); + test.testGetVersionCall4IsAllowed(); + sleep(1000); // Wait 1 second before calling getversion in next test. + } + + @Test + @Order(2) public void testGetVersion() { GetVersionTest test = new GetVersionTest(); test.testGetVersion(); } @Test - @Order(2) + @Order(3) public void testRegisterDisputeAgents() { RegisterDisputeAgentsTest test = new RegisterDisputeAgentsTest(); test.testRegisterArbitratorShouldThrowException(); From 10727fc0830330e004351cd0567b994173a5d5c5 Mon Sep 17 00:00:00 2001 From: ghubstan <36207203+ghubstan@users.noreply.github.com> Date: Wed, 23 Dec 2020 11:30:16 -0300 Subject: [PATCH 45/45] Fix GrpcCallRateMeter method and variable name - Change method isAllowed() -> checkAndIncrement(). - Change variable allowedCallsPerTimeUnit -> allowedCallsPerTimeWindow. --- .../CallRateMeteringInterceptor.java | 4 ++-- .../grpc/interceptor/GrpcCallRateMeter.java | 18 +++++++------- .../GrpcServiceRateMeteringConfig.java | 6 ++--- .../GrpcServiceRateMeteringConfigTest.java | 24 +++++++++---------- 4 files changed, 26 insertions(+), 26 deletions(-) diff --git a/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java b/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java index 191ac0cf6e1..8cd7e1edeb0 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java +++ b/daemon/src/main/java/bisq/daemon/grpc/interceptor/CallRateMeteringInterceptor.java @@ -66,7 +66,7 @@ private void checkRateMeterAndMaybeCloseCall(Map.Entry callTimestamps; - public GrpcCallRateMeter(int allowedCallsPerTimeUnit, TimeUnit timeUnit) { - this(allowedCallsPerTimeUnit, timeUnit, 1); + public GrpcCallRateMeter(int allowedCallsPerTimeWindow, TimeUnit timeUnit) { + this(allowedCallsPerTimeWindow, timeUnit, 1); } - public GrpcCallRateMeter(int allowedCallsPerTimeUnit, TimeUnit timeUnit, int numTimeUnits) { - this.allowedCallsPerTimeUnit = allowedCallsPerTimeUnit; + public GrpcCallRateMeter(int allowedCallsPerTimeWindow, TimeUnit timeUnit, int numTimeUnits) { + this.allowedCallsPerTimeWindow = allowedCallsPerTimeWindow; this.timeUnit = timeUnit; this.numTimeUnits = numTimeUnits; this.timeUnitIntervalInMilliseconds = timeUnit.toMillis(1) * numTimeUnits; this.callTimestamps = new ArrayDeque<>(); } - public boolean isAllowed() { - if (getCallsCount() < allowedCallsPerTimeUnit) { + public boolean checkAndIncrement() { + if (getCallsCount() < allowedCallsPerTimeWindow) { incrementCallsCount(); return true; } else { @@ -60,7 +60,7 @@ public String getCallsCountProgress(String calledMethodName) { callTimestamps.size(), callTimestamps.size() == 1 ? "" : "s", shortTimeUnitName, - allowedCallsPerTimeUnit, + allowedCallsPerTimeWindow, shortTimeUnitName); } @@ -83,7 +83,7 @@ private void removeStaleCallTimestamps() { @Override public String toString() { return "GrpcCallRateMeter{" + - "allowedCallsPerTimeUnit=" + allowedCallsPerTimeUnit + + "allowedCallsPerTimeWindow=" + allowedCallsPerTimeWindow + ", timeUnit=" + timeUnit.name() + ", timeUnitIntervalInMilliseconds=" + timeUnitIntervalInMilliseconds + ", callsCount=" + callTimestamps.size() + diff --git a/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfig.java b/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfig.java index 245feacaa29..8c6297022f9 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfig.java +++ b/daemon/src/main/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfig.java @@ -57,7 +57,7 @@ public class GrpcServiceRateMeteringConfig { private static final String KEY_GRPC_SERVICE_CLASS_NAME = "grpcServiceClassName"; private static final String KEY_METHOD_RATE_METERS = "methodRateMeters"; - private static final String KEY_ALLOWED_CALL_PER_TIME_UNIT = "allowedCallsPerTimeUnit"; + private static final String KEY_ALLOWED_CALL_PER_TIME_WINDOW = "allowedCallsPerTimeWindow"; private static final String KEY_TIME_UNIT = "timeUnit"; private static final String KEY_NUM_TIME_UNITS = "numTimeUnits"; @@ -182,10 +182,10 @@ public static List deserialize(File configFile) { @SuppressWarnings("unchecked") private static GrpcCallRateMeter getGrpcCallRateMeter(Map.Entry gsonEntry) { Map valueMap = (Map) gsonEntry.getValue(); - int allowedCallsPerTimeUnit = ((Number) valueMap.get(KEY_ALLOWED_CALL_PER_TIME_UNIT)).intValue(); + int allowedCallsPerTimeWindow = ((Number) valueMap.get(KEY_ALLOWED_CALL_PER_TIME_WINDOW)).intValue(); TimeUnit timeUnit = TimeUnit.valueOf((String) valueMap.get(KEY_TIME_UNIT)); int numTimeUnits = ((Number) valueMap.get(KEY_NUM_TIME_UNITS)).intValue(); - return new GrpcCallRateMeter(allowedCallsPerTimeUnit, timeUnit, numTimeUnits); + return new GrpcCallRateMeter(allowedCallsPerTimeWindow, timeUnit, numTimeUnits); } private static void verifyConfigFile(File configFile) { diff --git a/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java b/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java index 36d5d48f563..a6b635eaab3 100644 --- a/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java +++ b/daemon/src/test/java/bisq/daemon/grpc/interceptor/GrpcServiceRateMeteringConfigTest.java @@ -107,19 +107,19 @@ public void testGetVersionCallRateMeter() { // Check the rate meter config. GrpcCallRateMeter rateMeter = versionServiceInterceptor.serviceCallRateMeters.get("getVersion"); - assertEquals(3, rateMeter.getAllowedCallsPerTimeUnit()); + assertEquals(3, rateMeter.getAllowedCallsPerTimeWindow()); assertEquals(SECONDS, rateMeter.getTimeUnit()); assertEquals(2, rateMeter.getNumTimeUnits()); assertEquals(2 * 1000, rateMeter.getTimeUnitIntervalInMilliseconds()); // Do as many calls as allowed within rateMeter.getTimeUnitIntervalInMilliseconds(). doMaxIsAllowedChecks(true, - rateMeter.getAllowedCallsPerTimeUnit(), + rateMeter.getAllowedCallsPerTimeWindow(), rateMeter); // The next 3 calls are blocked because we've exceeded the 3calls/2s limit. doMaxIsAllowedChecks(false, - rateMeter.getAllowedCallsPerTimeUnit(), + rateMeter.getAllowedCallsPerTimeWindow(), rateMeter); // Let all of the rate meter's cached call timestamps become stale by waiting for @@ -129,37 +129,37 @@ public void testGetVersionCallRateMeter() { assertEquals(0, rateMeter.getCallsCount()); doMaxIsAllowedChecks(true, - rateMeter.getAllowedCallsPerTimeUnit(), + rateMeter.getAllowedCallsPerTimeWindow(), rateMeter); // We've exceeded the call/second limit. - assertFalse(rateMeter.isAllowed()); + assertFalse(rateMeter.checkAndIncrement()); // Let all of the call timestamps go stale again by waiting for 2001 ms. rest(1 + rateMeter.getTimeUnitIntervalInMilliseconds()); // Call twice, resting 0.5s after each call. for (int i = 0; i < 2; i++) { - assertTrue(rateMeter.isAllowed()); + assertTrue(rateMeter.checkAndIncrement()); rest(500); } // Call the 3rd time, then let one of the rate meter's timestamps go stale. - assertTrue(rateMeter.isAllowed()); + assertTrue(rateMeter.checkAndIncrement()); rest(1001); // The call count was decremented by one because one timestamp went stale. assertEquals(2, rateMeter.getCallsCount()); - assertTrue(rateMeter.isAllowed()); - assertEquals(rateMeter.getAllowedCallsPerTimeUnit(), rateMeter.getCallsCount()); + assertTrue(rateMeter.checkAndIncrement()); + assertEquals(rateMeter.getAllowedCallsPerTimeWindow(), rateMeter.getCallsCount()); // We've exceeded the call limit again. - assertFalse(rateMeter.isAllowed()); + assertFalse(rateMeter.checkAndIncrement()); } private void doMaxIsAllowedChecks(boolean expectedIsAllowed, int expectedCallsCount, GrpcCallRateMeter rateMeter) { - for (int i = 1; i <= rateMeter.getAllowedCallsPerTimeUnit(); i++) { - assertEquals(expectedIsAllowed, rateMeter.isAllowed()); + for (int i = 1; i <= rateMeter.getAllowedCallsPerTimeWindow(); i++) { + assertEquals(expectedIsAllowed, rateMeter.checkAndIncrement()); } assertEquals(expectedCallsCount, rateMeter.getCallsCount()); }