Ack messages for all trade, offer- and dispute messages #12
Conversation
- Add Capability support - Add senderNodeAddress and sourceMsgClassName to AckMessage - Send msg at handleTaskRunnerSuccess and handleTaskRunnerFault - Avoid that DAO methods get called in case of connection loss
If the AckMessage is sent as a mailbox message we need to know if the peer supports the capability. If there is no connection already created or if the connection is new and the capabilities not set by the peer, we would not know his capabilities. One solution would be to send first a request fro his capabilities but that adds time delay and complexity. We use the persisted peer data to store the capability. In the use case of a trade we have been earlier in contact with the peer so we should have his capability received. The peer list gets cleaned up with old entries after a while but we expect that the trade messages are sent earlier before a peer gets removed from the peers list. And even if we would not have the peer there the worst thing is that the AckMessage will not be sent. We log a warning for such cases. We use a weakReference for the listener for notifying the peers if their capabilities property gets changed. Peers might get removed from the list anytime. With the weak reference we don't need to worry about removing the listeners to avoid memory leaks.
- Show errors at send dispute msg and at ackMsg. - Add supportedCapabilities of persisted or reported peers to a peer from a new connection (which has empty supportedCapabilities).
|
utACK |
|
|
||
| @Override | ||
| public long getTTL() { | ||
| return TimeUnit.DAYS.toMillis(10); |
There was a problem hiding this comment.
Not opposing, but what was the reasoning behind 10 days?
There was a problem hiding this comment.
The trade period can take about 1 week (sepa), so 10 days seems to be reasonable safe to not miss acks. Also the dispute can take sometimes longer if a peer need to wait for bank reply or the like. But agree hard to say whats the best value here....
| @@ -322,6 +334,10 @@ public void removeMessageListener(MessageListener messageListener) { | |||
| "That might happen because of async behaviour of CopyOnWriteArraySet"); | |||
| } | |||
|
|
|||
| public void addSupportedCapabilitiesListenerAsWeakReference(SupportedCapabilitiesListener listener) { | |||
There was a problem hiding this comment.
I find these overly long method names rather hard to read. It's preference, but I would prefer a shorter name and a comment on the details about the weak reference.
There was a problem hiding this comment.
Yes, its terrible long ;-). Was just too lazy to think of a good short name....
There was a problem hiding this comment.
addWeakCapabilitiesListener should do it... will rename it...
See: bisq-network/bisq#1585
Depends on: bisq-network/bisq-common#26