portfolio icon indicating copy to clipboard operation
portfolio copied to clipboard

CSVAccountTransactionExtractor: Do not blindly ignore amount sign

Open pfalcon opened this issue 8 months ago • 4 comments

CSV importer's existing mental model is that each transaction would be explicitly categorized (via a "type" column) as Buy or Sell. It then just ignores the sign of transaction amount (by calling Math.abs()).

This assumption is not realistic to cover all the cases. Another way is to have a generic "security transaction" type, and the sign of the amount signifies the actual operation, e.g. negative for Buy, positive for Sell.

This change allows to handle such CSV statements, by "inverting" the configured transaction type. For example, if given type column value is mapped to "BUY", but amount is negative, it is turned into "SELL".

pfalcon avatar Oct 29 '23 11:10 pfalcon

This addresses p.2 from https://github.com/portfolio-performance/portfolio/issues/3616

pfalcon avatar Oct 29 '23 11:10 pfalcon

I have my doubts here that this is a good idea....

Several problems will arise here.

  1. If the sale value of the shares is less than the fees. (Sale with negative amount)
  2. Hereby cancellations are made possible of purchase and sale and not removed. The transactions are posted or duplicated at the wrong time. (Purchase or sale cancellation from day X).
  3. The same applies to reverse transactions of deliveries and removals.
  4. Also, here is created the possibility to book via the CSV options... (Call / Put). The "Put" would then be negative, whereby these are then not recorded as fees.
  5. It would not matter which transaction is identified. As soon as any negative zusatzpunkte are added incorrectly booked. (e.g. dividends, where the tax burden is greater than the dividend itself).

Nirus2000 avatar Oct 30 '23 16:10 Nirus2000

@Nirus2000 writes:

Several problems will arise here.

Let's recap what this change enables.

Let's say we have a CSV of this format:

date;type;amount
2023-10-31;Ein-/Auszahlung;2000
2023-11-01;Ein-/Auszahlung;-1000

In this case, the type cannot be used to determine the transaction - the sign of the value is needed as well. The sign of the amount can be used to make it a DEPOSIT or REMOVAL (= withdrawal). In this case, one can import the CSV as a 2000 EUR deposit and 1000 EUR removal. The imported transactions does not carry the sign anymore, of course, as all internal values must be positive in the model of Portfolio Performance.

If I understand your concerns, they are about negative values in the internal transactions model of Portfolio Performance. At the moment, I do not see this change allowing the negative values to leak in.

buchen avatar Oct 31 '23 07:10 buchen

date;type;amount
2023-10-31;Ein-/Auszahlung;2000
2023-11-01;Ein-/Auszahlung;-1000

Yes, this exemplifies a case we see here. I understand that it may change behavior for some users of CSV import, so it would depend on benefits it brings vs possible corner cases, and IMHO benefits outweigh. All in all, I posted it for feedback. I'll try to address review comments as time permits. (I otherwise have a whole bunch of changes in my fork, some adhoc, all without tests, but some certainly may be useful to other people, and to make PP a more generic tool (== cover more usecases), which should be a good aim for an open-source project.)

pfalcon avatar Oct 31 '23 07:10 pfalcon