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

BSIP61: Operation to Update Limit Orders #150

Closed
nathanielhourt opened this issue Feb 22, 2019 · 33 comments
Closed

BSIP61: Operation to Update Limit Orders #150

nathanielhourt opened this issue Feb 22, 2019 · 33 comments
Labels

Comments

@nathanielhourt
Copy link
Contributor

nathanielhourt commented Feb 22, 2019

BSIP: 0061
Title: Operation to Update Limit Orders
Authors:
  Nathan Hourt <nat.hourt@gmail.com>
Contributors and Reviewers:
  TBD
Status: Draft
Type: Protocol
Created: 2019-02-22
Discussion: https://github.com/bitshares/bsips/issues/150
Worker: Unpaid

Abstract

This BSIP proposes a new operation, limit_order_update_operation, which will allow direct modification of a limit order. This operation will be smaller to serialize than the cancel/create operation combination presently required to achieve the same result, as it must only store new field values rather than all field values. Moreover, the update operation will require lower processing and RAM consumption. In return for this reduction in consumption, the overall fee charged for an update operation can be set lower than the equivalent cancel/create combo, thus incentivizing market makers to keep freshly updated orders at the top of the books.

Motivation

At present, the BitShares protocol does not allow direct modification of limit orders in the markets. Instead, to modify a limit order, the order must be canceled and and a new one created in its place. This results in greater-than-necessary overhead when updating a limit order, an action which is performed by market maker robots with great frequency (hundreds of times per day, per market). Additionally, the cancel/create combination requires payment of two fees, which has led to some pressure from market makers for BitShares to provide a cheaper mechanism for maintaining fresh order walls in the markets.

Rationale

The specification below was chosen on the following criteria:

  • Minimal storage space for historical data (i.e. serialized operation size)
    • Cancel/create combination requires two distinct operations, and explicit restatement of all order parameters
    • Update operation requires only one operation, which elides all fields which are not being updated
  • Minimal processing
    • No deletion/construction is necessary, object is modified in place
    • Cancel/create requires two separate modifications of balances; update requires zero or one
    • Order matching is only triggered if order is moved past previous top of book
  • Minimal RAM consumption for undo storage
    • Object modification requires slightly less space than object deletion and creation
    • Balances and account statistics are not updated unless necessary, avoiding unnecessary undo storage
  • Finer granularity of fees
    • Low fee if only the order object is modified
    • Higher fee if balances must be updated as well

Specification

A complete implementation of this BSIP is available for review here.

The limit_order_update_operation contains the following fields:

       asset fee;
       account_id_type seller;
       limit_order_id_type order;
       optional<price> new_price;
       optional<share_type> delta_amount_to_sell;
       optional<time_point_sec> new_expiration;

       extensions_type extensions;

A single new evaluator is required, which shall check that the new field values are valid for the order being updated, and update the order and balances as necessary, then trigger order matching only if the price was modified such that the order has moved past the previous top-of-book order.

No new indexes are required. No modifications to existing indexes are required. No new database objects are defined. No modifications to existing database objects are required.

Discussion and Summary for Shareholders

The changes proposed in this BSIP are easily implemented, and the only downside apparent to the author is the definition of a new operation type, which slightly increases the complexity of creating a new implementation of the BitShares protocol. Thus, the question must be considered, is the new operation of significant value to the community, and will it be used?

The new operation is of greatest value to market makers, as these individuals typically run robots which must monitor the markets and maintain competitively priced orders on both sides of the market. These orders must be updated regularly, especially for BitAsset markets. The addition of an operation to modify limit orders will reduce the cost of market making, thus increasing the incentive for users to provide this service. Additionally, it reduces the requirements for processing and storing market maker activity.

In conclusion, this BSIP proposes a very small modification to the protocol with minimal downside, and a meaningful upside for market makers and reduced requirements for all nodes which process the markets.

Copyright

Intellectual Property is BS.
Those who wish to pretend otherwise are hereby directed to treat this document as public domain.

See Also

bitshares/bitshares-core#1604

@pmconrad
Copy link
Contributor

@ryanRfox please assign a number

@pmconrad
Copy link
Contributor

Excellent, thanks.

Typo at the end of "Motivation" section: fresh order walls

Please expand the "Specification" section by enumerating validity checks and pre-execution conditions.

delta_amount_to_sell could be a share_type instead of asset, to save another 1-2 bytes.

Actually, delta_amount_to_sell complicates things a bit, at least with a negative delta, because we'll have to check if the remainder is to be considered dust. Since the order is cancelled anyway if the remainder is considered dust, this would allow abuse as a cheaper version of limit_order_cancel. Perhaps we should only allow increasing the amount to avoid this problem?

@ryanRfox ryanRfox changed the title New BSIP: Operation to Update Limit Orders BSIP61: Operation to Update Limit Orders Feb 23, 2019
@ryanRfox
Copy link
Contributor

ryanRfox commented Feb 23, 2019

  • Assigned as BSIP61
  • @nathanhourt suggesting to update the BSIP Discussion URL in the header to "BSIP61: Operation to Update Limit Orders #150"
  • Suggesting to include Issue 1604 in the (to-be-added) "See Also" section near the end of the BSIP (reference BSIP Template)
  • Suggesting further drafting of the specification in this Issue, then return to 1604 for implementation
  • I will update the README.md shortly

@pmconrad
Copy link
Contributor

I've been thinking about the dust issue and realized that the dust check will also have to be applied when the price is modified. So to avoid abuse we should disallow the order to be auto-cancelled after update.

Another question: is it better to specify the new amount_to_sell as an absolute value or as a delta? In other words, what is the desired behaviour if the existing order gets partially filled before the update enters the chain?

@nathanielhourt
Copy link
Contributor Author

I can add a dust-check, if needed; how should such a check work?

I did consider using new_amount_to_sell rather than a delta, but I went with the delta because the first thing the backend has to do with a flat amount is calculate the delta anyways. I figured we could just push that step to the frontend. I also suspect that the delta will generally be a smaller number than the flat amount, which might translate to fewer bytes in the serial operation.

And to your last question, I think a delta is more consistent with the principle of least surprise in the case of the order getting partially filled in the last moment before the update hits the chain. If I specify a new amount to go into the order which is only slightly off from the old amount, and then right before that processes, my order gets almost completely filled, the chain will automatically withdraw the entire deficit from my account to fill the order all the way back up to my specified new amount. This could be confusing to the user, who had more money taken from his account than he expected.

I recall also a principle in BitShares operation design that the adjustment to an account's balance is always explicit from the operation directly, without examining chain state. A delta fulfills that, but a new amount does not.

As to using a share_type rather than an asset, great point; I'll update the spec.

@pmconrad
Copy link
Contributor

pmconrad commented Feb 26, 2019

I can add a dust-check, if needed; how should such a check work?

https://github.com/bitshares/bitshares-core/blob/2.0.190219/libraries/chain/db_market.cpp#L301

I agree that the delta is better. I can imagine that a market maker might want to update the order to a new absolute value, independent from whether it was previously filled or not though. He can just do another update then of course.

@nathanielhourt
Copy link
Contributor Author

So... funny thing about using share_type rather than asset... doing so violates the principle I just described, the ability to determine the adjustment to the account balance by looking at just the operation and not requiring chain state. You can't tell what the adjustment to the account balance is by looking at the operation because you don't know what asset is being moved unless you look at chain state. 🤦‍♂️

So in light of the above, do we consider it better to use the couple bytes for the asset and comply with the principle?

@pmconrad
Copy link
Contributor

Since this is not the only place where we have redundancy I think it's better to uphold the principle.

@nathanielhourt
Copy link
Contributor Author

Agreed.

Vis-a-vis the dust check, it seems the actual check is order.amount_to_receive().amount == 0 -- so should the check simply be that if the operation specifies a negative delta amount, assert that after applying the delta, the order's amount_to_receive() should not be zero?

@nathanielhourt
Copy link
Contributor Author

Added dust check: nathanielhourt/bitshares-2@1c81eaf

Dust check: if either the price or the amount is updated, check that the new amount to receive will be nonzero.

@xeroc
Copy link
Member

xeroc commented Feb 27, 2019

For sake for discussion, can we look into implications for order creation refund? Does it make a difference whether to

  • cancel an existing order, or
  • set the expiration time to few seconds to have it expire?

@abitmore
Copy link
Member

Need to describe fee refunding mechanism. See https://github.com/bitshares/bsips/blob/master/bsip-0026.md

@nathanielhourt
Copy link
Contributor Author

As @pmconrad has noted, the intent of this operation is not to allow a cheaper way to cancel orders. For this reason, perhaps we should require that, if new_expiration is set, it must be later than the old one? So you can update your order to expire later, but not sooner?

This would make sense, as the intended user of this operation is market makers, who probably place orders with near expirations (possibly as short as 5 minutes) as these orders are automatically generated and the maker would not want the order to remain on the books after they go stale, in the event that the bot crashes/etc. When the maker updates the order, they may wish to bump the expiration back another 5 minutes, but they would be unlikely to desire to move the expiration closer.

Shall I add a requirement that the new expiration be later than the old one?

@nathanielhourt
Copy link
Contributor Author

nathanielhourt commented Feb 27, 2019

Additionally, do we wish to accumulate fees from order updates into the deferred fees for later potential refund?

For my part, I expect this operation to be set with a very low processing fee which is non-refundable; however, if there is substantial consensus desiring a (presumably higher) refundable fee, I'll be happy to implement it.

@abitmore
Copy link
Member

I don't think updating expiration matters.

I expect this operation to be set with a very low processing fee which is non-refundable

I agree.
Actually, amount of fees of the "cancel/create operation combination" mentioned in OP is single cancellation fee since creation fee would be refunded, thus the description is misleading.

@pmconrad
Copy link
Contributor

Shall I add a requirement that the new expiration be later than the old one?

Sounds good.

I expect this operation to be set with a very low processing fee which is non-refundable

Also agree.

@abitmore
Copy link
Member

abitmore commented Feb 27, 2019

Shall I add a requirement that the new expiration be later than the old one?

Sounds good.

Why not allow it? It's legit (imho) that a user wants to free some RAM earlier, she won't pay less for doing so thus no harm to the chain.

@abitmore
Copy link
Member

What's the rationale of having a high balance-updating fee but a low price-updating fee?

My take:

  • if an order has been partially filled, to update it, the user should pay a fee which is around order creation fee, to avoid changing a dust order to a new order with less cost;
  • if an order has not been partially filled, to update it, the user should pay a fee which is around order cancellation fee.

When signing the transaction, since the user don't know chain state, she should pay order creation fee, the chain can refund immediately when applying the operation, if found the updating order hasn't been filled.

When there is a fee schedule change after an order is created, when updating the order, ideally new fee schedule should apply, that said, we can refund the original order creation fee then charge the updating fee (defer it).

@nathanielhourt
Copy link
Contributor Author

What's the rationale of having a high balance-updating fee but a low price-updating fee?

Updating the price and/or expiration requires only modifying the order object. Updating the balance requires also updating the account balance, and possibly the account statistics objects as well.

if an order has been partially filled, to update it, the user should pay a fee which is around order creation fee, to avoid changing a dust order to a new order with less cost;

But changing a dust order to a new order should cost less than creating a new order... The operation is smaller, and it recycles already-available RAM rather than allocating new space on the heap.

The purpose of this proposal is to make market making more efficient, both in terms of blockchain processing, and in terms of fees paid. Those who will use this operation will probably execute it well over a thousand times per day. It makes good sense to streamline their operations for lighter processing, as well as to give them a lower fee for being bulk buyers. They're our biggest customers -- we ought to treat them well.

@abitmore
Copy link
Member

abitmore commented Feb 28, 2019

On the other hand, they're already bloating the chain due to the low fee. While they're earning money, the chain is being operated at a loss. Consider the chain as a for-profit business but not a charity, fees should be on a certain level.

But changing a dust order to a new order should cost less than creating a new order... The operation is smaller, and it recycles already-available RAM rather than allocating new space on the heap.

The chain charges the first fill of every order an order creation fee, this is the main income from order processing. When a partially filled order (dust) is cancelled then replaced, it means new potential income. If we allow partially filled order to be updated/renewed with a cheaper fee, it means less income when the order is filled next time.

@pmconrad
Copy link
Contributor

The idea here is that market making improves liquidity, better liquidity attracts more traders, which boosts volume, which generates more fees.

Presumably a market maker will mostly cancel and re-create unfilled limit orders, which means the chain earns 1 cancel fee for an expensive limit_order_create and a cheap limit_order_cancel, whereas limit_order_update will be much cheaper to validate + execute than the other two and take less bandwidth. Note that even if the order has been partially filled there has been at least one other order created and fully filled, which earns the chain at least one limit_order_create fee.

@abitmore
Copy link
Member

abitmore commented Mar 4, 2019

Anyway, I've never seen such a feature in traditional exchanges, although in some exchanges it's possible to update an order's size to smaller (but no price change). That said, (I guess) this feature is not common, thus introducing it may probably confuse average traders. How would the UI look like? @clockworkgr @sschiessl-bcp

@sschiessl-bcp
Copy link
Collaborator

@sschiessl-bcp For the UI I imagine that you have an edit button in open orders, that pops out the Buy component as a modal

@pmconrad
Copy link
Contributor

pmconrad commented Mar 5, 2019

Since it is mostly meant for market-makers (who will be running bots), it need not be present in the UI. Except if the UI team want to have it.

@cloud-8
Copy link

cloud-8 commented Apr 6, 2019

A great addition to the protocol, this is often used on many of the popular CEX's and would make Bitshares more attractive to active traders.

@sschiessl-bcp
Copy link
Collaborator

sschiessl-bcp commented Apr 16, 2019

Since it is mostly meant for market-makers (who will be running bots), it need not be present in the UI. Except if the UI team want to have it.

UI team wants to support all calls, yet this is certainly targeting bots. A traditional exchange also would need this as order replacing doesn't cost anything.

@sschiessl-bcp
Copy link
Collaborator

Should we forward this into a PR?

@abitmore
Copy link
Member

@sschiessl-bcp shared an article https://hackernoon.com/the-ideal-crypto-trading-api-b1bbb2675875. IMHO it means it's better to have a "replace" operation rather than an "update" operation, which basically combines a limit_order_cancel_operation and a limit_order_create_operation into one operation. With this approach, we can reuse most of current code, and logic about fees would be clear as well.

@abitmore
Copy link
Member

Instead of adding a new operation, we can add an extension to limit_order_create_operation to indicate which order to be replaced.

@block-chained
Copy link

block-chained commented Aug 13, 2019

Is there a possibility to add "lock order" capability, so for example a user can place an order that cannot be revoked? E.g. to provide trustless automated token-to-token exchange cmart contract.

@nathanielhourt
Copy link
Contributor Author

If you're looking for an exchange smart contract with predefined parties, you probably want HTLCs, which are currently supported. An irrevocable market order could be taken by anyone in the market, which is different.

An irrevocable order is certainly possible; it could be added if there is sufficient support for the idea, but it would require a separate BSIP.

@ryanRfox
Copy link
Contributor

@nathanhourt this feels "done" to me (leaving out the bits about irrevocable order, which needs a distinct discussion). Please PR with your latest text.

@abitmore
Copy link
Member

Draft (#215) merged. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants