joshuaulrich / IBrokers

R API to Interactive Brokers Trader Workstation
65 stars 54 forks source link

fixed eWrapper for openOrder and orderStatus #13

Open Javdat opened 8 years ago

Javdat commented 8 years ago

eWrapper was not fully implemented for openOrder and orderStatus.

joshuaulrich commented 8 years ago

Please describe (or better yet, show an example) of what behavior is broken, and then provide the rationale for your proposed changes. It's difficult to evaluate your patch without any context.

Also, please open an issue before submitting a pull request, so I (and/or others) can provide input before you spend time making changes to code.

Javdat commented 8 years ago

Well, basically there is already eventhandler for OPEN_ORDER and ORDER_STATUS but they are not being used.

processMsg(curmsg,con,ewr) when curmsg==.twsIncomingMSG$ORDER_STATUS orcurmsg==.twsIncomingMSG$OPEN_ORDER is basically dumping incoming message from IB, even though eWrapper is specified (which is not being used)

Thus requesting from IB writeBin(c(.twsOutgoingMSG$REQ_OPEN_ORDERS,"1"),con)

and feeding it back to processMsg(curmsg,con,ewr) is giving raw results.

This change makes sure that if curmsg==.twsIncomingMSG$OPEN_ORDER and default ewrapper is used, then return output of e_open_order(msg) instead of just c(curMsg, msg) itself...

Similarly, when curmsg==.twsIncomingMSG$ORDER_STATUS ,e_order_status(curMsg, msg) is actually being calculated, but c(curMsg, msg) is returned. Why? Why calculate e_order_status(curMsg, msg) if its not used?

So, changes make sure that in these cases of Open_Order and Order_Status, correctly formated results by eventhandler (which is already implemented in the package) is used.

This is inline with every other correctly implemented code in eWrapper.R. tickPrice, tickSize, tickOptionComputation etc all use their appropriate e_tick_option etc.

openOrder, openOrderEnd etc are just not finished.

orderStatus is semi implemented. it uses e_order_status but then rewrites it with c(curMsg, msg)

joshuaulrich commented 8 years ago

Your last commit changes 91 files with 1,731 additions and 1,375 deletions. I will not merge that. Please undo all the whitespace, newline, and file mode changes. Then use git commit --amend to update the commit, and git push --force to push it.

Javdat commented 8 years ago

Hi,

I know you will not merge it. I didn't take into account that the changes will trigger this pull request. I can obviously, change it. But, as far as I can see you are not planning to merge the previous request as well.

Thus, I do not see much point of it. But I have updated it anyway.

joshuaulrich commented 8 years ago

That I haven't merged it yet doesn't mean I don't intend to ("'no' is temporary; 'yes' is forever"). I do not use this package for my job, nor do I use it personally, so evaluating and merging pull requests for it is difficult and not at the top of my priority list.

Also, it is good Git practice to create pull requests using a branch in your fork. If you create a pull request on your version of master, and I do not accept it verbatim (e.g. I make changes/rebase before merging), then you may have a conflict when you try to sync your fork.

Javdat commented 8 years ago

Duly noted

joshuaulrich commented 4 years ago

Hi @Javdat. I know it's been a long time since you submitted this PR... I'm sorry I wasn't able to resolve it sooner. Can you please see if the changes I made for #19 fix the issue you addressed here? Thanks!

Javdat commented 4 years ago

Hi. yes, it has been a long time and I have switched into python wrapper. So, it will take me sometime to test this out.

But from a quick look, I think the problem is not completely solved.

Looking at: https://github.com/joshuaulrich/IBrokers/commit/19695d6ad7c39a383126eaff47217f726ff2b993#diff-64caf15b28203b231b864d096a8625c5

openOrder() know correctly calls e_open_order() which assigns message contents to named list eoo

But line 87 in eWrapper.R defeats the most of the benefit of creating this eoo as it is never returned to the user. Instead user receives raw messages: c(curMsg, msg)

openOrder  <- function(curMsg, msg, timestamp, file,  ...) {
      e_open_order(curMsg, msg)
      c(curMsg, msg)
    }

orderStatus() has the same issue. New commit returns eos from e_order_status() but it is never returned from orderStatus() thus does not reach the user.

orderStatus <- function(curMsg, msg, timestamp, file,  ...) { 
      e_order_status(curMsg, msg)
      c(curMsg, msg)
    }

In both cases c(curMsg, msg) should be removed. From my understanding c(curMsg, msg) is useful only if there is no corresponding wrapper to handle messages (e_open_order() and e_order_status()).