Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Added concepts.Order class #13

Merged
merged 3 commits into from

3 participants

@StevenLooman

Added concepts.Order class

Also made some changes to the Trade class. e.g., the market paramater for init was required (through an assert) but was a named parameter.

@StevenLooman

Using a boolean would be possible, but seeing 'BUY' or 'SELL' in the debugger is clearer than seeing True or False.

Also, I don't think you'll have thousands of Orders in memory; sacrificing readability for the 16/17 bytes per instance (see below) seems like a over-optimisation. If really needed, we can optimise later on.

@StevenLooman

Maybe we do actually. The depth book ('open orders') wil possibly hold a thousand orders or more. Anyway, for now I suggest we stick to 'BUY'/'SELL' and, if needed, optimise later to a boolean.

@goncalopp
Owner

Agreed. As long as the constant is used, it's a one line change

@goncalopp goncalopp merged commit ceda39e into from
@goncalopp
Owner

I was going through Order again:

    For now, more specific properties can be set through the properties
parameter of the constructor.

Does it make sense to have properties set as a string? Why not have the markets simply override the constructor? It makes the properties explicit...

@goncalopp
Owner

Also, do we actually need buy_or_sell? It can be inferred from the market's currencies, so it seems to be repeated info
Additionally, I was thinking it would be desirable to let the order exchange_rate be None, and, in that case, the market will substitute it with the proper bid/ask rate so that the order can be fulfilled optimally.

If these two are done (and some minor tweaks), the following becomes possible:

#I believe the EUR will crash, so I want to sell all my EUR in all my markets, exchanged to whatever currency the market buys OR sells in
for p in all_available_active_participants:
    p.placeOrder( from_amount=Amount( 9999999999, EUR))

The API is then able to take care of the (arguably) minor details for you

@StevenLooman

I think buy_or_sell is preferred, it makes it explicit if the order is a bid or ask order. You can derive it from the Market instance, by looking at the currencies, but this makes the code more complex and therefore more error prone.

About the code, if you do this, the user needs to understand he is creating a 'market-order.' I.e., execute my order for any price. I think it would be best that the user explicitly needs to specify what kind of order he wants (limit, market, ...)

The current implementation of MtGoxMarket.placeBidOrder() implicitly makes limit order (you specify at what price you want to buy), so we might need to change this too, by adding a property 'order_type'. Then, we can also make the 'price'-parameter optional, as with a market-order, no price is given.

If you agree, can you make a new issue? Then I'll create a pull request. Or, of course, you can solve it yourself. :)

@goncalopp
Owner

You can derive it from the Market instance, by looking at the currencies, but this makes the code more complex and therefore more error prone

I'm not sure if you've checked 9e0f0ef and 956068c ?

Implementing bid/ask inferring is a simple one line change, as far as ActiveParticipant is concerned. Of course, all of this must be tested to infinity and beyond (I've been doing test scripts to make sure what I write is at least superficially correct, but it's not really production code)

OTOH, what happens if bid/ask is explicit and the from_amount conflicts with the market buy/sell currency? There's conflicting duplicate information, and I'd argue that that a exception must be thrown to warn the user.

I think it would be best that the user explicitly needs to specify what kind of order he wants

About the code, if you do this, the user needs to understand he is creating a 'market-order.

So, two points here. The first is the classic explicit is better than implicit. The second is: are the semantics clear enough?

About the first: are you familiar, by change, with the requests library? With raving reviews and being used by a lot of important companies, I think it's a perfect example that, at the user level of a API, being implicit is much more powerful, by using sane defaults.

About the semantics:

p.placeOrder( from_amount=Amount( 9999999999, EUR))

To me, it's very clear that you are placing a market order - since you're not setting the price yourself!

p.placeOrder( from_amount=Amount( 9999999999, EUR), exchange_rate=... )

Ît also seems clear that, here, the order has a fixed exchange rate ("price").
But maybe I'm biased here, since I've been thinking this way all along?

The current implementation of MtGoxMarket.placeBidOrder() implicitly makes limit order (you specify at what price you want to buy), so we might need to change this too, by adding a property 'order_type'. Then, we can also make the 'price'-parameter optional, as with a market-order, no price is given.

I implemented a initial SimpleMarket on d6bfa1e , as we had discussed before.
Maybe we do need an ExplicitMarket ;)
We seem to have very different ideas as to the explicitness of the API interface. At this point you've probably done almost as much for mexbtcapi as I have myself; your help has been invaluable and I surely want to avoid a fork. It doesn't help at all that we have no third party opinion here.
As a last resort, I suggest that we set Market as a internal-use-only class and do the said Implicit/ExplicitMarket

@StevenLooman

I hadn't seen the commits when I wrote the comment. As you noted already, I prefer explicit over implicit. I have seen libraries which have sane defaults and make their API 'feel' very natural, which is a good thing. I do, however, think that an error is easily created in our case. For example, if price is None, through a mistake/by a bug in the client-code, a market-order is created while the intention was to create a limit-order (for example.) If you accidentally sell 100BTC against a very low/unintended rate... ouch :) (Not that I have that many BTC, but just an example.)

To elaborate, if you add a parameter 'properties' which contains, for example, 'market' or 'limit' to specify the order type, the implementation of the ActiveParticipant can validate the saneness of the parameters. If you specify 'limit' as order type, and price is missing, an exception should be raised.

Having placeBidOrder/placeAsklOrder makes the wanted actions more explicit as well and lowers the rate for error.

I don't think we should create two versions of the same thing, apart from some testing. It is better to have a single way to do something. Sometimes trade offs have to be made. At the moment we are building the core of the library. This is when the biggest decisions have to be made. We have to give it enough thought, but also make a decision at a certain time.

And don't worry about a fork.

@goncalopp
Owner

For example, if price is None, through a mistake/by a bug in the client-code, a market-order is created while the intention was to create a limit-order (for example.)

If we do agree that the semantics of the implicit interface are clear enough (I'm not sure, since you didn't comment on that), the point you raise is 'how to protect the user against user mistakes'. There's certainly no way to do that (in a absolute sense) and still have a functional API; even the securest of the operating systems is powerless to defend the user against himself. Asimov explored this problem to great depths, too.
With that being said, we can certainly try to make the user more aware of its decisions, and its consequences - since, as you mention, we're playing with real money. I don't think we should babysit the user against his will, though (remember the Windows UAC fiasco?). I (as a API user) shouldn't have to confirm twice that I REALLY want to pay for that coffee.

So I think this boils down to the user's choice and need for some hand-holding.

Having placeBidOrder/placeAsklOrder makes the wanted actions more explicit as well and lowers the rate for error.

Same here. Explicit is more secure and less "natural", as you put it. It's also simpler, in this particular case.

I don't think we should create two versions of the same thing, apart from some testing. It is better to have a single way to do something.

A single way of doing it means no choice for the user, though. I agree that we shouldn't have to code everything twice (it would be bad design, anyway), but, since hand-holding is nothing more than additional verification code, maybe it would be cleaner to develop it as a wrapper (OOP).

Other options are:

  • passing ONE MORE additional parameter, indicating if we want hand-holding (I'm only half joking...)
  • overloading methods (as much as we can, in python...)
  • A global HAND_HOLD variable (or a local variable in the relevant classes)
  • not providing choice at all, provide a single way to do things (either explicit or implicit)

At the moment we are building the core of the library. This is when the biggest decisions have to be made. We have to give it enough thought, but also make a decision at a certain time.

Indeed. If you plan to continue investing heavily into mexbtcapi, I think it's time to give you direct repository access, if you will. We can keep working on separate branches and opening issues, or maybe some other method. The fine guys at stackexchange mentioned they're using Trello "for keeping track of who is working on what", but I haven't tried it yet.

@goncalopp
Owner

@StevenLooman
Let me know if you want repo access, then

@zuijlen

This and other comments by zuijlen were moved to #18 --goncalopp

@StevenLooman

@goncalopp At the moment I'm rather busy, but I expect to have more time in the weekend.

For now, pull requests work best as we have different ideas about the API. I want to avoid that I commit something directly to your repo, and you have to/are going to reverse every commit. By using a pull request, we can discuss the changes first before merging. But thank you for the offer!

@zuijlen

This person seems also interested in the idea to set up a BTC forex: http://www.reddit.com/r/Bitcoin/comments/185atl/i_have_a_university_education_in_finance_would/
I sent him a link to MExBtcAPI

@goncalopp
Owner

@zuijlen
That looks cool, more advanced and reliable exchanges (and lower fees) are always needed. I'm not sure if mexbtcapi will be of any use to him if he doesn't code, bu maybe he'll get interesting contacts here.

I moved the discussion about the advanced order types to #18

@goncalopp
Owner

@StevenLooman
Ok, no problem. Discussion is definitely needed at this stage - I'll try to keep disruptive changes on a separate branch

@StevenLooman

Sorry for the late response. I have been busy with other things.

I have experimented a bit with a implicit API. Specifically, if price is None, then create a Market order, otherwise a Limit order. Seems to be working fine, so please continue on the implicit API (if you haven't done so already.)

If users of the library really feel the need for the API to be explicit, I'm sure a ticket will pop up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 59 additions and 15 deletions.
  1. +59 −15 mexbtcapi/concepts/market.py
View
74 mexbtcapi/concepts/market.py
@@ -6,26 +6,70 @@ class Trade(object):
"""Represents an exchange of two currency amounts.
May include the entities between which the trade is made
"""
- def __init__(self, from_amount, exchange_rate, from_entity=None,
- to_entity=None, opening_time=None, closing_time=None,
- market=None):
+
+ def __init__(self, market, timestamp, from_amount, exchange_rate):
+ assert isinstance(market, Market) # must not be null
+ assert isinstance(timestamp, datetime) # must not be null
+ assert isinstance(from_amount, Amount)
+ assert isinstance(exchange_rate, ExchangeRate)
+
+ self.market = market
+ self.timestamp = timestamp
+ self.from_amount = from_amount
+ self.exchange_rate = exchange_rate
+
+ def __str__(self):
+ return "{0} -> {1}".format(self.from_amount, self.exchange_rate)
+
+ def __repr__(self):
+ return "<Trade({0}, {1}, {2}, {3}>".format(self.market, self.timestamp,
+ self.from_amount, self.exchange_rate)
+
+
+class Order(object):
+ """Represents an order to buy or sell a number of from_amount for
+ exchange_rate.
+
+ For now, more specific properties can be set through the properties
+ parameter of the constructor.
+ """
+
+ BUY = 'BUY'
+ SELL = 'SELL'
+
+ def __init__(self, market, timestamp, buy_or_sell, from_amount,
+ exchange_rate, properties="",
+ from_entity=None, to_entity=None):
+ assert isinstance(market, Market) # must not be null
+ assert isinstance(timestamp, datetime) # must not be null
+ assert buy_or_sell in [self.BUY, self.SELL]
assert isinstance(from_amount, Amount)
assert isinstance(exchange_rate, ExchangeRate)
+ assert isinstance(properties, str)
assert all([x is None or isinstance(x, Participant) for x
in (from_entity, to_entity)])
- assert all([x is None or isinstance(x, datetime) for x
- in (opening_time, closing_time)])
- assert isinstance(market, Market) # must not be null
+
+ self.market = market
+ self.timestamp = timestamp
+ self.buy_or_sell = buy_or_sell
self.from_amount = from_amount
self.exchange_rate = exchange_rate
+ self.properties = properties
self.from_entity = from_entity
self.to_entity = to_entity
- self.opening_time = opening_time
- self.closing_time = closing_time
- self.market = market
+
+ def is_buy_order(self):
+ return self.buy_or_sell == self.BUY
+
+ def is_sell_order(self):
+ return self.buy_or_sell == self.SELL
def __str__(self):
- return "{0} -> {1}".format(self.from_amount, self.to_amount)
+ return "{0} -> {1}".format(self.from_amount, self.exchange_rate)
+
+ def __repr__(self):
+ return "<Order({0}, {1}, {2}, {3}>".format(self.market, self.timestamp,
+ self.from_amount, self.exchange_rate)
class Market(object):
@@ -80,13 +124,13 @@ def placeTrade(trade):
def cancelTrade(trade):
raise NotImplementedError()
- class TradeAlreadyClosed(Exception):
- """occurs when trying to cancel a already-closed trade"""
+ class TradeAlreadyClosedError(Exception):
+ """Occurs when trying to cancel a already-closed trade"""
pass
- class NotAuthorized(Exception):
- """Occurs when the user is not authorized to do the requested
- operation"""
+ class NotAuthorizedError(Exception):
+ """Occurs when the user is not authorized to do the requested operation
+ """
pass
Something went wrong with that request. Please try again.