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

Incorrect FIFO order used when doing multiple transfers #4446

Open
metal450 opened this issue Jan 3, 2025 · 18 comments
Open

Incorrect FIFO order used when doing multiple transfers #4446

metal450 opened this issue Jan 3, 2025 · 18 comments
Labels

Comments

@metal450
Copy link
Contributor

metal450 commented Jan 3, 2025

I'm reporting this bug from the forum here on GH for visibility (apologies if it already exists somewhere in German - I did my best to search but didn't find it).

The issue: transferring an asset between accounts more than once doesn't respect the proper FIFO order. Repro:

  • Buy 1 unit on a crypto exchange (corresponding to one account in PP). Call it lot E1.
  • Buy 1 unit in a wallet (corresponding to another account in PP). Call it lot W.
  • Buy another on the same exchange. Call it lot E2.
  • Transfer everything from the exchange to the wallet (2 units).
  • Transfer 1 unit from the wallet back to the exchange. It should pull E1 as it’s oldest.
  • Sell 1 on the exchange. It should sell E1. However, it sold lot W.

This is quite impactful in a variety of scenarios - here's some that have come up on the forum:

  • In crypto, it's extremely common to move assets around between wallets & exchanges. The US now requires separate cost basis tracking per-wallet for tax purposes. This bug means that PP will ultimately sells the wrong lot & show the wrong cost basis, as illustrated above.
  • Because PP doesn't yet support custom sale orderings (i.e. HIFO / LIFO / specific lots / etc), a number of users have tried to use transfers to workaround the limitation - i.e. move shares out of an account, sell the desired lot, then move them back. This also causes the bug.
  • Users who transfer equities between custodians for custody transfer bonuses

8fdaa192ebb63f9e048f91bf47d84211643a9f3a
4bdce65eec3b1e02da96c600444a9f20e28cd42b
test portfolio.xml.txt

@metal450 metal450 added the bug label Jan 3, 2025
@metal450
Copy link
Contributor Author

metal450 commented Jan 3, 2025

Wow what the heck, I've never seen crypto scammers even in github comments....

@Nirus2000
Copy link
Member

Nirus2000 commented Jan 3, 2025

I've remove the Scam/Phishing comments.. Thx 👍🏻
Interesting that it only affects your issue. Please check your PC and change your access data/passwords to be on the safe side.

@metal450
Copy link
Contributor Author

metal450 commented Jan 3, 2025

Likely bots just picked up on the crypto-related keywords in the post

@metal450
Copy link
Contributor Author

metal450 commented Jan 4, 2025

A couple questions hopefully for an in-the-know developer (i.e. @Nirus2000 perhaps? Or even @buchen? ;) )

  • I’d like to understand a little more concretely when this bug can be expected to arise (vs when can I be confident that it will respect the FIFO order). I think the answer is: Anytime you transfer shares more than 1 time, the second transfer may take shares in the wrong order. So i.e. Buy AAA in account 1 → Transfer AAA to account 2 (correct order) → Now if you transfer less than all of AAA to any other account (whether a 3rd account or back to the original), that transfer will not pull the shares from account 2 in FIFO order. Is that accurate?

  • It would also be useful to understand how it actually chooses the lots (i.e. it’s not in FIFO order, what is the order used?)

  • As far as implementing a fix, any technical details you could provide would be useful. There are some details, but they're all in German & I wasn't really able to make sense of them via Google Translate due to the differences in terminology.

I'm pretty urgently motivated to try to figure something out (US taxes are due in a few months, & as I really rely on PP for tracking & reporting my crypto cost basis, this bug is unfortunately wreaking some havoc on my tax accounting)

Thanks in advance

@metal450
Copy link
Contributor Author

metal450 commented Jan 4, 2025

Ok I spent the past hour trying examples, here's the best I could come up with:

  • Buy shares in Exchange E
  • Transfer to Wallet W (FIFO order will be correct for this 1st transfer)
  • HOWEVER, if there were any other shares already in W when you transferred these new shares, and:
    • If you do a partial outbound transfer from W, it will take the wrong shares. This is the case whether you transfer back to original Exchange E or to a 3rd account. It seems to transfer the shares that were in W earlier, rather than shares that were originally bought earlier.
    • If you transfer all shares out of W to an empty 3rd account, then do a partial transfer to an empty 4th account, then sell, the order will still be wrong. So whether you do a full or partial transfer out of W, the bug carries forward.
    • If you transfer all shares out of W to an empty 3rd account, then sell some of those shares, the order of the sale will be correct. This is really weird & breaks my understanding of the issue, but I'm sure it's the case.
    • If you sell directly from W without transferring again, the order will be correct.

So to summarize, it seems like if you transfer shares into an account that had other shares there beforehand, and you transfer out of that account ever again (whether full or partial), sales/transfers after that may or may not be in the wrong order.

Is that correct?

@Nirus2000
Copy link
Member

We need an example file... with transactions.

Not with "a" and "b" or 1 and 2... and in and out.
Show us on a real example... and what calculate is wrong or right.

@metal450
Copy link
Contributor Author

metal450 commented Jan 4, 2025

The example file corresponding to the screenshots is above in the original post (just below the screenshots) - was that one no good (or just unnoticed)? It doesn't show every possible case, but it definitely shows the bug....?

Thanks for the reply :)

@metal450
Copy link
Contributor Author

metal450 commented Jan 4, 2025

To be a little more clear/explicit: in the sample file posted above, the sale at the end sold the lot that was purchased for $2.50, but the expectation was that it sell the one purchased for $1

@Nirus2000
Copy link
Member

Nirus2000 commented Jan 4, 2025

You Transfer out from a to b and on the same date from b to a?
All on the same day at the same time, of course... what do you expect?
How realistic is that...

I don't quite understand it either... but can you even explain the problem in one comment and not in an essay of 4 comments?
Please don't take this the wrong way, but then mixing in other "problems" confuses the whole thing even more.

@metal450
Copy link
Contributor Author

metal450 commented Jan 4, 2025

You Transfer out from a to b and on the same date from b to a? All on the same day at the same time, of course... what do you expect?

The bug is not related to the dates. Here, I edited it to have 1 month between every action. Behavior is identical.

test.portfolio.xml.txt

can you even explain the problem in one comment and not in an essay of 5 comments?

The bullets in the very first post concisely explain each transaction in the sample file. 6 transactions, 6 bullets.

Also, my previous comment was a one-liner: "the sale at the end sold the lot that was purchased for $2.50, but the expectation was that it sell the one purchased for $1"

mixing in other "problems" confuses the whole thing even more.

It's not other problems. All other follow-ups are just my attempts at trying to understand when the bug occurs & when it doesn't. Feel free to pick one example & ignore others if more information isn't helpful.

@Nirus2000
Copy link
Member

Nirus2000 commented Jan 4, 2025

And what ist the issue and witch numbers are wrong?

@metal450
Copy link
Contributor Author

metal450 commented Jan 4, 2025

And what ist the issue and witch numbers are wrong?

"the sale at the end sold the lot that was purchased for $2.50, but the expectation was that it sell the one purchased for $1"

Sorry & respectfully, if I'm being unclear I really don't see where...? 🙏

@metal450
Copy link
Contributor Author

metal450 commented Jan 4, 2025

Link to that thread is in the 1st sentence of my original post. I'm not sure what you're trying to communicate with that link, but there are also 2 replies to that specific post from Rafa with further details (from Avendo & LOLinger78).

I mentioned a bunch of stuff from that thread in my comments above.

@pfalcon
Copy link
Contributor

pfalcon commented Jan 13, 2025

@metal450 : Did you consider making a (Java) testcase checking for the expected behavior?

@metal450
Copy link
Contributor Author

metal450 commented Jan 13, 2025

Hi - thanks for chiming in. I'm not exactly sure what you mean, but:

  • As far as a testcase, there's a portfolio file above to show the issue & a description of the expected behavior?
  • In terms of code tho, I'm sadly not a Java developer - I have attempted to make sense of PP's source a few times in the past, but haven't really had much luck due to general unfamiliarity with the language & tooling, plus PP's almost total lack of code comments :(

That being said, having just seen your related #4461 & #4459 threads, I'm incredibly heartened that someone is at least looking into this related code :) With the recent US crypto tax law changes that make FIFO mandatory, have been feeling in a really tough spot & afraid I'd have to figure out how to migrate my decades' worth of history to some other software where FIFO would follow the law. That this is even being looked into gives me a glimmer of hope :)

@pfalcon
Copy link
Contributor

pfalcon commented Jan 18, 2025

@metal450 :

In terms of code tho

Yes, I meant formalizing your test XML and expected behavior as a coded Java test, which could be run and produce pass/fail result automatically. Writing tests is a pretty boring part of programming, especially open-source, but at the same time, needed to maintain quality, so the only solution is apparently to "crowdsource" them ;-).

That being said, having just seen your related #4461 & #4459 threads, I'm incredibly heartened that someone is at least looking into this related code :)

Yes, I'm trying to add short trade support to PP, and in my initial hacking I simply ignored any transfer transactions, while in the background thinking how they could be (re)implemented. This ticket brought some things I didn't even think about, so now I'm thinking how to put it all together...

@metal450
Copy link
Contributor Author

I meant formalizing your test XML and expected behavior as a coded Java test, which could be run and produce pass/fail result automatically.

Ahhh I see, you meant write a unit test. Yeah great idea - my first thought was that it might not be worth having tests that would always fail given the current code if nobody was actually working on this issue, but if you're already progressing toward a solution & need it to be validated, maybe worth trying to figure it out? (I did have immense difficulty even just trying to get Eclipse working when I tried a few years ago unfortunately, haha)

now I'm thinking how to put it all together

Amazing. Ideally/hopefully as you reimagine transfers/sales, the solution might even be generalized enough that it could support other ordering too (i.e. I know you already saw the my post about Specific Lots - there are forum threads going back at least a few years of people trying to get around that limitation by transferring shares back & forth between temporary accounts).

Of course, working FIFO would be immensely valuable by itself. Just throwing that out there as a possible consideration as you're digging through all the ordering code anyway :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants