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

Operation to modify existing limit order #1604

Closed
2 of 17 tasks
nathanielhourt opened this issue Feb 20, 2019 · 15 comments · Fixed by #2743
Closed
2 of 17 tasks

Operation to modify existing limit order #1604

nathanielhourt opened this issue Feb 20, 2019 · 15 comments · Fixed by #2743
Assignees
Labels
1b User Story The User Story details a requirement. It may reference a parent Epic. It may reference child Task(s) 2b Gathering Requirements Status indicating currently refining User Stories and defining Requirements 3b Feature Classification indicating the addition of novel functionality to the design 5a Docs Needed Status specific to Documentation indicating the need for proper documentation to be added 6 API Impact flag identifying the application programing interface (API) 6 DEX Impact flag identifying the Decentralized EXchange, market engine, etc. 6 Protocol Impact flag identifying the blockchain logic, consensus, validation, etc. hardfork

Comments

@nathanielhourt
Copy link
Contributor

User Story
As a market maker I (would) want an operation to modify an existing limit order so that order walls in the markets can be maintained cheaply.

Technically, this isn't for me, but @litepresence in Telegram has requested it, and it's an easy win, so I say we do it.

Impacts
Describe which portion(s) of BitShares Core may be impacted by your request. Please tick at least one box.

  • API (the application programming interface)
  • Build (the build process or something prior to compiled code)
  • CLI (the command line wallet)
  • Deployment (the deployment process after building such as Docker, Travis, etc.)
  • DEX (the Decentralized EXchange, market engine, etc.)
  • P2P (the peer-to-peer network for transaction/block propagation)
  • Performance (system or user efficiency, etc.)
  • Protocol (the blockchain logic, consensus, validation, etc.)
  • Security (the security of system or user data, etc.)
  • UX (the User Experience)
  • Other (please add below)

Additional Context (optional)
At present, modifying a limit order on the books requires a cancel/create combo, which is a double fee. Now that the fees have risen, there is some pressure to add a limit_order_update_operation which will allow for cheaply updating limit orders.

There are two ways to modify a limit order: update the price, or update the amount. Updating the price should be extremely cheap, as this involves only a modification to one small object and re-sorting the order books. Updating the amount should cost around twice as much, as it also involves modifying the account balance.

Question: what should the fees be set to?

CORE TEAM TASK LIST

  • Evaluate / Prioritize Feature Request
  • Refine User Stories / Requirements
  • Define Test Cases
  • Design / Develop Solution
  • Perform QA/Testing
  • Update Documentation
@nathanielhourt nathanielhourt self-assigned this Feb 20, 2019
@clockworkgr
Copy link
Member

Same as cancel or half of it imho...

Still, new op so up to the committee to decide :)

@litepresence
Copy link
Contributor

litepresence commented Feb 21, 2019

via telegram

litepresence, [20.02.19 18:13]
would update_op be any less bloat than cancel and create ?

Nathan Hourt, [20.02.19 18:14]
It's like 1/3
And waaayyy less processing

The whole purpose of development work on cancel order fees is to decrease chain bloat in the end. Nobody would use this feature if it cost the same as cancel because it takes more code and thought on client side to implement.

If updating is more efficient in both space and processing than we should at the very least discount it to match the decreased burden on the chain and servers. I recommend decreasing it even further to encourage its usage through subsidy by raising create and cancel; relatively inefficient means to the same end.

I would like to see:

create
cancel = 1/10 create
update = 1/10 cancel

I have made several recommendations over the past month to reduce the cost of cancel create operations from a technical standpoint:

maker/taker
minimum order size
taxing unreasonably priced orders more 
taxing high frequency in short period more than same number over longer period
merkle tree pruning of unrealized cancel operations
annual tax refund for activities technically deemed "market making" 

By comparison, limit_order_update should be 1) the easiest to implement 2) immediately tangible in results. Regardless of final fee, I think this is a great feature because it is a technical path to slow chain bloat and reduce server load other than raising taxes.

nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Feb 21, 2019
Define and implement the limit_order_update_operation. Still needs
testing.
@pmconrad
Copy link
Contributor

pmconrad commented Feb 21, 2019

IMO 1/10 of cancel fee is too low (but that's a discussion for the committee).

Implementation should consider that after modifying the order price, order matching need only be performed if the price is adjusted in favour of the buyer AND the order stays at or moves to the front of the book. In that case, processing cost is close to limit_order_create, otherwise processing cost is extremely low.

@abitmore
Copy link
Member

Create a BSIP please. https://github.com/bitshares/bsips/

nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Feb 22, 2019
Some major facepalms in here... haha! but overall, it's looking good.
@nathanielhourt
Copy link
Contributor Author

Oops, didn't mean to close

nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Feb 22, 2019
Some major facepalms in here... haha! but overall, it's looking good.
@pmconrad
Copy link
Contributor

I know coding is more fun, but for consensus changes we usually write a spec (aka BSIP) first. :-)

@nathanielhourt
Copy link
Contributor Author

Wuff, so much paperwork for such a small change! No wonder nobody's done this yet... ;P OK, I'll draft it up.

nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Feb 22, 2019
Add logic to match an updated limit order if the price is changed to be
higher than the previous top-of-book price.

I believe this completes bitshares#1604
nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Feb 22, 2019
Add logic to match an updated limit order if the price is changed to be
higher than the previous top-of-book price.

I believe this completes bitshares#1604
nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Feb 22, 2019
Add logic to match an updated limit order if the price is changed to be
higher than the previous top-of-book price.

I believe this completes bitshares#1604
@nathanielhourt
Copy link
Contributor Author

Holding off on a PReq until I've drafted up a BSIP (do I need someone to assign me a BSIP number?), but I welcome review ahead of then. My patch can be read here. Did I make any mistakes or forget something? :]

@oxarbitrage
Copy link
Member

In a quick view you have missed the hardfork dates and the checks for it. Operation cant be executed before hardfork. Need to check also in proposals.

@nathanielhourt
Copy link
Contributor Author

BSIP Draft: bitshares/bsips#150

@ryanRfox
Copy link
Contributor

I suggest we move the discussion to the BSIP61 discussion thread to finalize the specification, then return here for implementation (<-- assumption). We should keep this Issue open until the BSIP process completes.

@nathanielhourt
Copy link
Contributor Author

nathanielhourt commented Feb 24, 2019

TODO:

  • Change delta_amount_to_sell to share_type (Rejected)
  • Dust check or disable auto-canceling of dust orders
  • Hardfork guards

nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Feb 26, 2019
Add dust check to limit order update logic, and test
nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Feb 26, 2019
nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Feb 26, 2019
@abitmore
Copy link
Member

More hardfork guard code needed. Like this:

void operator()(const graphene::chain::committee_member_update_global_parameters_operation &op) const {
if (block_time < HARDFORK_CORE_1468_TIME) {
FC_ASSERT(!op.new_parameters.extensions.value.updatable_htlc_options.valid(), "Unable to set HTLC options before hardfork 1468");
FC_ASSERT(!op.new_parameters.current_fees->exists<htlc_create_operation>());
FC_ASSERT(!op.new_parameters.current_fees->exists<htlc_redeem_operation>());
FC_ASSERT(!op.new_parameters.current_fees->exists<htlc_extend_operation>());
}
}

nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Feb 27, 2019
Updating an order's expiration must make it expire later than before,
not the same or sooner.
nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Feb 27, 2019
Define and implement the limit_order_update_operation. Still needs
testing.
nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Feb 27, 2019
Some major facepalms in here... haha! but overall, it's looking good.
nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Feb 27, 2019
Updating an order's expiration must make it expire later than before,
not the same or sooner.
nathanielhourt added a commit to nathanielhourt/bitshares-2 that referenced this issue Feb 27, 2019
Guard against the comittee setting fees for the operation which is not
yet defined.
@nathanielhourt
Copy link
Contributor Author

More hardfork guard code needed.

Done

@pmconrad pmconrad added 1b User Story The User Story details a requirement. It may reference a parent Epic. It may reference child Task(s) 2b Gathering Requirements Status indicating currently refining User Stories and defining Requirements 3b Feature Classification indicating the addition of novel functionality to the design 5a Docs Needed Status specific to Documentation indicating the need for proper documentation to be added 6 API Impact flag identifying the application programing interface (API) 6 Protocol Impact flag identifying the blockchain logic, consensus, validation, etc. 6 DEX Impact flag identifying the Decentralized EXchange, market engine, etc. labels Mar 5, 2019
@abitmore abitmore linked a pull request Apr 30, 2023 that will close this issue
@abitmore abitmore assigned abitmore and unassigned nathanielhourt May 10, 2023
@abitmore
Copy link
Member

Done via #2743.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1b User Story The User Story details a requirement. It may reference a parent Epic. It may reference child Task(s) 2b Gathering Requirements Status indicating currently refining User Stories and defining Requirements 3b Feature Classification indicating the addition of novel functionality to the design 5a Docs Needed Status specific to Documentation indicating the need for proper documentation to be added 6 API Impact flag identifying the application programing interface (API) 6 DEX Impact flag identifying the Decentralized EXchange, market engine, etc. 6 Protocol Impact flag identifying the blockchain logic, consensus, validation, etc. hardfork
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants