Petersoj / alpaca-java

A Java API for Alpaca, the commission free, algo friendly, stock trading broker. https://alpaca.markets
https://petersoj.github.io/alpaca-java/
MIT License
198 stars 84 forks source link

Added OAuthAlpaca Class Instantiation #78

Closed tamuseanmiller closed 3 years ago

tamuseanmiller commented 3 years ago

Haven't tested, and won't work with normal properties yet. I couldn't see another way to do this then make another class, so this is what I cam up with.

tamuseanmiller commented 3 years ago

Also, since these tokens are usually garnered dynamically, might be worth it to remove it from ".properties" to just a publicly accessible mutator. Probably better than going through the effort of writing to a file.

tamuseanmiller commented 3 years ago

Haven't tested, and won't work with normal properties yet. I couldn't see another way to do this then make another class, so this is what I cam up with.

Bump

Petersoj commented 3 years ago

Hey sorry for the late response. Duplicating a large piece of code and adapting it for another feature is generally not considered good practice. I think the best way would be to modify AlpacaRequest to take in an OAuth token as you did and then modifying the various constructors in AlpacaAPI to take in an OAuth token or by adding methods that implement the OAUth RestAPI flow (getting OAuth URLs and tokens) and modifying the underlying AlpacaRequest object within AlpacaAPI after an OAuth token has been acquired. I can take a better look at better solutions later this week. Also, for future PRs, make a new feature branch on your fork instead of committing directly to master.

tamuseanmiller commented 3 years ago

Hey sorry for the late response. Duplicating a large piece of code and adapting it for another feature is generally not considered good practice. I think the best way would be to modify AlpacaRequest to take in an OAuth token as you did and then modifying the various constructors in AlpacaAPI to take in an OAuth token or by adding methods that implement the OAUth RestAPI flow (getting OAuth URLs and tokens) and modifying the underlying AlpacaRequest object within AlpacaAPI after an OAuth token has been acquired. I can take a better look at better solutions later this week. Also, for future PRs, make a new feature branch on your fork instead of committing directly to master.

Yeah, totally hear you I didn't particularly like the approach just wanted some feedback. You think adding another string parameter to the constructors would work? Didn't quite want to implement the whole OAuth flow as well, I did mine using AppAuth which doesn't exactly work universally here, definitely something to add later on though. Appreciate the reply, and apologies for committing to master, again just wanted some quick feedback.

Petersoj commented 3 years ago

Yes, modifying the constructors for an oauth_token and including the header changes will work. I agree with you about letting the OAuth workflow be handled by the user. See my changes that implement OAuth integration and let me know what you think (and if you can test it).

Edit: CI is failing because the build stopped prematurely so ignore that. It builds fine.

Petersoj commented 3 years ago

I will add the OAuth for the Polygon implementation later.

tamuseanmiller commented 3 years ago

Yes, modifying the constructors for an oauth_token and including the header changes will work. I agree with you about letting the OAuth workflow be handled by the user. See my changes that implement OAuth integration and let me know what you think (and if you can test it).

Edit: CI is failing because the build stopped prematurely so ignore that. It builds fine.

Ah! I see what you did, makes it much more cohesive. I'm about to run some tests here in a few. For authentication on the frontend, do you wish to pass the token into the constructor every time?

I will add the OAuth for the Polygon implementation later.

And this is pretty easy, I think the Javadoc is the only real necessary change unless you just want to go through the trouble of introducing a new variable for the ID.

tamuseanmiller commented 3 years ago

Alright, everything works on my side!

Petersoj commented 3 years ago

Perfect. I'll add the Polygon implementation soon. As for your question about the OAuth token constructor, yes, most applications should really only need one AlpacaAPI instance since the constructor only needs authentication keys once (e.g. OAuth or keyID and secret) and aren't typically dynamic throughout a client-side application.

tamuseanmiller commented 3 years ago

Perfect. I'll add the Polygon implementation soon.

As for your question about the OAuth token constructor, yes, most applications should really only need one AlpacaAPI instance since the constructor only needs authentication keys once (e.g. OAuth or keyID and secret) and aren't typically dynamic throughout a client-side application.

For sure, sounds good. Appreciate the help Jacob :)

Petersoj commented 3 years ago

For sure, sounds good. Appreciate the help Jacob :)

You're welcome! Also, the OAuth implementation for Polygon requires no changes except that the library user must generate a keyID via the Alpaca OAuth endpoint that is then passed into the PolygonAPI constructor just like a keyID normally is. I'll integrate this into the next release. Thanks for contributing :)

tamuseanmiller commented 3 years ago

For sure, sounds good. Appreciate the help Jacob :)

You're welcome! Also, the OAuth implementation for Polygon requires no changes except that the library user must generate a keyID via the Alpaca OAuth endpoint that is then passed into the PolygonAPI constructor just like a keyID normally is. I'll integrate this into the next release. Thanks for contributing :)

Yessir! Integrating it an as we speak. Appreciate all the help!