Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

server/{market,swap,db}: free cancels #1682

Merged
merged 3 commits into from Aug 22, 2022
Merged

Conversation

buck54321
Copy link
Member

server/db:
Add epoch_gap column to cancel order table. When set (not EpochGapNA),
the epoch_gap is the number of epochs between the limit order placement
and the cancel order placement. Update InsertEpoch method to accept an
epoch gap argument (or EpochGapNA).

server/auth:
RecordCancel now accepts an epoch gap argument. Epoch gap is loaded from
the db via ExecutedCancelsForUser with a modified return value.
(*latestOrders).counts now only counts cancel orders if 0 <= epochGap < 2.

server/market:
processOrder now reports the epoch gap to the AuthManager for matched
cancel orders.

@@ -116,7 +124,7 @@ func (lo *latestOrders) counts() (total, cancels int) {

total = len(lo.orders)
for _, o := range lo.orders {
if o.target != nil {
if o.target != nil && o.epochGap >= 0 && o.epochGap < freeCancelThreshold {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the line that matters. This is a rule change. Free cancels after the order sits for one full epoch. If the order is submitted in epoch 10, and the cancel order submitted in epoch 10 or 11, that's a "short" cancel (not a free cancel), and the cancellation can hurt to the user's score.

@chappjc
Copy link
Member

chappjc commented Jun 24, 2022

Have not looked at the code, but as discussed, this is a very reasonable compromise to outright eliminating the cancellation ratio limit that is overly restrictive. I think a very small freeCancelThreshold on epochGap, which you have proposed here, would not create much more of a challenge for mesh than already exists with the current order history knowledge requirements since it's necessarily very recent history. Good stuff!

However, I still view the cancellation ratio limit as largely unnecessary given the epoch-shuffle matching algorithm, which makes it risky to cancel rapidly for deceptive purposes, combined with the need to have unique coins funding every order and the various order quantity limits, which throttle the ability of a user to place an excessive number of (presumably bogus) orders.

@buck54321
Copy link
Member Author

buck54321 commented Jun 24, 2022

I think this will improve the character of our order books. Human users should have a fair chance to match with orders that they see. Whereas before, an order could be 1) placed and canceled in the same epoch, or 2) placed near the end of an epoch, and canceled in the next epoch. In either of those cases, there's a chance that I could see that order, place my order immediately, and then that order is canceled before my order comes up for matching. It's like probabilistic spoofing. But if you wait for the free cancel, that's eliminated.

If you truly make a mistake or something and want to cancel quickly, you still have that choice. Cancellations are relatively low impact in terms of penalization. Human traders will never run up against the cancellation ratio. But bots are encouraged to trade in a way that makes the book more stable and usable.

@chappjc
Copy link
Member

chappjc commented Jun 24, 2022

Well put. I think this is a great balance.

Although I think users always did have a fair chance because as you said its a probabilistic spoofing game, not without risk to the would-be spoofer.

@buck54321
Copy link
Member Author

Although I think users always did have a fair chance

I guess that's true, in a sense. It think the proposed rule change gently incentivizes behavior that contributes to a better trading experience, which was the intended effect of cancellation limits.

In regards to previous conversations regarding market maker order book management, I think the proposed rules are relatively easy to satisfy without a large disruption to existing protocols. All of that careful order management I was talking about is unneeded.

@buck54321 buck54321 marked this pull request as ready for review July 4, 2022 17:41
@chappjc chappjc added this to the 0.5.1 milestone Jul 12, 2022
Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Some minor nits.

server/auth/auth.go Outdated Show resolved Hide resolved
server/db/interface.go Outdated Show resolved Hide resolved
server/db/interface.go Outdated Show resolved Hide resolved
server/db/driver/pg/orders.go Outdated Show resolved Hide resolved
@@ -2137,7 +2141,7 @@ func (m *Market) prepEpoch(orders []order.Order, epochEnd time.Time) (cSum []byt
// Change the order status from orderStatusEpoch to orderStatusRevoked.
coid, revTime, err := m.storage.RevokeOrder(ord)
if err == nil {
m.auth.RecordCancel(ord.User(), coid, ord.ID(), revTime)
m.auth.RecordCancel(ord.User(), coid, ord.ID(), db.EpochGapNA, revTime)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking this one should perhaps use a gap of 0. Yes, the user will get the preimage miss strike too, but this is effectively cancelling an order in the same epoch. Redundant with the preimage violation?

server/db/driver/pg/upgrades.go Outdated Show resolved Hide resolved
server/db/driver/pg/upgrades.go Outdated Show resolved Hide resolved
server/auth/cancel.go Outdated Show resolved Hide resolved
server/db:
Add epoch_gap column to cancel order table. When set (not EpochGapNA),
the epoch_gap is the number of epochs between the limit order placement
and the cancel order placement. Update InsertEpoch method to accept an
epoch gap argument (or EpochGapNA).

server/auth:
RecordCancel now accepts an epoch gap argument. Epoch gap is loaded from
the db via ExecutedCancelsForUser with a modified return value.
(*latestOrders).counts now only counts cancel orders if 0 <= epochGap < 2.

server/market:
processOrder now reports the epoch gap to the AuthManager for matched
cancel orders.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants