Add UI OfferBook list add/remove checks #5659
Conversation
- Define set of editable offer payload fields in MutableOfferPayloadFields. - Move bulk of EditOfferDataModel#onPublishOffer logic to OfferUtil.
- Add editOffer to GrpcOffersService, CoreApi, CoreOffersService. - Set editOffer call rate meter to 1 / minute. - Use new EditOfferValidator to verify editOffer params OK. - Adust getMyOffer(s) rpc impl and OfferInfo model to use OpenOffer for accessing activation state and trigger price.
Reduces size of GrpcClient while allowing for additional bot-friendly variations of the new grpc editOffer method.
Support for editing BSQ offers is in place, but will be added in another PR.
Optionally displaying an ENABLED column in CLI side getoffer output depends on the value of offer.isMyOffer, which is passed via new boolean arguments to the trade & offer pojo builders.
BSQ offers are fixed-price only. This change blocks an attempt to change an altcoin offer to a margin price based offer, or set a trigger price.
And log CLI's getoffer output to see getoffer formatting -- after adding new ENABLED and TRIGGER-PRICE columns.
Use LinkedHashSet to maintain NetworkEnvelope ordering when Connection#onBundleOfEnvelopes calls listeners. Connection#onBundleOfEnvelopes builds a set from an ordered list of NetworkEnvelopes, then calls listeners during set iteration. The envelope list ordering is lost if a HashSet is built, but maintained by switching to a LinkedHashSet. Losing the envelope ordering becomes a problem if the peer receives a RemoveDataMessage and an AddDataMessage in the same batch of envelopes, and relays them to listeners in the wrong (random) order. For example, an API 'editoffer' call may result in the edited offer being added, then immediately remove from their UI's offer book.
This reverts commit add6536.
Using the API's CLI to edit offers can sometimes result in add/remove messages being received on peers in the same batch of envolopes, and these messages are sometimes passed to the UI in (1) add, (2) remove order. This can result in a newly edited offer being removed immediately after being added to the OfferBook list. This change uses storage entry sequence number and storage entry payload hash comparisons to avoid the problem. - OfferBookListItem Added new constructor taking P2PDataStorage.ByteArray hashOfPayload, and int sequenceNumber params. Added a new toString() method. - OfferBook Added new checks on OfferBookListItem hashOfPayload and sequenceNumber while determining if offer candidates should be added or removed from the UI's OfferBook List. See OfferBook contructor's implementation of OfferBookChangedListener#onAdded and OfferBookChangedListener#onRemoved. Added many comments explaining the add/remove rules, and plenty of debug statements to help trace the add/remove event process. - OfferBookService#OfferBookChangedListener Added new P2PDataStorage.ByteArray hashOfPayload, and int sequenceNumber params to listener's onAdded and onRemoved method signatures. Added these two new paramater values to listener.onAdded and listener.onRemoved calls. - TakeOfferDataModel Replaced unused, old tradeManager param in offerBook.removeOffer() with (null) P2PDataStorage.ByteArray hashOfPayload, and (-1) int sequenceNumber params. OfferBook will remove the candidate offer as before. - MarketAlerts Adjusted onAdded() & onRemoved listener method signatures, even though new P2PDataStorage.ByteArray hashOfPayload, int sequenceNumber params are not used by the implementations.
(Accidentally included in last commit.)
ghubstan
changed the title
This change is a refactoring for handling the removal of a peer UI's offer item when it is deactivated and edited in the same CLI `editoffer` command. On the API side, an `editoffer --price=N --enable=false` command results in the edited offer not being re-published. On a UI peer's side, the edited offer is not added to the peer's storage, and the peer's onRemoved(offer) listener event does not find a storage entry with matching payload-hash. This fix assumes an offer that is not in the local store should be removed from the UI's view list -- when the onRemoved method's hashOfPayload does not match the UI's view list item's hashOfPayload.
ghubstan
changed the title
| "old offerBookListItem={}, new offerBookListItem={}", candidateWithSameId.get(), offerBookListItem); | ||
| offerBookListItems.remove(candidateWithSameId.get()); | ||
| OfferBookListItem newOfferBookListItem = new OfferBookListItem(offer, hashOfPayload, sequenceNumber); | ||
| removeAnyOldOfferBookListItemsBeforeAddingReplacement(newOfferBookListItem); |
There was a problem hiding this comment.
I find this method name somewhat on the long side
There was a problem hiding this comment.
Requested change in commit 59c0313.
| @@ -79,7 +110,7 @@ public WitnessAgeData getWitnessAgeData(AccountAgeWitnessService accountAgeWitne | |||
| // either signed & limits lifted, or waiting for limits to be lifted | |||
| // Or banned | |||
| daysSinceSignedAsLong = TimeUnit.MILLISECONDS.toDays(optionalWitness.map(witness -> | |||
| accountAgeWitnessService.getWitnessSignAge(witness, new Date())) | |||
| accountAgeWitnessService.getWitnessSignAge(witness, new Date())) | |||
There was a problem hiding this comment.
Either, this is formatting of an older commit (which I have not found yet). Or my IDE style rules have changed since I upgraded to latest version (doubtful because my bisq style rule file has not changed).
Lines 112-114 in the repo and after auto-formatting in my IDE are
daysSinceSignedAsLong = TimeUnit.MILLISECONDS.toDays(optionalWitness.map(witness ->
accountAgeWitnessService.getWitnessSignAge(witness, new Date()))
.orElse(0L));
| .filter(o -> !isOfferIdBanned(o)) | ||
| .filter(o -> isV3NodeAddressCompliant(o)) |
There was a problem hiding this comment.
If you use ifOfferAllowed() both of these filters can use method references instead. I find that's usually preferred when possible.
There was a problem hiding this comment.
Requested change in commit d709338 .
| if (!isStoredLocally(offer)) { | ||
| Optional<OfferBookListItem> viewItem = getOfferBookListItem(offer); | ||
| viewItem.ifPresent((item) -> { | ||
| offerBookListItems.remove(item); |
There was a problem hiding this comment.
Why is this needed? Isn't this just to present it nicely to the UI and that's not a problem when running the cli?
There was a problem hiding this comment.
Why is this needed?
Short explanation is a stale offer with no matching offer (with id) in local protected storage cannot be taken and should be removed from view.
Isn't this just to present it nicely to the UI and that's not a problem when running the cli?
The OfferBookListItem is a JFX observable used for display in the OfferBook views. This code block supports the CLI being able to atomically edit and deactivate an offer in the same editoffer command, something the UI never does. When you edit an offer with the UI, you deactivate the offer in the 1st mouse click (pencil button), then re-activate it with the confirm-edit or cancel button clicks -- two separate actions that ensure the temporarily deactivated offer is removed from peers, and the edited (or not) offer is re-published to peers (re-activated).
When the CLI edits and deactivates an offer in the same editoffer command, the edited&disabled offer will not be re-published by the API. On another UI/Peer, the old stale OfferBookListItem would remain (without this code block) in the UI's observable item list because the peer receives on onRemoved msg containing the payload-hash of the newly edited offer the peer never received; there is no matching payload-hash, and OfferBook.onRemoved(offer, payloadHash) does not know which item to remove.
|
File conflicts resolved in child PR #5666 |
Hash of protectedStorageEntry (should be offerPayload) was sometimes resulting in incorrect hash being sent to OfferBook listener methods onAdded(offer, hashOfPayload, sequenceNumber), and onRemoved(offer, hashOfPayload, sequenceNumber). Hash of OfferPayload is correctly passed to listener with this change. Sending the correct hash allows removal of a dubious code block that removed a book view list item when hash compare failed, and no matching offer existed in the OfferBookService. See bisq-network#5659 (comment)
Problem: Using the API's CLI to edit offers causes add/remove messages to be received on peers in the same envelope batch, then passed to the UI in random order. When
OfferBookreceives a remove(offer) event after an add(offer) event, the edited offer will be removed immediately after being added to the UI'sOfferBooklist.Fix: This change uses storage entry sequence number and storage entry payload hash comparisons to avoid the problem.
OfferBookListItemAdded new constructor takingP2PDataStorage.ByteArray hashOfPayload,and
int sequenceNumberparams. Defined atoStringmethod.OfferBookAdded new checks onOfferBookListItem'shashOfPayloadandsequenceNumberwhiledetermining if offer candidates should be added or removed from the UI's OfferBook List.
See OfferBook contructor's implementation of OfferBookChangedListener#onAdded and
OfferBookChangedListener#onRemoved. Added many (maybe too many) comments explaining the add/remove rules,
and plenty of debug statements (maybe too many) to help trace the add/remove event process.
OfferBookService#OfferBookChangedListenerAdded newP2PDataStorage.ByteArrayhashOfPayload,and
int sequenceNumberparams to listener'sonAddedandonRemovedmethod signatures.Added these two new parameter values to listener.onAdded and listener.onRemoved calls.
TakeOfferDataModelReplaced unusedtradeManagerparam inofferBook.removeOfferwith
P2PDataStorage.ByteArray hashOfPayload(null), andint sequenceNumber(-1) params.OfferBookwill remove the candidate offer as before.MarketAlerts
AdjustedonAdded&onRemoved` listener method signatures.This PR is based on #5577, which should be reviewed and merged before this one.