-
Notifications
You must be signed in to change notification settings - Fork 101
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
[order,matcher]: Define the Order type and begin matcher. #15
Conversation
Warning! This adds several PLACEHOLDER/TODO sections. Deviations from spec: - hash func is blake256, not sha256. Easily changed. - market orders do not have time-in-force, only limit orders do. market/order: Create the Order type under the server/market/order package. Implement order serialization and order ID calculation. matcher: Add sortQueue and shuffleQueue functions, to be used in order matching. The Matcher type and its Match method are placeholders. asset: Primarily a placeholder, but create an AssetType enum and UTXO type for use in the order.Order type. account: Primarily a placeholder, but create an AccountID type with a constructor to double-hash a pubkey as per spect. account/pki: Primarily a placeholder. Just define PrivateKey in terms of secp256k1, and pub/priv key size consts similarly. book: Entirely a placeholder. When the matcher.Booker interface is completely specified, the OrderBook methods can be implemented. market: Entirely a placeholder. Create a stub Market type with comments outlining the primary intended functions. Hint at the need for a concrete EpochQueue type, which should either implement some interface defined by the matcher package, or have a primitive field of perhaps []*order.Order that can be provided as a concrete type to the matcher.
Add Filled field to MarketOrder (and LimitOrder). Remove OrderSide enum and use a Sell bool instead.
Completed test coverage of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of questions, otherwise good to go.
server/market/order/order.go
Outdated
type Prefix struct { | ||
AccountID account.AccountID | ||
BaseAsset uint32 // must be 0 for CancelOrder | ||
QuoteAsset uint32 // must be 0 for CancelOrder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any thoughts on how these uint32 asset IDs might be generated? In the spec, they are ASCII-encoded ticker symbols. The reasoning there was that the ticker symbol is unique and lends itself to auditing. The downside is that they are variable length.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine the DEX will be in charge of mapping the integer identifier to the ticker symbol string. Perhaps the "asset object" returned to the client in response to the assets
RPC in https://github.com/decred/dcrdex/blob/master/spec/README.mediawiki#exchange-variables can be updated with the integer ID.
My thinking about this Prefix
struct is that it's primary purpose is byte serialization for computing order ID and storing the order data compactly, while the ticker symbol strings would come in with JSON serialization, and that would be something handled by something higher up like the dex or communications hub.
I suppose I'd be open to having these fields as arrays (i.e. [5]byte
), but I wanted to avoid a string here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[5]byte
is interesting. We need it to be repeatable for future mesh interoperability. If we wanted an int, we could potentially use the BIP-0044 coin type index.
https://github.com/satoshilabs/slips/blob/master/slip-0044.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BIP-0044 coin types makes a lot of sense. I'd go with that as a convention unless there are strong architectural benefits of putting the ticker characters in the order.Prefix object, but I think the ticker mapping can exist at a higher level in the dex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #29
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An itch that I still can't quite scratch is that these two asset IDs don't apply to a cancel order, or don't have to at least.
- Should cancel order prefixes be validated to ensure these values are as expected for the order in question (not much point of that)?
- Should they be required to both be some other sentry value like uint32(-1)?
- Should the asset types be moved from the prefix to the MarketOrder (and thus LimitOrder too)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see it either way. I like to think that if I was handed a serialized cancel order, I would be able to tell what market it's for, but you're right that it's not a necessity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, we'll set these asset types for cancel orders, but I'm not sure if we'll want to bother validating them for incoming cancel orders.
Checking the quote asset quantity of market buy orders against lot size, best sell order rate, and the fractional market buy buffer should occur only when validating incoming orders, not in the matching process.
Price rate is atoms of quote asset, applied per 1e8 units of the base asset. Add matcher.BaseToQuote and matcher.QuoteToBase to perform the big integer math with an integer rate.
Warning! This adds several PLACEHOLDER/TODO sections.
Deviations from spec (updated via #16):
Code
market/order
:Order
interface under the server/market/order package.OrderID
type.matcher
:Matcher
type.(*Matcher).Match
method and the unexported helper functions.:Booker
interface for interacting with the order book.account
: Primarily a placeholder, but create an AccountID type with a constructor to double-hash a pubkey as per spec.account/pki
: Primarily a placeholder. Just define PrivateKey in terms of secp256k1, and pub/priv key size consts similarly.market
: Entirely a placeholder. Create a stubMarket
type with comments outlining the primary intended functions. Hint at the need for a concreteEpochQueue
type, which should either implement some interface defined by the matcher package, or have a primitive field of type[]order.Order
that can be provided as a concrete type to the matcher.