Changes to support cost transfers

[Eric Altendorf]

I have revisited the work I did earlier to tweak Beancount to propagate cost basis with asset transfers.  (As has been discussed, this is a bit of a corner case in general, but is actually very common and important for capital gains calculations for cryptocurrency assets, since they are frequently transferred between accounts.)

Digging through my git repo, it looks like what I did was this:
https://github.com/beancount/beancount/compare/master...ericaltendorf:beancount:cost-transfer?expand=1

I would appreciate some feedback on whether this generally looks like the right approach, as well as any shortcomings that would need to be addressed before I can send a PR to merge it into Beancount proper.

Thanks,

Eric

(Is there a "beancount-dev" list that would be better to send this to? :)

[Martin Blais]

- There's no dev list, this is it, not enough traffic to justify a separate list.

- Your feature touches a very sensitive part of the booking process - one I've struggled to get sort of right in the past, has a lot of weird corner cases, it's been a bit of a whack-a-mole situation - I'd have to really immerse myself back into this to give it proper review, and I don't have time for a while. 

- I would suggest adding unit tests for the specific changes it improves would be very useful to that effect.

- In the meantime I'm happy to create for you a permanent branch in the beancount repo itself to track this, or perhaps even better, to install a conditional in the master branch with an option that dispatches between the two implementations.

- The cheap (time-boxed) and easy thing I could do could be to ensure before/after results match.

LMK what you'd like to do,

--
You received this message because you are subscribed to the Google Groups "Beancount" group.
To unsubscribe from this group and stop receiving emails from it, send an email to beancount+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/beancount/CAFXPr0u1vHW-5C32d_LvpgWXTd_quhsk-UGP8U%2BO8VU9syj3xw%40mail.gmail.com.

[Eric Altendorf]

Awesome, thanks.  This sounds good:

1) I'll add unit tests, try to clean up some of the questions and todos i had, and develop more certainty around the problem and solution

2) I'll share with you my findings and the unit tests, and you can try running it on your data to ensure things match

3) Then we can make a call on creating a branch or a conditional in the master branch.

fwiw i'm not too worried about divergence -- i created this at least 6 months ago, and just pulled from head with no merge conflicts, so i don't think this is very active part of the code base :)

To view this discussion on the web visit https://groups.google.com/d/msgid/beancount/CAK21%2BhMuNzXX61TJqRawP3Cq-jK8U1Weokra9rmd19PCMbYBkw%40mail.gmail.com.

[Eric Altendorf]

I've cleaned up and added some unit tests.  The preliminary PR is here:

https://github.com/beancount/beancount/pull/840

The main issue right now is that it only works for transfers with two equal and opposite augment/reduce postings (and worse, does not gracefully detect and reject more complex forms, such as a transfer from one account into two others, or from two others into one).  The case of split-transferring from one account into two others (e.g., account1: -100 HOOL, account2: 50 HOOL, account3: 50 HOOL) is especially problematic as there's no way to know which cost units should go to account2 and which to account3.

Preliminary feedback from anyone would be very much appreciated, specifically:

- this question of how to handle and/or reject more-complex-transfers

- cases to add more tests for

- anything about the code itself

On Sat, Apr 13, 2024 at 2:39 PM Eric Altendorf <erical...@gmail.com> wrote:

Awesome, thanks.  This sounds good:

1) I'll add unit tests, try to clean up some of the questions and todos i had, and develop more certainty around the problem and solution

2) I'll share with you my findings and the unit tests, and you can try running it on your data to ensure things match

3) Then we can make a call on creating a branch or a conditional in the master branch.

fwiw i'm not too worried about divergence -- i created this at least 6 months ago, and just pulled from head with no merge conflicts, so i don't think this is very active part of the code base :)

On Sat, Apr 13, 2024 at 6:57 AM Martin Blais <bl...@furius.ca> wrote:

- There's no dev list, this is it, not enough traffic to justify a separate list.

- Your feature touches a very sensitive part of the booking process - one I've struggled to get sort of right in the past, has a lot of weird corner cases, it's been a bit of a whack-a-mole situation - I'd have to really immerse myself back into this to give it proper review, and I don't have time for a while. 

The more I look at it, yeah...... I agree.  This feels like it is calling out for an implementation in a higher-level, more declarative language more amenable to static analysis and correctness proofs.  No idea what that language is, though.... :)

[Eric Altendorf]

BTW, in case anyone's looking at this, another minor issue with what I shared earlier is that it may mutate the order of some postings within a transaction during booking.  I have a different implementation now as well which preserves order and feels a bit less hacky at the expense of being more verbose and arguably complicated.  (Transitory snapshot right now here.)

The underlying issue is that to transfer cost info from reductions to augmentations (that is, the cost of an asset held in account one to be transferred to that asset being held in account two) I need to process all reductions first then process augmentations.  This requires two passes, or other funky operations.