[bisq-network/bisq] Add 'takeoffer' API method (#4673)

[sqrrm]

@sqrrm requested changes on this pull request.

Some smaller comments, but over all I think it looks good again.

> +    @BeforeEach
+    public void init() {
+        alicesAccount = getDefaultPerfectDummyPaymentAccount(alicedaemon);
+        bobsAccount = getDefaultPerfectDummyPaymentAccount(bobdaemon);
+    }
+
+    @Test
+    @Order(1)
+    public void testTakeAlicesBuyOffer() {
+        try {
+            var alicesOffer = createAliceOffer(alicesAccount, "buy", "usd", 12500000);
+            var offerId = alicesOffer.getId();
+
+            // Wait for Alice's AddToOfferBook task.
+            // Wait times vary;  my logs show >= 2 second delay.
+            sleep(3000);

All these wait times in the test will make the test very slow eventually. I don't really have a solution to it, more of a general observation.

Maybe grouping all the tasks that need waiting on and then run the tests, but that would also group the tests, making them less independent and thus less useful.

> +        // TODO is this a bug?  Why is offer.state == available?
+        assertEquals(AVAILABLE.name(), trade.getOffer().getState());

I suspect it's keeping the last state after the offer was taken, although I haven't looked into it before. There is no `TAKEN` state either. I think it's used to handle the state of the offer during its life cycle, which ends once the offer is taken and everything is then handled by the `Trade`.

> +                trade -> {
+                    resultHandler.accept(trade);
+                },

```suggestion
                resultHandler::accept,
```

> +    Trade getTrade(String tradeId) {
+        return getTradeWithId(tradeId);
+    }
+
+    private Trade getTradeWithId(String tradeId) {
+        return tradeManager.getTradeById(tradeId).orElseThrow(() ->
+                new IllegalArgumentException(format("trade with id '%s' not found", tradeId)));
+    }

I don't understand why this has to be split in two methods, plans for future PRs?

> +        // Set the default values (in rare cases if the fee request was not done yet we get the hard coded default values)
+        // But the "take offer" happens usually after that so we should have already the value from the estimation service.
+        txFeePerByteFromFeeService = feeService.getTxFeePerByte();
+        txFeeFromFeeService = offerUtil.getTxFeeBySize(txFeePerByteFromFeeService, feeTxSize);

Is this really necessary for the api? We probably want to wait for the fee to be calculated properly as an atomic part of initModel.

> +            txFeePerByteFromFeeService = feeService.getTxFeePerByte();
+            txFeeFromFeeService = offerUtil.getTxFeeBySize(txFeePerByteFromFeeService, feeTxSize);
+            calculateTotalToPay();

Fees are calculated here, after getting the reply from feeService.

> +        this.offer = offer;
+        this.paymentAccount = paymentAccount;
+        this.addressEntry = btcWalletService.getOrCreateAddressEntry(offer.getId(), OFFER_FUNDING);
+        validateModelInputs();
+
+        this.useSavingsWallet = useSavingsWallet;
+        this.amount = valueOf(Math.min(offer.getAmount().value, getMaxTradeLimit()));
+        this.securityDeposit = offer.getDirection() == SELL
+                ? offer.getBuyerSecurityDeposit()
+                : offer.getSellerSecurityDeposit();
+        this.isCurrencyForTakerFeeBtc = offerUtil.isCurrencyForTakerFeeBtc(amount);
+        this.takerFee = offerUtil.getTakerFee(isCurrencyForTakerFeeBtc, amount);
+
+        calculateTxFees();
+        calculateVolume();
+        calculateTotalToPay();

This is already done in calculateTxFees

> +    private Coin takerFee;
+    @Getter
+    private Coin totalToPayAsCoin;
+    @Getter
+    private Coin missingCoin = ZERO;
+    @Getter
+    private Coin totalAvailableBalance;
+    @Getter
+    private Coin balance;
+    @Getter
+    private boolean isBtcWalletFunded;
+    @Getter
+    private Volume volume;
+
+    @Inject
+    public TakeOfferModel(AccountAgeWitnessService accountAgeWitnessService,

There's a lot of code that would be nice to reuse from `TakeOfferDataModel` here. Can be done later though in some pure refactor commit.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/4673#pullrequestreview-514610085
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20201022/ae02c8bd/attachment.html>