From 961703ecea62df62946ab001ec28205662688ccd Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Wed, 9 Sep 2020 16:21:40 -0500 Subject: [PATCH] Review of API code and added some suggestions for code change and comments. More details is easier in a direct conversation... --- cli/src/main/java/bisq/cli/CliMain.java | 40 +++++++--- core/src/main/java/bisq/core/api/CoreApi.java | 7 +- .../core/api/CorePaymentAccountsService.java | 73 ++++++++++++++++--- .../bisq/core/api/CoreWalletsService.java | 52 ++++++++----- .../bisq/core/btc/wallet/WalletService.java | 4 + core/src/main/java/bisq/core/user/User.java | 10 +++ .../java/bisq/daemon/app/BisqDaemonMain.java | 14 +++- .../grpc/GetTradeStatisticsService.java | 55 ++++++++++++++ .../bisq/daemon/grpc/GetVersionService.java | 46 ++++++++++++ .../bisq/daemon/grpc/GrpcOffersService.java | 4 +- .../grpc/GrpcPaymentAccountsService.java | 6 +- .../java/bisq/daemon/grpc/GrpcServer.java | 53 ++++---------- .../bisq/daemon/grpc/GrpcWalletsService.java | 10 ++- .../daemon/grpc/PasswordAuthInterceptor.java | 13 +++- proto/src/main/proto/grpc.proto | 38 +++++++++- 15 files changed, 328 insertions(+), 97 deletions(-) create mode 100644 daemon/src/main/java/bisq/daemon/grpc/GetTradeStatisticsService.java create mode 100644 daemon/src/main/java/bisq/daemon/grpc/GetVersionService.java diff --git a/cli/src/main/java/bisq/cli/CliMain.java b/cli/src/main/java/bisq/cli/CliMain.java index f229fcd4538..fb067790245 100644 --- a/cli/src/main/java/bisq/cli/CliMain.java +++ b/cli/src/main/java/bisq/cli/CliMain.java @@ -58,6 +58,14 @@ @Slf4j public class CliMain { + /* + * TODO enums follow code style of constants + * "Because they are constants, the names of an enum type's fields are in uppercase letters." + * https://docs.oracle.com/javase/tutorial/java/javaOO/enum.html + * + * I see that you map the cli commands to the enum and stuyle of cli is all lowercase but I think we should use a + * mapper to get less dependency on cli-world with java-world. + */ private enum Method { getoffers, createpaymentacct, @@ -72,6 +80,13 @@ private enum Method { setwalletpassword } + private static Method getMethodFromCmd(String methodName) { + // TODO if we use const type for enum we need add some mapping + // Even if we don't change now it is handy to have flexibility in case we change internal code and don't want + // to break user commands + return Method.valueOf(methodName.toLowerCase()); + } + public static void main(String[] args) { try { run(args); @@ -114,9 +129,9 @@ public static void run(String[] args) { } var methodName = nonOptionArgs.get(0); - final Method method; + Method method; try { - method = Method.valueOf(methodName); + method = getMethodFromCmd(methodName); } catch (IllegalArgumentException ex) { throw new IllegalArgumentException(format("'%s' is not a supported method", methodName)); } @@ -149,7 +164,7 @@ public static void run(String[] args) { return; } case getaddressbalance: { - if (nonOptionArgs.size() < 2) + if (nonOptionArgs.size() < 2) // TODO should the args check be more strict with '!=2' ? throw new IllegalArgumentException("no address specified"); var request = GetAddressBalanceRequest.newBuilder() @@ -169,29 +184,34 @@ public static void run(String[] args) { throw new IllegalArgumentException("incorrect parameter count, expecting direction (buy|sell), currency code"); var direction = nonOptionArgs.get(1); - var fiatCurrency = nonOptionArgs.get(2); + var currencyCode = nonOptionArgs.get(2); var request = GetOffersRequest.newBuilder() .setDirection(direction) - .setFiatCurrencyCode(fiatCurrency) + .setCurrencyCode(currencyCode) .build(); var reply = offersService.getOffers(request); - out.println(formatOfferTable(reply.getOffersList(), fiatCurrency)); + out.println(formatOfferTable(reply.getOffersList(), currencyCode)); return; } case createpaymentacct: { + /* + * Will require much more work to support diff. account types... + */ if (nonOptionArgs.size() < 4) throw new IllegalArgumentException( "incorrect parameter count, expecting account name, account number, currency code"); - var accountName = nonOptionArgs.get(1); - var accountNumber = nonOptionArgs.get(2); - var fiatCurrencyCode = nonOptionArgs.get(3); + var paymentMethodId = nonOptionArgs.get(1).toUpperCase(); + var accountName = nonOptionArgs.get(2); + var accountNumber = nonOptionArgs.get(3); + var currencyCode = nonOptionArgs.get(4).toUpperCase(); var request = CreatePaymentAccountRequest.newBuilder() + .setPaymentMethodId(paymentMethodId) .setAccountName(accountName) .setAccountNumber(accountNumber) - .setFiatCurrencyCode(fiatCurrencyCode).build(); + .setCurrencyCode(currencyCode).build(); paymentAccountsService.createPaymentAccount(request); out.println(format("payment account %s saved", accountName)); return; diff --git a/core/src/main/java/bisq/core/api/CoreApi.java b/core/src/main/java/bisq/core/api/CoreApi.java index 30cedeb3b93..78686a8e23e 100644 --- a/core/src/main/java/bisq/core/api/CoreApi.java +++ b/core/src/main/java/bisq/core/api/CoreApi.java @@ -127,8 +127,11 @@ public void createOffer(String offerId, // PaymentAccounts /////////////////////////////////////////////////////////////////////////////////////////// - public void createPaymentAccount(String accountName, String accountNumber, String fiatCurrencyCode) { - paymentAccountsService.createPaymentAccount(accountName, accountNumber, fiatCurrencyCode); + public void createPaymentAccount(String paymentMethodId, + String accountName, + String accountNumber, + String currencyCode) { + paymentAccountsService.createPaymentAccount(paymentMethodId, accountName, accountNumber, currencyCode); } public Set getPaymentAccounts() { diff --git a/core/src/main/java/bisq/core/api/CorePaymentAccountsService.java b/core/src/main/java/bisq/core/api/CorePaymentAccountsService.java index f40c56c20d7..48fdb4eaa30 100644 --- a/core/src/main/java/bisq/core/api/CorePaymentAccountsService.java +++ b/core/src/main/java/bisq/core/api/CorePaymentAccountsService.java @@ -33,6 +33,8 @@ import lombok.extern.slf4j.Slf4j; +import static com.google.common.base.Preconditions.checkNotNull; + @Slf4j class CorePaymentAccountsService { @@ -49,17 +51,11 @@ public CorePaymentAccountsService(Config config, this.user = user; } - public void createPaymentAccount(String accountName, String accountNumber, String fiatCurrencyCode) { - // Create and persist a PerfectMoney dummy payment account. There is no guard - // against creating accounts with duplicate names & numbers, only the uuid and - // creation date are unique. - PaymentMethod dummyPaymentMethod = PaymentMethod.getDummyPaymentMethod(PaymentMethod.PERFECT_MONEY_ID); - PaymentAccount paymentAccount = PaymentAccountFactory.getPaymentAccount(dummyPaymentMethod); - paymentAccount.init(); - paymentAccount.setAccountName(accountName); - ((PerfectMoneyAccount) paymentAccount).setAccountNr(accountNumber); - paymentAccount.setSingleTradeCurrency(new FiatCurrency(fiatCurrencyCode.toUpperCase())); - user.addPaymentAccount(paymentAccount); + void createPaymentAccount(String paymentMethodId, String accountName, String accountNumber, String currencyCode) { + PaymentAccount paymentAccount = getNewPaymentAccount(paymentMethodId, accountName, accountNumber, currencyCode); + + //TODO not sure if there is more to do at account creation. Need to check all the flow when its created from UI + user.addPaymentAccountIfNotExists(paymentAccount); // Don't do this on mainnet until thoroughly tested. if (config.baseCurrencyNetwork.isRegtest()) @@ -68,6 +64,61 @@ public void createPaymentAccount(String accountName, String accountNumber, Strin log.info("Payment account {} saved", paymentAccount.getId()); } + private PaymentAccount getNewPaymentAccount(String paymentMethodId, + String accountName, + String accountNumber, + String currencyCode) { + PaymentAccount paymentAccount = null; + PaymentMethod paymentMethod = PaymentMethod.getPaymentMethodById(paymentMethodId); + switch (paymentMethod.getId()) { + case PaymentMethod.UPHOLD_ID: + case PaymentMethod.MONEY_BEAM_ID: + case PaymentMethod.POPMONEY_ID: + case PaymentMethod.REVOLUT_ID: + log.error("PaymentMethod {} not supported yet: ", paymentMethod); + break; + case PaymentMethod.PERFECT_MONEY_ID: + // Create and persist a PerfectMoney dummy payment account. There is no guard + // against creating accounts with duplicate names & numbers, only the uuid and + // creation date are unique. + + paymentAccount = PaymentAccountFactory.getPaymentAccount(paymentMethod); + paymentAccount.init(); + paymentAccount.setAccountName(accountName); + ((PerfectMoneyAccount) paymentAccount).setAccountNr(accountNumber); + paymentAccount.setSingleTradeCurrency(new FiatCurrency(currencyCode)); + break; + case PaymentMethod.SEPA_ID: + case PaymentMethod.SEPA_INSTANT_ID: + case PaymentMethod.FASTER_PAYMENTS_ID: + case PaymentMethod.NATIONAL_BANK_ID: + case PaymentMethod.SAME_BANK_ID: + case PaymentMethod.SPECIFIC_BANKS_ID: + case PaymentMethod.JAPAN_BANK_ID: + case PaymentMethod.ALI_PAY_ID: + case PaymentMethod.WECHAT_PAY_ID: + case PaymentMethod.SWISH_ID: + case PaymentMethod.CLEAR_X_CHANGE_ID: + case PaymentMethod.CHASE_QUICK_PAY_ID: + case PaymentMethod.INTERAC_E_TRANSFER_ID: + case PaymentMethod.US_POSTAL_MONEY_ORDER_ID: + case PaymentMethod.MONEY_GRAM_ID: + case PaymentMethod.WESTERN_UNION_ID: + case PaymentMethod.CASH_DEPOSIT_ID: + case PaymentMethod.HAL_CASH_ID: + case PaymentMethod.F2F_ID: + case PaymentMethod.PROMPT_PAY_ID: + case PaymentMethod.ADVANCED_CASH_ID: + default: + log.error("PaymentMethod {} not supported yet: ", paymentMethod); + break; + } + + checkNotNull(paymentAccount, + "Could not create payment account with paymentMethodId " + paymentMethodId); + return paymentAccount; + } + public Set getPaymentAccounts() { return user.getPaymentAccounts(); } diff --git a/core/src/main/java/bisq/core/api/CoreWalletsService.java b/core/src/main/java/bisq/core/api/CoreWalletsService.java index 10feea5b8e6..b765cb992cb 100644 --- a/core/src/main/java/bisq/core/api/CoreWalletsService.java +++ b/core/src/main/java/bisq/core/api/CoreWalletsService.java @@ -49,6 +49,10 @@ import static java.lang.String.format; import static java.util.concurrent.TimeUnit.SECONDS; +// I have the feeling we are doing too much here and will end up with a different impl as it is done in +// BtcWalletService. There is a lot of complexity (and old code). I don't feel confident to review that without much +// more time and would prefer that we take the approach to refactor the existing core classes to be more usable for the +// needs of the API instead adding domain logic here. @Slf4j class CoreWalletsService { @@ -71,7 +75,7 @@ public CoreWalletsService(Balances balances, this.btcWalletService = btcWalletService; } - public long getAvailableBalance() { + long getAvailableBalance() { verifyWalletsAreAvailable(); verifyEncryptedWalletIsUnlocked(); @@ -82,40 +86,39 @@ public long getAvailableBalance() { return balance.getValue(); } - public long getAddressBalance(String addressString) { - Address address = getAddressEntry(addressString).getAddress(); + long getAddressBalance(String addressString) { + Address address = btcWalletService.getAddress(addressString); return btcWalletService.getBalanceForAddress(address).value; } - public AddressBalanceInfo getAddressBalanceInfo(String addressString) { + AddressBalanceInfo getAddressBalanceInfo(String addressString) { var satoshiBalance = getAddressBalance(addressString); var numConfirmations = getNumConfirmationsForMostRecentTransaction(addressString); return new AddressBalanceInfo(addressString, satoshiBalance, numConfirmations); } - public List getFundingAddresses() { + List getFundingAddresses() { verifyWalletsAreAvailable(); verifyEncryptedWalletIsUnlocked(); // Create a new funding address if none exists. - if (btcWalletService.getAvailableAddressEntries().size() == 0) + if (btcWalletService.getAvailableAddressEntries().isEmpty()) { btcWalletService.getFreshAddressEntry(); + } - List addressStrings = - btcWalletService - .getAvailableAddressEntries() - .stream() - .map(AddressEntry::getAddressString) - .collect(Collectors.toList()); + List addressStrings = btcWalletService + .getAvailableAddressEntries() + .stream() + .map(AddressEntry::getAddressString) + .collect(Collectors.toList()); // getAddressBalance is memoized, because we'll map it over addresses twice. // To get the balances, we'll be using .getUnchecked, because we know that // this::getAddressBalance cannot return null. var balances = memoize(this::getAddressBalance); - boolean noAddressHasZeroBalance = - addressStrings.stream() - .allMatch(addressString -> balances.getUnchecked(addressString) != 0); + boolean noAddressHasZeroBalance = addressStrings.stream() + .allMatch(addressString -> balances.getUnchecked(addressString) != 0); if (noAddressHasZeroBalance) { var newZeroBalanceAddress = btcWalletService.getFreshAddressEntry(); @@ -129,13 +132,13 @@ public List getFundingAddresses() { .collect(Collectors.toList()); } - public int getNumConfirmationsForMostRecentTransaction(String addressString) { + int getNumConfirmationsForMostRecentTransaction(String addressString) { Address address = getAddressEntry(addressString).getAddress(); TransactionConfidence confidence = btcWalletService.getConfidenceForAddress(address); return confidence == null ? 0 : confidence.getDepthInBlocks(); } - public void setWalletPassword(String password, String newPassword) { + void setWalletPassword(String password, String newPassword) { verifyWalletsAreAvailable(); KeyCrypterScrypt keyCrypterScrypt = getKeyCrypterScrypt(); @@ -165,7 +168,9 @@ public void setWalletPassword(String password, String newPassword) { walletsManager.backupWallets(); } - public void lockWallet() { + //TODO we should stick with the existing domain language we use in the app (e.g. encrypt wallet) + // Otherwise reviewers need to learn the new language and map it to the existing. + void lockWallet() { if (!walletsManager.areWalletsEncrypted()) throw new IllegalStateException("wallet is not encrypted with a password"); @@ -175,7 +180,9 @@ public void lockWallet() { tempAesKey = null; } - public void unlockWallet(String password, long timeout) { + // I think we duplicate too much domain logic here. Lets move the code where it is used in the UI to the domain + // class where it should be (walletsManager) and re-use that. + void unlockWallet(String password, long timeout) { verifyWalletIsAvailableAndEncrypted(); KeyCrypterScrypt keyCrypterScrypt = getKeyCrypterScrypt(); @@ -196,6 +203,11 @@ public void unlockWallet(String password, long timeout) { lockTask = null; } + // TODO This adds new not existing and problematic functionality. If a user has open offers he need the key in case + // a taker takes the offer. If the timeout has removed the key take offer fails. + // As we are in the core app domain now we should use existing framework for timers (UserThread.runAfter) + // We need to take care that we do not introduce threading issues. The UserThread.setExecutor() was set in the + // main app. lockTask = new TimerTask() { @Override public void run() { @@ -213,7 +225,7 @@ public void run() { // Provided for automated wallet protection method testing, despite the // security risks exposed by providing users the ability to decrypt their wallets. - public void removeWalletPassword(String password) { + void removeWalletPassword(String password) { verifyWalletIsAvailableAndEncrypted(); KeyCrypterScrypt keyCrypterScrypt = getKeyCrypterScrypt(); diff --git a/core/src/main/java/bisq/core/btc/wallet/WalletService.java b/core/src/main/java/bisq/core/btc/wallet/WalletService.java index d56b34a7c95..703f12b328d 100644 --- a/core/src/main/java/bisq/core/btc/wallet/WalletService.java +++ b/core/src/main/java/bisq/core/btc/wallet/WalletService.java @@ -734,6 +734,10 @@ public static Transaction maybeAddTxToWallet(Transaction transaction, return maybeAddTxToWallet(transaction.bitcoinSerialize(), wallet, source); } + public Address getAddress(String addressString) { + return Address.fromBase58(params, addressString); + } + /////////////////////////////////////////////////////////////////////////////////////////// // bisqWalletEventListener diff --git a/core/src/main/java/bisq/core/user/User.java b/core/src/main/java/bisq/core/user/User.java index d9adac9a09c..c9dd6dc17fc 100644 --- a/core/src/main/java/bisq/core/user/User.java +++ b/core/src/main/java/bisq/core/user/User.java @@ -189,6 +189,16 @@ public boolean hasPaymentAccountForCurrency(TradeCurrency tradeCurrency) { // Collection operations /////////////////////////////////////////////////////////////////////////////////////////// + public void addPaymentAccountIfNotExists(PaymentAccount paymentAccount) { + if (paymentAccountNotInList(paymentAccount)) { + addPaymentAccount(paymentAccount); + } + } + + private boolean paymentAccountNotInList(PaymentAccount paymentAccount) { + return getPaymentAccountsAsObservable().stream().noneMatch(e -> e.equals(paymentAccount)); + } + public void addPaymentAccount(PaymentAccount paymentAccount) { paymentAccount.onAddToUser(); diff --git a/daemon/src/main/java/bisq/daemon/app/BisqDaemonMain.java b/daemon/src/main/java/bisq/daemon/app/BisqDaemonMain.java index f7a5dac3d6b..a2a855c9d90 100644 --- a/daemon/src/main/java/bisq/daemon/app/BisqDaemonMain.java +++ b/daemon/src/main/java/bisq/daemon/app/BisqDaemonMain.java @@ -23,6 +23,7 @@ import bisq.common.UserThread; import bisq.common.app.AppModule; +import bisq.common.handlers.ResultHandler; import bisq.common.setup.CommonSetup; import com.google.common.util.concurrent.ThreadFactoryBuilder; @@ -39,6 +40,8 @@ @Slf4j public class BisqDaemonMain extends BisqHeadlessAppMain implements BisqSetup.BisqSetupListener { + private GrpcServer grpcServer; + public static void main(String[] args) { new BisqDaemonMain().execute(args); } @@ -76,6 +79,7 @@ protected void onApplicationLaunched() { @Override protected AppModule getModule() { + // TODO dont we want to use our custom module? return new CoreModule(config); } @@ -99,7 +103,15 @@ protected void startApplication() { protected void onApplicationStarted() { super.onApplicationStarted(); - GrpcServer grpcServer = injector.getInstance(GrpcServer.class); + grpcServer = injector.getInstance(GrpcServer.class); grpcServer.start(); } + + @Override + public void gracefulShutDown(ResultHandler resultHandler) { + super.gracefulShutDown(resultHandler); + + grpcServer.shutdown(); + + } } diff --git a/daemon/src/main/java/bisq/daemon/grpc/GetTradeStatisticsService.java b/daemon/src/main/java/bisq/daemon/grpc/GetTradeStatisticsService.java new file mode 100644 index 00000000000..8ef3a9f3d75 --- /dev/null +++ b/daemon/src/main/java/bisq/daemon/grpc/GetTradeStatisticsService.java @@ -0,0 +1,55 @@ +/* + * 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; +import bisq.core.trade.statistics.TradeStatistics2; + +import bisq.proto.grpc.GetTradeStatisticsGrpc; +import bisq.proto.grpc.GetTradeStatisticsReply; +import bisq.proto.grpc.GetTradeStatisticsRequest; + +import io.grpc.stub.StreamObserver; + +import javax.inject.Inject; +import javax.inject.Singleton; + +import java.util.stream.Collectors; + +@Singleton +class GetTradeStatisticsService extends GetTradeStatisticsGrpc.GetTradeStatisticsImplBase { + private final CoreApi coreApi; + + @Inject + GetTradeStatisticsService(CoreApi coreApi) { + this.coreApi = coreApi; + } + + @Override + public void getTradeStatistics(GetTradeStatisticsRequest req, + StreamObserver responseObserver) { + + var tradeStatistics = coreApi.getTradeStatistics().stream() + .map(TradeStatistics2::toProtoTradeStatistics2) + .collect(Collectors.toList()); + + var reply = GetTradeStatisticsReply.newBuilder().addAllTradeStatistics(tradeStatistics).build(); + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } +} diff --git a/daemon/src/main/java/bisq/daemon/grpc/GetVersionService.java b/daemon/src/main/java/bisq/daemon/grpc/GetVersionService.java new file mode 100644 index 00000000000..45f758cba3a --- /dev/null +++ b/daemon/src/main/java/bisq/daemon/grpc/GetVersionService.java @@ -0,0 +1,46 @@ +/* + * 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; + +import bisq.proto.grpc.GetVersionGrpc; +import bisq.proto.grpc.GetVersionReply; +import bisq.proto.grpc.GetVersionRequest; + +import io.grpc.stub.StreamObserver; + +import javax.inject.Inject; +import javax.inject.Singleton; + +@Singleton +class GetVersionService extends GetVersionGrpc.GetVersionImplBase { + private final CoreApi coreApi; + + @Inject + GetVersionService(CoreApi coreApi) { + this.coreApi = coreApi; + } + + @Override + public void getVersion(GetVersionRequest req, StreamObserver responseObserver) { + var reply = GetVersionReply.newBuilder().setVersion(coreApi.getVersion()).build(); + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } +} diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcOffersService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcOffersService.java index 8a3fa548d92..cac62932537 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcOffersService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcOffersService.java @@ -30,12 +30,14 @@ import io.grpc.stub.StreamObserver; import javax.inject.Inject; +import javax.inject.Singleton; import java.util.List; import java.util.stream.Collectors; import lombok.extern.slf4j.Slf4j; +@Singleton @Slf4j class GrpcOffersService extends OffersGrpc.OffersImplBase { @@ -52,7 +54,7 @@ public void getOffers(GetOffersRequest req, // The client cannot see bisq.core.Offer or its fromProto method. // We use the lighter weight OfferInfo proto wrapper instead, containing just // enough fields to view and create offers. - List result = coreApi.getOffers(req.getDirection(), req.getFiatCurrencyCode()) + List result = coreApi.getOffers(req.getDirection(), req.getCurrencyCode()) .stream().map(offer -> new OfferInfo.OfferInfoBuilder() .withId(offer.getId()) .withDirection(offer.getDirection().name()) diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcPaymentAccountsService.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcPaymentAccountsService.java index e630b30fc50..d01e272e42d 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcPaymentAccountsService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcPaymentAccountsService.java @@ -45,7 +45,7 @@ public GrpcPaymentAccountsService(CoreApi coreApi) { @Override public void createPaymentAccount(CreatePaymentAccountRequest req, StreamObserver responseObserver) { - coreApi.createPaymentAccount(req.getAccountName(), req.getAccountNumber(), req.getFiatCurrencyCode()); + coreApi.createPaymentAccount(req.getPaymentMethodId(), req.getAccountName(), req.getAccountNumber(), req.getCurrencyCode()); var reply = CreatePaymentAccountReply.newBuilder().build(); responseObserver.onNext(reply); responseObserver.onCompleted(); @@ -54,10 +54,10 @@ public void createPaymentAccount(CreatePaymentAccountRequest req, @Override public void getPaymentAccounts(GetPaymentAccountsRequest req, StreamObserver responseObserver) { - var tradeStatistics = coreApi.getPaymentAccounts().stream() + var paymentAccounts = coreApi.getPaymentAccounts().stream() .map(PaymentAccount::toProtoMessage) .collect(Collectors.toList()); - var reply = GetPaymentAccountsReply.newBuilder().addAllPaymentAccounts(tradeStatistics).build(); + var reply = GetPaymentAccountsReply.newBuilder().addAllPaymentAccounts(paymentAccounts).build(); responseObserver.onNext(reply); responseObserver.onCompleted(); } diff --git a/daemon/src/main/java/bisq/daemon/grpc/GrpcServer.java b/daemon/src/main/java/bisq/daemon/grpc/GrpcServer.java index a1293dfa0ac..fee8baa38ab 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcServer.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcServer.java @@ -17,51 +17,40 @@ package bisq.daemon.grpc; -import bisq.core.api.CoreApi; -import bisq.core.trade.statistics.TradeStatistics2; - import bisq.common.config.Config; -import bisq.proto.grpc.GetTradeStatisticsGrpc; -import bisq.proto.grpc.GetTradeStatisticsReply; -import bisq.proto.grpc.GetTradeStatisticsRequest; -import bisq.proto.grpc.GetVersionGrpc; -import bisq.proto.grpc.GetVersionReply; -import bisq.proto.grpc.GetVersionRequest; - import io.grpc.Server; import io.grpc.ServerBuilder; -import io.grpc.stub.StreamObserver; import javax.inject.Inject; +import javax.inject.Singleton; import java.io.IOException; import java.io.UncheckedIOException; -import java.util.stream.Collectors; - import lombok.extern.slf4j.Slf4j; +@Singleton @Slf4j public class GrpcServer { - private final CoreApi coreApi; private final Server server; @Inject public GrpcServer(Config config, - CoreApi coreApi, + PasswordAuthInterceptor passwordAuthInterceptor, GrpcOffersService offersService, + GetVersionService getVersionService, + GetTradeStatisticsService getTradeStatisticsService, GrpcPaymentAccountsService paymentAccountsService, GrpcWalletsService walletsService) { - this.coreApi = coreApi; this.server = ServerBuilder.forPort(config.apiPort) - .addService(new GetVersionService()) - .addService(new GetTradeStatisticsService()) + .addService(getVersionService) + .addService(getTradeStatisticsService) .addService(offersService) .addService(paymentAccountsService) .addService(walletsService) - .intercept(new PasswordAuthInterceptor(config.apiPassword)) + .intercept(passwordAuthInterceptor) .build(); } @@ -73,32 +62,16 @@ public void start() { server.shutdown(); log.info("shutdown complete"); })); + } catch (IOException ex) { throw new UncheckedIOException(ex); } } - class GetVersionService extends GetVersionGrpc.GetVersionImplBase { - @Override - public void getVersion(GetVersionRequest req, StreamObserver responseObserver) { - var reply = GetVersionReply.newBuilder().setVersion(coreApi.getVersion()).build(); - responseObserver.onNext(reply); - responseObserver.onCompleted(); - } + public void shutdown() { + log.info("Server shutdown started"); + server.shutdown(); + log.info("Server shutdown complete"); } - class GetTradeStatisticsService extends GetTradeStatisticsGrpc.GetTradeStatisticsImplBase { - @Override - public void getTradeStatistics(GetTradeStatisticsRequest req, - StreamObserver responseObserver) { - - var tradeStatistics = coreApi.getTradeStatistics().stream() - .map(TradeStatistics2::toProtoTradeStatistics2) - .collect(Collectors.toList()); - - var reply = GetTradeStatisticsReply.newBuilder().addAllTradeStatistics(tradeStatistics).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 04dd3460234..1e9a018eab0 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java +++ b/daemon/src/main/java/bisq/daemon/grpc/GrpcWalletsService.java @@ -41,10 +41,12 @@ import io.grpc.stub.StreamObserver; import javax.inject.Inject; +import javax.inject.Singleton; import java.util.List; import java.util.stream.Collectors; +@Singleton class GrpcWalletsService extends WalletsGrpc.WalletsImplBase { private final CoreApi coreApi; @@ -54,11 +56,15 @@ public GrpcWalletsService(CoreApi coreApi) { this.coreApi = coreApi; } + // TODO we need to support 3 or 4 balance types: available, reserved, lockedInTrade and maybe total wallet balance (available+reserved) + // To not duplicate the methods we should pass an enum type. Eenums in proto are a bit cumbersome as they are global + // so you get quickly namespace conflicts if not always prefixes which makes it more verbose. In the core code base we + // move to the strategy to store the enum name and map it. This gives also more flexibility with updates. @Override public void getBalance(GetBalanceRequest req, StreamObserver responseObserver) { try { - long result = coreApi.getAvailableBalance(); - var reply = GetBalanceReply.newBuilder().setBalance(result).build(); + long availableBalance = coreApi.getAvailableBalance(); + var reply = GetBalanceReply.newBuilder().setBalance(availableBalance).build(); responseObserver.onNext(reply); responseObserver.onCompleted(); } catch (IllegalStateException cause) { diff --git a/daemon/src/main/java/bisq/daemon/grpc/PasswordAuthInterceptor.java b/daemon/src/main/java/bisq/daemon/grpc/PasswordAuthInterceptor.java index 7798857dd2a..cafd0d2cb7f 100644 --- a/daemon/src/main/java/bisq/daemon/grpc/PasswordAuthInterceptor.java +++ b/daemon/src/main/java/bisq/daemon/grpc/PasswordAuthInterceptor.java @@ -17,12 +17,17 @@ package bisq.daemon.grpc; +import bisq.common.config.Config; + import io.grpc.Metadata; import io.grpc.ServerCall; import io.grpc.ServerCallHandler; import io.grpc.ServerInterceptor; import io.grpc.StatusRuntimeException; +import javax.inject.Inject; +import javax.inject.Singleton; + import static io.grpc.Metadata.ASCII_STRING_MARSHALLER; import static io.grpc.Metadata.Key; import static io.grpc.Status.UNAUTHENTICATED; @@ -34,14 +39,16 @@ * * @see bisq.common.config.Config#apiPassword */ +@Singleton class PasswordAuthInterceptor implements ServerInterceptor { - public static final String PASSWORD_KEY = "password"; + private static final String PASSWORD_KEY = "password"; private final String expectedPasswordValue; - public PasswordAuthInterceptor(String expectedPasswordValue) { - this.expectedPasswordValue = expectedPasswordValue; + @Inject + public PasswordAuthInterceptor(Config config) { + this.expectedPasswordValue = config.apiPassword; } @Override diff --git a/proto/src/main/proto/grpc.proto b/proto/src/main/proto/grpc.proto index 730920b0622..c1fab86e631 100644 --- a/proto/src/main/proto/grpc.proto +++ b/proto/src/main/proto/grpc.proto @@ -52,7 +52,7 @@ service Offers { message GetOffersRequest { string direction = 1; - string fiatCurrencyCode = 2; + string currencyCode = 2; } message GetOffersReply { @@ -105,10 +105,13 @@ service PaymentAccounts { } } + message CreatePaymentAccountRequest { - string accountName = 1; - string accountNumber = 2; - string fiatCurrencyCode = 3; + string paymentMethodId = 1; + string accountName = 2; + string accountNumber = 3; + // TODO We need to support multiple currencyCodes. Maybe add a repeated and if only one is used its a singletonList + string currencyCode = 4; } message CreatePaymentAccountReply { @@ -176,6 +179,7 @@ message GetAddressBalanceReply { message GetFundingAddressesRequest { } +// Isn't a string address entry enough here? why we need the balance and numConfirmations? message GetFundingAddressesReply { repeated AddressBalanceInfo addressBalanceInfo = 1; } @@ -209,6 +213,32 @@ message UnlockWalletRequest { message UnlockWalletReply { } +/* +We should not repeat the design mistake (from bisq UI code) to use addresses as important domain objects. +What if you have 2 txs to the same address? Which numConfirmations do you display? +A better approach is to use tx and txOutputs as basic elements. For the user who is interested in the balance on a +certain address we should model it in a different way. +For instance (just a quick sketch not 100% sure if that is the best way to model it): + +message AddressBalanceInfo { + string address = 1; + int64 balance = 2; + repeated Utxo utxoList = 3; +} + +message Utxo { + tx tx = 1; + int32 index = 2; + int64 value = 3; +} + +message tx { + string txId = 1; + int32 numConfirmations = 2; + ... more +} +*/ + message AddressBalanceInfo { string address = 1; int64 balance = 2;