[bisq-network/bisq] Adjust API 'editoffer' to PR 5651 (include extraData field when editing offer) (#5666)

[sqrrm]

@sqrrm commented on this pull request.



>              }
         });
     }
 
-    public void removeOffer(Offer offer, TradeManager tradeManager) {
+    private void removeDuplicateItem(OfferBookListItem newOfferBookListItem) {
+        String offerId = newOfferBookListItem.getOffer().getId();
+        // We need to remove any view items with a matching offerId before
+        // a newOfferBookListItem is added to the view.
+        List<OfferBookListItem> duplicateItems = offerBookListItems.stream()
+                .filter(item -> item.getOffer().getId().equals(offerId))
+                .collect(Collectors.toList());
+        if (duplicateItems.size() > 0) {

This check is not needed since `forEach` won't do anything if there are no items in the list.

> +                if (log.isDebugEnabled()) {
+                    log.debug("onAdded: Removed old offer {}\n"
+                                    + "\twith payload hash {} from list.\n"
+                                    + "\tThis may make a subsequent onRemoved( {} ) call redundant.",
+                            offerId,
+                            oldOfferItem.getHashOfPayload() == null ? "null" : oldOfferItem.getHashOfPayload().getHex(),
+                            oldOfferItem.getOffer().getId());
+                }

There are still a lot of debug logs. Do you want to keep them for the first release with these changes? If so, I think it would be good to add a TODO to remove them later as I don't think they're that valuable in the long run.

> +        if (!candidateWithMatchingPayloadHash.isPresent()) {
+            if (log.isDebugEnabled()) {
+                log.debug("UI view list does not contain offer with id {} and payload-hash {}",
+                        offer.getId(),
+                        hashOfPayload == null ? "null" : hashOfPayload.getHex());
+            }
+            return;
+        }
+
+        OfferBookListItem candidate = candidateWithMatchingPayloadHash.get();
+
+        // Remove the candidate only if the candidate's offer payload hash matches the
+        // onRemoved hashOfPayload parameter.  We may receive add/remove messages out of
+        // order (from api's 'editoffer'), and use the offer payload hash to
+        // ensure we do not remove an edited offer immediately after it was added.
+        if ((candidate.getHashOfPayload() == null || candidate.getHashOfPayload().equals(hashOfPayload))) {

Does this mean an item with null hash will be removed if only the id matches? Seems like a way to get around the hash checks.

-- 
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/5666#pullrequestreview-738221680
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20210825/81e61798/attachment.htm>