Tribler / anydex-core

The Universal Decentralized Exchange
GNU Lesser General Public License v3.0
9 stars 6 forks source link

WIP: Minor code improvements #67

Closed Solomon1732 closed 3 years ago

Solomon1732 commented 3 years ago

Implement minor code improvements as discussed in #63 Fixes #63

tribler-ci commented 3 years ago

Can one of the admins verify this patch?

devos50 commented 3 years ago

ok to test

devos50 commented 3 years ago

add to whitelist

devos50 commented 3 years ago

retest this please

Solomon1732 commented 3 years ago

Seems like I can't test it locally. It keeps failing on import of orjson. Even when I install the package it fails importing it when I run the tests.

devos50 commented 3 years ago

That's unexpected. Doing a pip install orjson should work. Maybe you could try to install the package both in user space and system-wide?

Solomon1732 commented 3 years ago

I just had to uninstall the module than reinstall it. I have no idea what happened before. I'm getting different errors now, but at least I've solved that problem.

Edit: These are the errors I'm getting: https://gist.github.com/Solomon1732/d079b53b701aceabd2cffb3f47195e5a

Solomon1732 commented 3 years ago

Latest commit was mainly to add a comment in order to prevent further issues like #68 being opened in the future upon code inspection.

Solomon1732 commented 3 years ago

@devos50 in match_queue I'm trying to implement the internal items as a NamedTuple (from python's typing). However when trying to write the methods for less-than, greater-than, equal, and so forth, I don't know how to compare the internal structure of the tuple in such a way that it would result in the desired order. Could you maybe advise me as to what items I should compare? I just don't want to blindly copy the cmp_items method over and over again, with little modifications only as the differences between the rich-comparison methods (lt, ge, eq, etc.)

Currently the named tuple is this:

    @dataclasses.dataclass(eq=True, frozen=True)
    class _QueueItem(NamedTuple):
        retries: int
        price: Price
        is_ask: bool
        order_id: OrderId
        other_quantity: Any
devos50 commented 3 years ago

Good question. First, you should understand some basics of the matchmaking mechanism as implemented in AnyDex. In AnyDex, matchmakers send match notifications to traders that have outstanding buy and sell orders. When a trader receives such a notification, it adds an entry to the match queue of the associated order. The match queue is a priority queue that prioritizes orders with a high quality. Each entry in the match queue has a few fields as you implemented in the _QueueItem data class. The retries field is an integer that indicates how many times we have tried to negotiate about a specific entry (also see Figure 3 in our article).

To answer your question, we prioritize first on retries, and then on the price (a better price will be prioritized). However, what constitutes the 'better' price depends on whether the order is a buy or sell order. This is the logic on line 33-42 in the match queue logic.

Hopefully this provides sufficient information for you to continue 👍

Solomon1732 commented 3 years ago

So, just to see if I'm on the mark, is this example fitting for what you have in mind?

def __eq__(self, other) -> bool:
    return self.retries == other.retries and self.price == other.price

def __lt__(self, other) -> bool:
    if self.retries < other.retries:
        return True
    elif self.retries == other.retries:
        if self.match_order.is_ask():
            return self.price < other.price
        else:
            return not (self.price < other.price)
    else:
        return False
devos50 commented 3 years ago

@Solomon1732 I think it is the correct logic, but you could consider adding a few unit tests to more thoroughly check/compare your solution 👍

devos50 commented 3 years ago

@Solomon1732 any plans to finish this PR?

Solomon1732 commented 3 years ago

Things happened and I completely forgot about this PR! I'm so sorry! I'll close it for now since in the near future I don't think I'll manage to push this through.