[bisq-network/bisq] Add API 'gettrades' method (PR #5976)

[xyzmaker123]

@xyzmaker123 requested changes on this pull request.



> +            stream(CATEGORY.values()).anyMatch(v -> v.name().equalsIgnoreCase(c));
+
+    public GetTradesOptionParser(String[] args) {
+        super(args);
+    }
+
+    public GetTradesOptionParser parse() {
+        super.parse();
+
+        // Short circuit opt validation if user just wants help.
+        if (options.has(helpOpt))
+            return this;
+
+        if (options.has(categoryOpt)) {
+            String category = options.valueOf(categoryOpt);
+            if (category.isEmpty())

Seems like it's dead scenario because [here](https://github.com/bisq-network/bisq/pull/5976/files#diff-67646481a741ff93ae7b93def1bb41b1ae9642082991a8fa4ab3b0c425b30d0cR52) it defaults to `open`.

> @@ -72,6 +78,22 @@ public TradeInfo getTrade(String tradeId) {
         return grpcStubs.tradesService.getTrade(request).getTrade();
     }
 
+    public List<TradeInfo> getOpenTrades() {
+        var request = GetTradesRequest.newBuilder()
+                .build();
+        return grpcStubs.tradesService.getTrades(request).getTradesList();
+    }
+
+    public List<TradeInfo> getTradeHistory(GetTradesRequest.Category category) {
+        if (!category.equals(CLOSED) && !category.equals(FAILED))

1. Why not two methods: `getClosedTrades` and `getFailedTrades` or single method that handles all 3 categories?
2. Do we need category validation at this level?

> @@ -206,12 +214,19 @@ protected void validate() {
                     ? formatToPercent(t.getOffer().getMarketPriceMargin())

What about [`formatPercentagePrice`](https://github.com/bisq-network/bisq/blob/14008a670a5193175dc578d04968121cf2693e54/core/src/main/java/bisq/core/util/FormattingUtils.java#L218)?

> + * 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 <http://www.gnu.org/licenses/>.
+ */
+
 package bisq.cli.table.builder;

Seems like `AbstractTradeTableBuilder` class is not used anywhere

> @@ -273,15 +289,33 @@ Trade getTrade(String tradeId) {
                 ));
     }
 
+    List<Trade> getOpenTrades() {
+        coreWalletsService.verifyWalletsAreAvailable();
+        coreWalletsService.verifyEncryptedWalletIsUnlocked();
+        return tradeManager.getTrades();

Why not mapping to `(TradeModel) t` here, to be consistent with `getTradeHistory` method at this level?

> +                    var isMyOffer = coreApi.isMyOffer(tradeModel.getOffer());
+                    var isBsqSwapTrade = tradeModel instanceof BsqSwapTrade;
+                    var numConfirmations = isBsqSwapTrade
+                            ? coreApi.getTransactionConfirmations(((BsqSwapTrade) tradeModel).getTxId())
+                            : 0;
+                    var closingStatus = category.equals(OPEN)
+                            ? "Pending"
+                            : coreApi.getClosedTradeStateAsString(tradeModel);
+                    return isBsqSwapTrade
+                            ? toTradeInfo((BsqSwapTrade) tradeModel, role, isMyOffer, numConfirmations, closingStatus)
+                            : toTradeInfo(tradeModel, role, isMyOffer, closingStatus);
+                })
+                .collect(Collectors.toList());
+
+        // Add canceled OpenOffers to returned closed trades list.
+        Optional<List<OpenOffer>> canceledOpenOffers = category.equals(CLOSED)

Why canceled open offers are treated as closed trades? 

> @@ -273,15 +289,33 @@ Trade getTrade(String tradeId) {
                 ));
     }
 
+    List<Trade> getOpenTrades() {
+        coreWalletsService.verifyWalletsAreAvailable();
+        coreWalletsService.verifyEncryptedWalletIsUnlocked();
+        return tradeManager.getTrades();
+    }
+
+    List<TradeModel> getTradeHistory(GetTradesRequest.Category category) {
+        coreWalletsService.verifyWalletsAreAvailable();
+        coreWalletsService.verifyEncryptedWalletIsUnlocked();
+        if (category.equals(CLOSED)) {
+            var closedTrades = closedTradableManager.getClosedTrades().stream()
+                    .map(t -> (TradeModel) t)
+                    .collect(Collectors.toList());
+            closedTrades.addAll(bsqSwapTradeManager.getBsqSwapTrades());

Shouldn't we use `getConfirmedBsqSwapTrades` here?

-- 
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/5976#pullrequestreview-862722646
You are receiving this because you are subscribed to this thread.

Message ID: <bisq-network/bisq/pull/5976/review/862722646 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20220125/431d2e13/attachment-0001.htm>