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

Add FillEvent with common market order ID #315

Closed
wants to merge 5 commits into from
Closed

Add FillEvent with common market order ID #315

wants to merge 5 commits into from

Conversation

alnoki
Copy link
Member

@alnoki alnoki commented Jun 30, 2023

Summary of changes

  • Deprecates TakerEvent and OrderBook.taker_events in lieu of FillEvent and FillEventHandles.
  • Updates order book counter to increment for each order that is placed (incremented in match() and in place_limit_order()).
  • Modifies match() function to defer FillEvent emission in the case of a limit order.
  • Adds view function (did_order_post()) for determining if a market order ID corresponds to an order that posted to the book.
  • Enforces common market order ID across all FillEvent(s) when an order results in multiple fills, which is common to the MakerEvent market order ID in the case of a limit order that crosses the spread and fills as a taker before posting as a maker.
  • Implemented as a backwards-compatible package upgrade.

@vercel
Copy link

vercel bot commented Jun 30, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
econia ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 30, 2023 5:28pm

@vercel vercel bot temporarily deployed to Preview June 30, 2023 17:28 Inactive
alnoki added a commit that referenced this pull request Jun 30, 2023
Bump devnet listing for FillEvent paradigm (#315)
@alnoki alnoki requested a review from bogberry June 30, 2023 17:48
Comment on lines 13 to +17
Once a market is registered, signing users and delegated custodians
can place limit orders on the book as makers, takers can place
market orders or swaps against the order book, and makers can cancel
or change the size of any outstanding orders they have on the book.
can place limit orders on the book as makers and/or as takers,
takers can place market orders or swaps against the order book, and
makers can cancel or change the size of any outstanding orders they
have on the book.
Copy link
Contributor

Choose a reason for hiding this comment

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

This wording seems strange to me. There is no sense in which any given user is a maker or a taker—any given order can be a maker order or a taker order, and the user will be a maker or a taker with regard to that specific order, but that doesn't describe their relationship to the market itself.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tagging #321 to incorporate refactors on that iteration

Comment on lines -2772 to -2774
event::emit_event(taker_handle, TakerEvent{
market_id, side, market_order_id, maker, custodian_id:
maker_custodian_id, size: fill_size, price});
Copy link
Contributor

Choose a reason for hiding this comment

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

Backwards compatibility means that anyone who had built anything on top of a previous version of this code would not have to make any changes for their code to keep on running as expected.

As we are no longer emitting taker events, and any system that relied on taker events would have to make the switch over to fills events, this change is not backwards compatible.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bogberry Good point, backwards-compatibility is only in the sense that the contract itself will pass a bytecode verifier upgrade check, required to deploy to the same address on mainnet

Tagging #321 to incorporate refactors on that iteration

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, so it's not really backward-compatible. Aptos simply performs a minimal check where the struct declarations and function signatures are compared to catch any changes that are obviously not backwards compatible, as far as I'm aware. This is in no way a guarantee of backwards compatibility, and since we know that this in fact isn't a backwards compatible change, we should stop describing it as such.

Comment on lines +3244 to +3245
// Increment order counter.
order_book_ref_mut.counter = order_book_ref_mut.counter + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment simply repeats code

Copy link
Member Author

Choose a reason for hiding this comment

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

Tagging #321 to incorporate refactors on that iteration

@alnoki
Copy link
Member Author

alnoki commented Jul 7, 2023

Closing in lieu of more comprehensive #321

@alnoki alnoki closed this Jul 7, 2023
@alnoki alnoki deleted the fill-events branch July 7, 2023 21:38
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.

2 participants