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

fixed eWrapper for openOrder and orderStatus #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Javdat
Copy link

@Javdat Javdat commented May 27, 2016

eWrapper was not fully implemented for openOrder and orderStatus.

@joshuaulrich
Copy link
Owner

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
Copy link
Author

Javdat commented May 27, 2016

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
Copy link
Owner

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
Copy link
Author

Javdat commented Aug 19, 2016

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
Copy link
Owner

joshuaulrich commented Aug 19, 2016

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
Copy link
Author

Javdat commented Aug 19, 2016

Duly noted

@joshuaulrich
Copy link
Owner

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
Copy link
Author

Javdat commented Sep 19, 2019

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:
19695d6#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()).

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.

None yet

2 participants