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.

Sign up for GitHub

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

Jump to bottom

Add position intent to PlaceOrderRequest #294

Merged
merged 3 commits into from
Jul 9, 2024
Merged

Add position intent to PlaceOrderRequest #294

merged 3 commits into from
Jul 9, 2024

Conversation

ankitsridhar16
Copy link
Contributor

Resolves #289

  • PlaceOrderRequest is missing position_intent query param

@ankitsridhar16 ankitsridhar16 requested a review from gnvk as a code owner July 3, 2024 16:08
@ankitsridhar16 ankitsridhar16 mentioned this pull request Jul 3, 2024
Closed
gcatlin
gcatlin approved these changes Jul 3, 2024
View reviewed changes
Copy link
Contributor

@gcatlin gcatlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the issue mentioned re: Side, this looks good to me. @gnvk might want you to update rest_test.go.

Thanks for doing this work!

alpaca/entities.go Outdated Show resolved Hide resolved
@gnvk gnvk changed the title added position intent to PlaceOrderRequest Add position intent to PlaceOrderRequest Jul 9, 2024
gnvk
gnvk approved these changes Jul 9, 2024
View reviewed changes
Copy link
Collaborator

@gnvk gnvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you for your contribution!

@gnvk gnvk requested a review from said-saifi July 9, 2024 07:30
said-saifi
said-saifi approved these changes Jul 9, 2024
View reviewed changes
Copy link

@said-saifi said-saifi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Note: We are considering replacing the current options with to_open and to_close only, given that the side is already specified in the request. If we proceed with this change, we will maintain the original options for backward compatibility until everyone transitions to the new intents. Please note that this is still under evaluation and not yet confirmed.

@gnvk gnvk merged commit 8642da9 into alpacahq:master Jul 9, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gcatlin gcatlin gcatlin approved these changes

@said-saifi said-saifi said-saifi approved these changes

@gnvk gnvk gnvk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PlaceOrderRequest is missing position_intent query param
4 participants
@ankitsridhar16 @gcatlin @gnvk @said-saifi