-
Notifications
You must be signed in to change notification settings - Fork 27
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 CI Workflow Action #50
base: master
Are you sure you want to change the base?
Conversation
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.
@weklund superb work altogether! I've fixed most of the issues on master branch.
What do you think we should do with the oauth1a.py
? There's a bunch of errors flagging up there, but this is code almost entirely copied over from IBKR.
- On one hand, we should leave it as is, so that it's easier to just bring in whatever fixes and changes IBKR rolls out.
- On the other hand, we should take responsibility of that code and fix any issues with it. This means if IBKR rolls out updates we'd need a bit more effort to bring these in.
Thoughts on this?
] | ||
|
||
ignore = [ | ||
"E501", # Ignore line length errors |
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 wanna bring up ignoring 'E712' - since in several locations we do care about doing if x==False
rather than if not x
, seeing that x can be None. Any objections?
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.
Nope. I actually was going to consider adding 2-3 other rulesets that I think are helpful for code quality, but I know we'll be opinionated and want to ignore several of the rules. Do you mind I add E712, and add the additional rulesets so we can see what we want to ignore?
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.
Great, what other rulesets you think of adding?
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.
[
"PL", # pylint
"DOC", # Documentation
"D", # pydocstyle
]
The findings could end up being pretty pedantic, more out of curiosity if we end up getting new findings that we actually see value in.
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.
This is resolved now?
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.
Not sure exactly what you're referring to regarding resolved. We have added several rule sets, and accompanying ignores that we either don't find value, or we're punting on for future refactors.
As of now ruff is passing with the current config
@Voyz I'm inclined on this. Good ownership for our users, regardless of how many times IB has changes (Hopefully not many) I'm happy to take a first stab at it, and if we could find a volunteer that has oauth onboarding to test the branch while we work on the PR |
@Voyz Just for reference, once we get linting green, then the workflow will move on to the bandit scan, and we'll likely have a similar workflow here where we fix/ignore to get to green before the PR is ready. |
Also side note, do you have a preference on draft PRs vs regular? Some teams like PRs in 'draft|wip' mode until the PR author is ready to be merged in or not. I have zero preference just wondering if you already had a convention here. |
* master: (68 commits) updated README small fix to tests renamed QueueControllerClass and SubscriptionProcessorClass arguments in IbkrWsClient to lower camel case minor lint fixes turned oauth into a subpackage of ibind and renamed oauth.py to oauth1a.py renamed OAuth nomenclature to OAuth 1.0a (or OAuth1a) in anticipation of OAuth 2.0 support removed print(client.live_session_token) added ability to catch request errors in IbkrClient and added better handling of 'no bridge' error breaking: removed redundant 'publish' parameter from initialize_brokerage_session which needs to always be set to True added support for calling initialize_brokerage_session in init_oauth added IBIND_OAUTH_WS_URL and default to it if no `url` is passed to IbkrWsClient maintain gateway support fix websocket oauth added oauth for websocket support updated rest_client tests added OAuthConfig.copy simplified oauth optional dependencies added oauth_rest_url to OAuthConfig simplified requirements-oauth.txt updated inline docs for IbkrClient ...
re: oauth1a, looking at this a bit closer, a lot of these initial linting errors is from using all caps where the variable is only local to the function. I've normally seen all caps when you're defining a constant that's more global that a bunch of functions will use. like:
And in each function that redefines, it is the same value. So to resolve instances where there's multiple uses, I'm going to shift it up, and keep the all caps. Other instances where it's only being used once I'll make lower case. |
Very good point. And yes, IBKR hardly ever updates this things historically. Thanks for sharing your thoughts, I agree this is the best way to go ahead.
Zero preference from my side as well. Sticking to regular PRs sounds good 👍
Superb! I appreciate you handling this, ping me when I could give it another review |
https://github.com/weklund/ibind/actions/runs/13320132207/job/37203063677 Over 4000 lines of linting to scroll through ha. Let's see what AI can summarize. Any of these look valuable? @Voyz The linting output you've provided highlights several types of issues in your codebase. Here's a summary of the main issues:
To address these issues, you should
|
Well frankly some of these are great! Some may need some lee way though.
All these sound silly and redundant to me. I cannot think how would this improve the code base. Additionally, the mixin comments are all 1-1 from IBKR docs, I'd feel it would be an unnecessary chore to try to come up with our own documentation only to meet these rules. What do you think?
This one is on a similar category, but I'm paying attention a bit more.
For the rest I'm on a fence. There's a ton of small methods or classes that are public but not sure if bloating them with docstrings would be a good or a bad move. Some come from examples or tests, where I'm not sure we should be writing docstrings, or care about their formatting. I think some kind of configuration to point this only at certain folders could help figuring out if these are meaningful - can that be done?
Half of these come from examples, which I give a pass. The other half come from valid areas where ints are parsed, eg: IBIND_WS_TIMEOUT = int(os.getenv('IBIND_WS_TIMEOUT', 5)) I don't know if changing that The only area where this actually properly highlights an issue is here, in IBIND_CACERT = os.getenv('IBIND_CACERT', False) But the expected interaction here is that user always sets this value to a path string. If it is absent, we do want it to be False, because this is how
These exclusively HTTP error codes, or are found in tests. I think I'd give this a pass.
That I'd love to fix, just no idea how. Any suggestions?
That's actually quite useful, though only appears in one place, I'll try fixing it. Should I do it on the master branch and you can merge it in? |
+1, let's add these to ignore.
I can go either way here. I'm less inclined since ibind is currently a bit smaller of a code base, however generally I'm slowly subscribing to the idea that verbose (while potentially ugly to look at) doc blocks helps future development/understanding, particularly with AI tooling. Giving the tools as much as context as possible should help when we have various prompts for analysis or development with less issues. Pedantic and verbose comment documentation kind goes against the vision for easy readability of Python syntax so I'm on the fence.
Hmm to make sure I understand, this finding is calling out that Generally I think this a good rule to keep to make sure we don't create new functions with overloaded parameters, but a/ might unlikely happen b/ having a fix for it might be more confusing than just os.getenv - so ultimately I think we should ignore this rule.
To ignore right? I think so. I didn't see a lot of other http codes mentioned. I only saw 400, 404, 401, and some values for test assertions. If we had various 2xx, 3xx, 4xx, 5xx http codes throughout the package then I think it would make sense to use enums from https://docs.python.org/3/library/http.html#http.HTTPStatus Safer and less likely to introduce errors.
Maybe? that I'd have to think on it, but I imagine it would require a descent amount of rewrite of the rest client. If we want to punt on it, we could add a single ignore for it above the finding, so we don't ignore the whole ruleset.
Sure thing. |
Love your work on linting, I'm looking at ruff output for the first time. Learning a lot, thanks!
Please let me know if I this is off topic for an initial CI workflow. I am lacking experience in how far one should go in making a linter happy. |
I'd say let's pass on module documentation for now then. If we encounter a good reason that would tip the scales let's revisit it.
Not so much the interpreter, it's just a some kind of consistency rule along the lines of "a method should return only one type". Since getenv will always return a string when it finds an env var, it is expected that its But in cases where the env var is expected to be an int or a bool, it is always directly fed into something like
Yeah, let's ignore it.
Yeah, that plus several other methods or constructors. IbkrWsClient has a pretty broad constructor too. That we could maybe break down a bit, but don't think we'd reach below 5.
def _request(
self,
method: str,
endpoint: str,
base_url: str = None,
extra_headers: dict = None,
attempt: int = 0,
log: bool = True,
**kwargs
) -> Result: Except for
Fixed 👍
@salsasepp great suggestion, appreciate you chipping in 👍 The In all fairness, moving it to a dataclass like you suggest wouldn't be a bad idea, but dictionary has the advantage in that it can be easily extended should a new parameter appear on the API. With a dataclass users won't be able to attach that new parameter easily without monkey patching, or overriding that dataclass.; with a dict adding a new parameter is legal and easy. We could work around that too, but this is just one of the many places where the linter observed too many parameters, so let's see if we can work out a solution that would fix things across the whole library first, and return to this idea if we fail to come up with anything else. |
Thank you! Totally overlooked that, my apologies. Makes my proposal obsolete. I can maybe see how dataclasses could reduce the argument burden with some of our |
Looks like AI missed a few. I made single instance ignores for rest client with todos attached. Here are other code quality based findings:
There were other instances of too many arguments but I omitted them, we get the picture. Just a heads up, I wanted to bring awareness to these other findings, but I think it's ok if we punt them, as long as we document in some way like TODOs to complete as a backlog item. |
@salsasepp no, I think your idea was actually the correct one, well done 👍 I read up a bit more about this and it seems that going config dataclasses is the most optimal way to do this. We can work around the translation part when using dataclasses. I'll try implementing this
Fixed it in one place, but in ibind/support/py_utils.py:313:17 it kind of makes sense to me. for key, value in optional.items():
if value is not None and value != [None]:
if preprocessors is not None and key in preprocessors:
value = preprocessors[key](value)
d[key] = value Thoughts on ignoring this?
Right, I know globals are bad, I dislike using them myself. Thing is,
Sure! Thought I enjoy being challenged by it and ask myself questions on whether things are correct. |
Hmm, what do you think about using a singleton here? Essentially having a LoggingManager that's instantiated once that can hold the state of both
So I believe the logic can stay the same, the finding is around the seeing a mutation of the same variable name as the loop variable. This might just go away by doing:
Regarding the other findings with too many arguments or too many branches: We can either bump the argument value to get to passing, or add single line ignores on each instance. I'm not too picky here. |
Thanks for prepping all that example code, its super helpful to see it right away 👍 I'm - possibly incorrectly - allergic to singletons ever since my uni professor used them everywhere 🤣 Jokes aside though, can you expand on how this would help, other than to remove that lint warning? Python's In fact, just deleting the LoggingManager singleton instance wouldn't suffice as a reset, since
Nice solution, thanks 👍
I think we may just ignore some areas, like the IbkrWsClient constructor. |
Hey @weklund just wanted to check in and see if we possibly could start closing in on introducing this? What are the outstanding questions here? |
Hey! Yep let's close this sooner rather than later :) On the logging topic, I do still think we would get value by having it's own singleton class. Now the value is super clear when we have tests, and when we extend the logging functionality. There's a case to be made to wait until those things happen. Value proposition:
Rather than having disconnected global variables like _initialized and _log_to_file, a singleton class keeps related state together in a single logical unit.
You're absolutely right that simply removing a singleton instance wouldn't reset the underlying logging system. However, with a class we could add an explicit reset method:
A class provides a clearer interface for managing logging behavior, making it easier for new developers to understand how logging is configured. Maybe it's just my opinion, but a module just doesn't seem like an explicit entity that we use logically. Open to being wrong.
If logging features grow, a class-based approach scales better than adding more globals like we are now. |
* master: added fixes for automatically registering signal handler on non-main thread v0.1.12 added load_dotenv monkey patch to display a warning if it is called after ibind has been imported updated .gitignore removed usage of _testcapi in test_utils.py and updated RaiseLogsContext with inline docs and docstrings Revert .gitignore Revert pyproject.toml Update Tickler to use threading events for stopping. No longer needs to wait for fix unit test added global default_filtering flag added OAuthConfig.verify_config changed 'localhost' default to '127.0.0.1' added extra notes to live_orders removed debug statements added use_session parameter and reworked the implementation of sessions to make it optional. Shutdown handling has been moved to RestClient, though `OAuth1aConfig.shutdown_oauth` parameter still defines whether OAuth gets closed when shutting down added way to specify custom parse_order_request mapping added is_close to OrderRequest parameters changed requests.requests for requests.Session
Pending alignment on the global vars topic, that finishes the ruff config. The last piece for this PR is the bandit security scanning. It does look like the finding is around pyCrypto library. I assume we don't want to rehash all the hard work you did for OAuth 😁 , but leveraging a deprecated library for security logic isn't ideal. What was the issue again that we couldn't use the cryptography library?
|
hey @weklund thanks for expanding on these. As for the logging singleton - I think I'm with you on the point that module just doesn't seem like an explicit entity that we use logically. Python treats them like these, and I'm accustomed to thinking this way already, but I agree that no only it is strange but also not very intuitive to embrace at first. Other arguments I think don't speak for either solution - it's all achievable in either system - but I'm inclined to give your suggestion a go. Would you like to give it a shot implementing the logging singleton? As for pyCrypto - interesting! It is the library used by IBKR. Most of the OAuth code is 1-1 copied from the code they distribute when signing up for OAuth. If there's a better what to do it without pyCrypto, it would require us to rewrite the OAuth logic. I don't feel nowhere near competent enough in internet security to do this reliably. If these were the two options:
Me not being knowledgeable in the topic and having to rely purely on trust of someone else doing a good job rewriting and later supporting that rewrite, I'd feel more confident with option A where we could just redirect all responsibility for any potential vulnerabilities to IBKR - at least until we have a larger group of established maintainers who could cross-validate that refactor. Hence, for now I think that anything related to pyCrypto we should just leave as it is until IBKR releases an update. I know that a similar topic came up earlier when it came to changing some variables from upper case to lower case. Small changes like these sound fine, but I don't think that level of refactoring would be reasonable at the moment.
It was only briefly introduced by Hugh (the author of OAuth 1.0a PR) in order to extract DH prime from a file automatically. I dropped it as I've had issues with reliably installing and distributing packages with |
Maybe a first step could be to make sure IBKR is aware of the issue? Try to put this on their agenda? If the worst case happens (i.e. an ibind[oauth] user gets their account hacked because of vulnerabilities in pyCrypto), it will be most difficult to prove that their code is to blame. Redirecting responsibility will only work if somebody is willing to take it. |
@salsasepp I agree it may be a good idea to let IBKR know about this. Can you elaborate a bit on your second point? It's an interesting discussion your point spawns - possibly worth its own issue. My view is as follows: Our responsibility certainly has its limit and whether someone else is willing to take it or not doesn't change it. I would say any parts that are copy pasted from IBKR fall outside of that scope. Another example of that is the documentation in the docstrings: almost all of it is verbatim from IBKR documentation, and I don't think it is my responsibility to ensure that they document their endpoints correctly. I think it's important to outline what this library is - an unofficial Python client wrapper around their API that makes its users' lives easier - and what it isn't - an attempt to fix their API and do their job for them. If the worst case happens, why would we be needing to prove anything? This is an open source library and in no place we provide any guarantees regarding security. If someone submits an order and loses money, would we also need to prove it's not our code that did something wrong? I think it's clear to see it's not on us at all. And if I'm wrong and the truth is contrary, then I'd say we should drop OAuth support completely, as I don't think anyone here would like to take on the responsibility for others' accounts getting hacked. Don't you think think a case of "your account got hacked because IBKR publishes outdated code" is much easier for us to deal with than "your account got hacked because we badly implemented OAuth"? Like I said, I'm not knowledgeable enough on the topic where I could publish the OAuth rewrite confident I did a better job than IBKR. If there are others here who can, I think there should be at least two active maintainers who could cross validate each other and stick around to provide maintenance to this module, as there I'd be more inclined to say that the responsibility falls partially on our side. That being said, I think it may be fair to outline this in the OAuth wiki, that this code is provided as-is from IBKR and contains known vulnerabilities. |
I believe I might have misunderstood your intention, and I was too brief in my answer, creating more misunderstanding. Apologies! I believe ibind, and its maintainer and contributors are totally safe based upon the terms of the ibind LICENSE. Let me rephrase my thought, from a trader's perspective: If ibind has vulnerabilities, because code copied verbatim from IBKR has vulnerabilities, and my account gets hacked because of said vulnerabilities, I believe it is highly unlikely that IBKR will take responsibility and compensate me. I am saying that, in this scenario, it does not help me in any way to redirect responsibility to IBKR. For lack of an official OAuth client free of known vulnerabilities, it is ultimately MY responsibility to either A) live with it, B) fix it, or C) stop using OAuth. We're both leaning towards A) for the time being.
From a contributor's perspective: yes. From a trader's perspective, it does not make any difference.
That is a good idea in any case, as well as opening a separate issue regarding possible pyCrypto replacement. BTW, where does IBKR's OAuth implementation come from? Is the code publicly available? For me, there was no onboarding process, I just requested OAuth access and that was it. |
@salsasepp I understand your point more now, I appreciate you expanding.
IBKR support sent it to one of the users before we started implementing OAuth. I guess they just give it out on case by case basis. |
These are great thoughts! Here's the way I see it: The moment we took code from another source (be it very likely the author of the API) and put it in Ibind's repo, Ibind took more ownership of the security model of Ibind's users to IB. I'm more inclined to be open to option B) in the future, and at least after contacting IB and seeing what they plan to do with the information about pyCrypto being depreciated. Regardless of the 'blame/proof chain', Ibind should do everything within it's control to secure its user's data and operational integrity. A shared responsibility model if you will. This includes defining for users what Ibind's security scope is and what its controls are. They aren't 'guarantees' per se, but more like what parts of the execution flow Ibind will consider in it's security scope, and what is not. As far as the risks for 'in-house' implementation, there's already risk just by supporting oauth. Because IB doesn't have a mechanism to update us if there's future changes to the implementation, the current code we have from them could still fail upon a new release of their implementation. Ibind should still provide this feature, just calling out that Ibind is already open to breaking changes. If that risk exists, I don't see it anymore risker to refactor their oauth to make it more secure and safer for Ibind users. On the topic of confidence, I completely agree that we should not take this lightly, and should really take it to a different threat than this PR to align on what the expectations are if we were to do option B). I'm open to at least thinking about it, as I believe it's the right thing to do for Ibind users, regardless of the author of the code. There's a few steps we can take, and seems like we're already aligned on a few:
|
I'd love to! I'm going to make a todo for this to unblock this PR so I can revisit. |
@Voyz I think we're getting close for this PR being ready! I would love another look at the full PR diff and see if there's anything we're missing. Running the linter there's also a lot of files with small diffs that I haven't been checking in yet. I was considering that once we approve the substantive changes. It's nit things like removing unused imports, f-strings when there's no vars to pass in, etc. |
Thanks for sharing your views @weklund 👍 This indeed sounds like a discussion for a separate issue. I don't exclude the possibility of having it rewritten it in the future. I'll update the Wiki and message the IBKR in the meantime. |
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.
Great progress altogether @weklund very cool initiative expanding the acronyms and adding meaningful inline comments 👏👏
I've added a few minor comments which should be easily fixable, other than that it looks great. Anything else that's pending for me to review other than the code?
] | ||
|
||
ignore = [ | ||
"E501", # Ignore line length errors |
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.
This is resolved now?
* master: re-added 0.1.13 dist v0.1.13
Hey @weklund the code looks great now, I'd say it's ready to merge 🙌 Is there anything else that's pending for me to review other than the code? |
* master: remaking session on ConnectionError only if self.use_session == True
Nope! It's ready! @Voyz I'm going to run the linter/formatter on this PR (lot of diffs incoming). Running it here so on the merge we don't have a failed CI - Let me know if anything looks off. |
hey @weklund I'm reviewing this recent change and it seems quite disrupting, and I don't think these rules are set in pep8 What didn't make sense:
Can we adjust the formatter's behaviour to avoid doing these? What did make sense:
|
Ok! I'm glad I checked in the formatter changes in so we can take a look before merging :) I'm having to burn midnight oil to get a feature for work in so apologizes for the delays between responses.
I have no preference personally, but I believe if we want consistently we probably want to configure ruff to either all single quotes or all double quotes. Don't believe it's context aware.
I believe this is max line length. Let me see what I can tweak here. Should I just consider things like function calls, imports can all go on one line?
My assumption here is that the formatter caught bad indentation and fixed. Do you have a good example? |
Ha, never heard that expression! 😅 No worries at all, no rush to get this out atm
Sure, all single on outside and double on inside then.
Ah right. Yeah, they can go on one line. I break them out when there's more than 3 parameters.
First thing I can find in that commit, from subscription_controller.modify_subscription, though any multi-line parameters got reformatted like this. It feels a bit arbitray.
Though admittedly maybe my IDE's one is arbitrary here... Yes, I found a setting to disable this indentation in my IDE. I think this is one more for the 'What did make sense' list 🙌 |
Github workflow that adds CI to Ibind.
Requirements based from #45
I've added ruff lint selectors that I believe is all PEP8 rules. Check out the linting errors from my runs: https://github.com/weklund/ibind/actions/runs/13299790209
Would like feedback on what was found, I'm happy to resolve them, or add those rules to the ignore list.