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

fix: Incorrect documentation for timestamp property of PortfolioHistory model #555

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

Conversation

fumoboy007
Copy link
Contributor

The timestamp is the end of the time window, not the start. I verified this by checking the API response against the timestamp of a cash deposit in my account.

For example:

  • The actual timestamp of the cash deposit was 2025-01-22T13:49:48-08:00.
  • get_portfolio_history with 1-hour granularity shows the cash deposit at timestamp 2025-01-22T14:00:00-08:00.

…ry model

The timestamp is the end of the time window, not the start. I verified this by checking the API response against the timestamp of a cash deposit in my account.

For example:
- The actual timestamp of the cash deposit was 2025-01-22T13:49:48-08:00.
- get_portfolio_history with 1-hour granularity shows the cash deposit at timestamp 2025-01-22T14:00:00-08:00.
@fumoboy007 fumoboy007 requested a review from hiohiohio as a code owner January 24, 2025 02:32
@hiohiohio
Copy link
Contributor

@fumoboy007 Thank you for the report. May I please ask the request parameters (GetPortfolioHistoryRequest) you are calling and approx time, also paper or live? The docstring is actually copied from API reference document [1]. Therefore, I would like to confirm the details.

*1 https://docs.alpaca.markets/reference/getaccountportfoliohistory-1

@fumoboy007
Copy link
Contributor Author

fumoboy007 commented Jan 24, 2025

May I please ask the request parameters (GetPortfolioHistoryRequest) you are calling and approx time, also paper or live?

https://api.alpaca.markets/v2/account/portfolio/history?timeframe=1H&intraday_reporting=continuous&start=2025-01-22T12%3A00%3A00-08%3A00&pnl_reset=per_day&end=2025-01-22T15%3A00%3A00-08%3A00&cashflow_types=ALL

The docstring is actually copied from API reference document.

Hmm interesting, I guess we need to update that documentation too. It would make more sense to be the end of the time window anyways because the equity property uses the value at the end of the time window.

@fumoboy007
Copy link
Contributor Author

(Probably a good idea to double-check the back-end code though.)

@hiohiohio
Copy link
Contributor

@fumoboy007 Thank you for the details. It really helps us to investigate. Agree to double-check the back-end code.

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.

2 participants