portfolio icon indicating copy to clipboard operation
portfolio copied to clipboard

PDF import from tastytrade and tastyworks

Open stockmarketpotato opened this issue 11 months ago • 12 comments

Issue: https://forum.portfolio-performance.info/t/pdf-import-from-tastytrade/24238

PDF import for tastytrade Trade Confirmations and Account Statements including support for Options trading and option's symbols following the OCC symbology standard.

Imports

  • Stock trades
  • Option trades
  • Option expiration and assignment
  • Dividends and interest payments
  • Debit interest
  • Funds paid and received
  • Journal to other accounts

stockmarketpotato avatar Aug 08 '23 08:08 stockmarketpotato

Hello @stockmarketpotato Thanks for this pull request.

I have had a little look at the source. We always try (95% all cases) to generate a unified source in the PDF importers. For this we have developed the Contributing Rules. We ask you to read them again and adjust your source code. Please have a look at the examples in the Contributing rules and implement the structure.

A good idea is, replace all [ ]+ with [\\s]+ if more than one space can occur. Otherwise a simple space .

I think the OccOsiSymbology.java has to be integrated into the PDF importer TastytradePDFExtractor.java, because it is used exclusively for this PDF importer. What do you think?

For taxes and fees, use the V-Bank Importer's identification of the addTaxesSectionsTransaction and addFeesSectionsTransaction. Could you imagine using them?

All in all, this is a good pull request and is not meant to be a criticism. 👍🏻 Good Job!

I would check again after your customization. Therefore, @buchen please put in "draft" again.

Regards Alex

Nirus2000 avatar Aug 09 '23 13:08 Nirus2000

Dear Alex, thanks for your fast review and your detailed guidance!😃

Please have a look at the examples in the Contributing rules and implement the structure.

Sorry, I was a bit too sloppy with the rules. The implementation puts more focus on functionality due to a tight time budget on my side. I will read the Contributing rules and implement the proposed structure.

I think the OccOsiSymbology.java has to be integrated into the PDF importer TastytradePDFExtractor.java, because it is used exclusively for this PDF importer. What do you think?

It depends on the relevance of options trading for PP and whether it is useful to other importers. I will merge it back into the importer. So we have that clean for now. It can be moved out into a separate file later, if needed.

For taxes and fees, use the V-Bank Importer's identification of the addTaxesSectionsTransaction and addFeesSectionsTransaction. Could you imagine using them?

I have no experience with Generic Types in Java yet. This is a good chance to learn about it and reduce some of the redundancies in the code.

All in all, this is a good pull request and is not meant to be a criticism. 👍🏻 Good Job!

Thank you 😊

I would check again after your customization. Therefore, @buchen please put in "draft" again.

I will try to provide the updated code by the end of CW33.

BR Sebastian

stockmarketpotato avatar Aug 10 '23 09:08 stockmarketpotato

It depends on the relevance of options trading for PP

Indeed, would be interesting to hear @buchen's opinion on whether option trading support might be added to PP (of course, support for short trades is on critical path for this).

In either case, in the name of https://github.com/portfolio-performance/portfolio/issues/3303 , I'd suggest to keep OccOsiSymbology.java separate for possible future reuse. This would be one small step towards growing something new cool in PP (like that option support). Of course, very small step, but dozens of such steps done by different people might lead somewhere. Whereas merging it into the importer won't lead anywhere by code duplication (next guy who'll need to deal with option symbols of course won't fish for it in another obscure importer, just write duplicate code again).

pfalcon avatar Aug 10 '23 10:08 pfalcon

Hello @stockmarketpotato

Sorry, I was a bit too sloppy with the rules. The implementation puts more focus on functionality due to a tight time budget on my side. I will read the Contributing rules and implement the proposed structure.

No problem... Sometimes I catch myself and have to do it again. The good thing about it is that through these repetitions of the source a quick understanding comes and then global changes can also be implemented very quickly. For example with function outsourcing.

It depends on the relevance of options trading for PP and whether it is useful to other importers. I will merge it back into the importer. So we have that clean for now. It can be moved out into a separate file later, if needed.

The idea of implementing options has been around for a long time, but it's just not that easy to implement. We have been thinking about how best to implement this for several months now and are in the process of identifying the problems and collecting them analytically. Including options simply with "Put &Call" is simply a whole new booking type with a cross behavior and must be considered separately. (Reference account <-> securities account) We would put the source for this in the "code wallet" to access it back later.... Could you think of pushing the pull request without the options for now? (maybe possible - your choise)

I have no experience with Generic Types in Java yet. This is a good chance to learn about it and reduce some of the redundancies in the code.

I can understand that... :smile: but you can do it. :muscle: 💯 If you have any questions... always here, we are happy about energetic contributors, who work permanently and continuously on PP.

Regards... and many thx Alex

Nirus2000 avatar Aug 10 '23 18:08 Nirus2000

I'd suggest to keep OccOsiSymbology.java separate for possible future reuse

I agree. If it is not specific to a one importer, place it where it is reusable.

About options in general: As I do not fully understand the implications, my strategy at the moment is: do not prevent the way options are currently maintained, but also do not introduce partial functionality that might required incompatible migration in the future. Personally, I just do not understand enough of the financial mathematics involved.

buchen avatar Aug 14 '23 07:08 buchen

Please have a look at the examples in the Contributing rules and implement the structure.

  • The structure is now reworked and comments have been added to make the code easier to understand. There are only two different types of documents. The entry points for the Trade Confirmation is addSummaryStatementBuySellTransaction() and the entry point for the Account Statement (aka Kontoauszug) is addDepotStatementTransaction(). The function addDividendTransaction() adds the dividend transactions which are also contained in the Account Statement. It could be added to addDepotStatementTransaction(), if you find it more suitable.
  • Names of detected values are updated.
  • There are some new test cases to cover options expiration and assignment.
  • I was able to figure out one place to use it. Do you think it makes sense? https://github.com/portfolio-performance/portfolio/blob/5485f042f134c94a9d9c30d21bce8fe9a7f4e2e2/name.abuchen.portfolio/src/name/abuchen/portfolio/datatransfer/pdf/TastytradePDFExtractor.java#L485
  • Do you see some other possible points for improvement?

Including options simply with "Put &Call" is simply a whole new booking type with a cross behavior and must be considered separately. (Reference account <-> securities account)

  • I lack the knowledge of the complete code base to make any reasonable proposal here. However, Options can be treated like any other Security in PP. They have an identification number, a name and a couple of distinctive properties. Maybe they could be added as attributes?
  • There are two properties which are special in some sense. One option contract contains 100 shares. That's why the importer multiplies with 100. That could be done under the hood.
  • The other property is expiration. A contract is removed from the account when it expires. This transaction appears in the account statement anyways. This is implemented in the current importer with a buy or sell transaction depending on the position size (long, short) with zero cost. Would a removal do the same thing, but more simple?
  • It is also very common to hold a short position and collect the premium (thetagang style) instead of betting on a long position. So it would not be necessary to warn on negative account balance for options.

Could you think of pushing the pull request without the options for now?

I'd prefer to keep the options. The options are added as a Security, like any other, and they do not need any special treatment. Quotes can be obtained from Yahoo using the OCC symbol.

I agree. If it is not specific to a one importer, place it where it is reusable.

Ok, I'll leave it in the utilities folder for now.

stockmarketpotato avatar Aug 15 '23 14:08 stockmarketpotato

@Nirus2000 thanks for your detailed suggestions. I will implement them all.

About this ^M thing, I have no clue what's going on there 🦧

stockmarketpotato avatar Aug 19 '23 08:08 stockmarketpotato

you need to upload in UNIX style. Maybe this helps you https://stackoverflow.com/questions/1889559/make-git-diff-ignore-m

Nirus2000 avatar Aug 19 '23 08:08 Nirus2000

Hello @stockmarketpotato I could imagine that you look at the other PDF importers again to internalize the structure as well as the hints we gave you. This could help us to better maintain the source code later when changes are made by other contributors.

Especially the comment from the main developer should make it clear to you that we don't want to process any options at the moment.

What do you think about it?

Regards Alex

Nirus2000 avatar Sep 06 '23 10:09 Nirus2000

Especially the comment from the main developer should make it clear to you that we don't want to process any options at the moment.

Hi Alex, I must have misunderstood the comment. It was not that clear to me. Especially since IBFlex importer does in fact process and import option trades https://github.com/portfolio-performance/portfolio/blob/5519b5caf1afc91ef80bf0817c73a0b6a0ceaf82/name.abuchen.portfolio/src/name/abuchen/portfolio/datatransfer/ibflex/IBFlexStatementExtractor.java#L737

What we can do is separate the options transactions in the importer, e.g. in separate functions or generics, and integrate them at a later point in time. What do you think?

Cheers Sebastian

stockmarketpotato avatar Sep 11 '23 08:09 stockmarketpotato

Hi @Nirus2000, @buchen, it's been some time since there has been any activity or updates from my end, and I appreciate your patience and the time you've already invested in reviewing it.

I wanted to propose removing all parts related to stock options to potentially make it more aligned with the project's goals. Would this change address any concerns and make the PR more acceptable?

I value your feedback and am open to any suggestions you might have. Looking forward to hearing from you.

BR Sebastian

stockmarketpotato avatar Mar 01 '24 13:03 stockmarketpotato