Add protection tools

[chimp1984]

Implements protection tools in context of API deployment (bisq-network/projects#46)

Based on #5048. Starts at 4bbc394

Replaced #5051

[ghubstan]

@ghubstan: If the offerFilter.canTakeOffer call can be included in getOffers(String direction, String currencyCode) and getOffer(String id), we can remove the getOffersAvailableForTaker method.

@chimp1984 If you want to back out that new core api getOffersAvailableForTaker method, add the two lines (commented in the snippet below as /* HERE */). I'll test if you make the suggested change in the PR.

// TODO should we add a check for offerFilter.canTakeOffer?
Offer getOffer(String id) {
    return offerBookService.getOffers().stream()
            .filter(o -> o.getId().equals(id))
            .filter(o -> offerFilter.canTakeOffer(o, true).isValid())   /* HERE */
            .findAny().orElseThrow(() ->
                    new IllegalStateException(format("offer with id '%s' not found", id)));
}

// TODO returns all offers also those which cannot be taken. Should we use the filter from
//  getOffersAvailableForTaker here and remove the getOffersAvailableForTaker method?
List<Offer> getOffers(String direction, String currencyCode) {
    List<Offer> offers = offerBookService.getOffers().stream()
            .filter(o -> {
                var offerOfWantedDirection = o.getDirection().name().equalsIgnoreCase(direction);
                var offerInWantedCurrency = o.getOfferPayload().getCounterCurrencyCode()
                        .equalsIgnoreCase(currencyCode);
                return offerOfWantedDirection && offerInWantedCurrency;
            })
            .filter(o -> offerFilter.canTakeOffer(o, true).isValid())	/* HERE */
            .collect(Collectors.toList());

    // A buyer probably wants to see sell orders in price ascending order.
    // A seller probably wants to see buy orders in price descending order.
    if (direction.equalsIgnoreCase(BUY.name()))
        offers.sort(Comparator.comparing(Offer::getPrice).reversed());
    else
        offers.sort(Comparator.comparing(Offer::getPrice));

    return offers;
}

[ghubstan]

@chimp1984 (RE another comment from the old PR replaced by this one)...
@ghubstan: I set the value in CoreApi and not directly in the CoreTradesService, so keep it more open that those sub domain classes can be used from desktop as well. Not sure if is not intended/permitted for some reason but I though that way we have more flexibility. Feel free to remove the param and set it directly in CoreTradesService if that class will never be used from non-api code.

Good. I hope CoreWhateverService can be used by non api code some day.

[chimp1984]

@ripcurlx Any idea why travis is failing so frequently?

* What went wrong:

Execution failed for task ':common:compileJava'.

> Could not resolve all files for configuration ':common:compileClasspath'.

   > Could not download javafx-base.jar (org.openjfx:javafx-base:11.0.2)

      > Could not get resource 'https://repo.maven.apache.org/maven2/org/openjfx/javafx-base/11.0.2/javafx-base-11.0.2.jar'.

         > Could not GET 'https://repo.maven.apache.org/maven2/org/openjfx/javafx-base/11.0.2/javafx-base-11.0.2.jar'.

            > Connection reset

[ghubstan]

@chimp1984 The filter added to CoreOffersService getOffer and getOffers [might] prevent api users from seeing their own offers.
Adding a new getmyoffers doesn't see right. Any suggestions?

Also, this filter is flushing out bugs in some of my offer/trade aptest cases, first one: creating offers for other fiat currencies than the dummy acct's currency, but that should be easy to fix. There may be other problems to be found while using this new filter...

[chimp1984]

@chimp1984 The filter added to CoreOffersService getOffer and getOffers prevents api users from seeing their own offers.
Adding a new getmyoffers doesn't see right. Any suggestions?

Also, this filter is flushing out bugs in some of my offer/trade aptest cases, first one: creating offers for other fiat currencies than the dummy acct's currency, but that should be easy to fix. There may be other problems to be found while using this new filter...

Ah true. It filters out all offers you cannot take, including your own ones. I assume the getOffer/getOffers methods are primarily used for finding an offer to take. To browse through offers for informational purpose seems not a real use-case, no?

We could add a getAllOffers method (without the filter) if that makes sense. It just should be clear that those are not usable for selecting to take an offer.

[ghubstan]

To browse through offers for informational purpose seems not a real use-case, no?

I would want to be able to see my own offers for correctness, current price (if not fixed), and get the id if I wanted to edit it (todo).

[chimp1984]

To browse through offers for informational purpose seems not a real use-case, no?

I would want to be able to see my own offers for correctness, current price (if not fixed), and get the id if I wanted to edit it (todo).

Ah ok. We could include a flag showMyOffers or a new methods for that. Whateven you find better suited for your needs... Or any other idea?

[ghubstan]

@chimp1984 Does this filter work (not filter out) offers associated with the regtest dummy acct?

I'm checking the filter's rejection reasons when I create a USD offer, then get it (IS_INSUFFICIENT_COUNTERPARTY_TRADE_LIMIT), or an EUR offer (HAS_NO_PAYMENT_ACCOUNT_VALID_FOR_OFFER).

No doubt, the filter will flush out apitest case bugs, but this seems strange. Have you tried this on regtest, using the dummy acct to getoffer? I mean in the UI, not the CLI. Sorry for the ambiguity.

[ghubstan]

We could include a flag showMyOffers or a new methods for that. Whateven you find better suited for your needs... Or any other idea?

I will think a bit.
I think a new getmyoffers (the CLI method name) would be good -- the analog to the UI's Portfolio -> "My Open Offers" view.

[chimp1984]

@chimp1984 Does this filter work (not filter out) offers associated with the regtest dummy acct?

I'm checking the filter's rejection reasons when I create a USD offer, then get it (IS_INSUFFICIENT_COUNTERPARTY_TRADE_LIMIT), or an EUR offer (HAS_NO_PAYMENT_ACCOUNT_VALID_FOR_OFFER).

No doubt, the filter will flush out apitest case bugs, but this seems strange. Have you tried this on regtest, using the dummy acct to getoffer? I mean in the UI, not the CLI. Sorry for the ambiguity.

The filter is in the app since long. I just added the denyApiUser flag. With the dummy accounts created in devtest mode it worked for me. Best to debug into it to see where it fails. Maybe account aget witness related?

[ghubstan]

@chimp1984 I'm working on new getmyoffers & getmyoffer api methods, and adjusting some test cases to make sure dummy offers are created with the right kind of dummy payment acct. Then I will debug the new filtering...

I know this is a bottleneck for you. I'll check it in later tonight or tomorrow afternoon.

[ghubstan]

After manually merging my add-getmyoffers-api-method branch's changes (#5056) into a clone of @chimp1984 's PR #5053, I find that offerFilter.canTakeOffer is working as expected, at least in the core api.

I don't know how you want to proceed, @chimp1984, but I would do the following:

1. Back out all changes made in core.api for PR Add protection tools #5053 (@chimp1984)
2. Review/Merge PR Add new api methods 'getmyoffers' and 'getmyoffer' #5056 (@chimp1984, @sqrrm)
3. Review/Merge PR Add protection tools #5053 (@ghubstan, @sqrrm)
4. Create a new PR to apply the filtering in core api's offers service (@ghubstan)

[chimp1984]

Ok. I will remove the changes possible.

[ghubstan]

A qualified Tested ACK

I reviewed the code changes, and manually copied my changes from #5056 into a clone of this PR for some basic api getoffer(s) testing -- but not all of the filtering edge cases.

[ripcurlx]

utACK based on #5053 (review)