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

Calculate and apply the instrument's tax rate percentage to transactions #44

Merged
merged 6 commits into from
Feb 17, 2025

Conversation

marius-hi
Copy link
Contributor

I implemented this feature to automatically pre-fill the tax field with the calculated value based on the tax rate defined in the instrument.

How to use:
Each instrument can have a specified tax rate percentage. When adding or editing a trade, the tax value is automatically calculated based on the defined tax rate and the total amount. If the field is manually modified, the entered tax value is retained. To disable the automatic calculation, the field must be set to 0.

@matthiasstraka
Copy link
Owner

matthiasstraka commented Feb 15, 2025

Thanks for the PR, i do like the idea of the automatic tax rate. I'm not entirely sure if the tax rate is an instrument property or if is should be an account property (to my knowledge, you pay the same tax for all gains or you pay some mixed tax rate that is not automatically computable anyways). Edit: I realized, you would have a differnet tax rate for domestic and foreign stock (depending on tax treaties). Maybe we can leave the custom tax rate for the instrument and augment it by an pre account or per user tax rate later.

I did not try out your code yet, but will the automatic tax be applied always? It definitely makes sense for dividend payments, but opening a position is usually tax free and closing it (partially) does not allow you to directly compute the tax without knowing how gains are computed.
Instead of computing the tax in the background, I would prefer a button (or some other UI element) that computes the tax on request with the set tax rate. Maybe like this:
image
Then we do not have any backwards compatibility issues with old transactions and the user can decide if he wants to apply the auto-tax for every transaction.

So in summary:

  • move the tax rate to the account object/table We leave it in the instrument table, and maybe later add another tax rate for the account (like a default that gets applied when the instrument tax rate is NULL)
  • show a button next to the Tax field to apply the tax via javascript

Are you interested in changing it this way, otherwise I can take a look at it after I merge the PR.

@marius-hi
Copy link
Contributor Author

Thanks for your suggestions and for your work done on this project.

Tax rate is on the instrument
Indeed, this may not apply to every country, but in mine, tax rates vary depending on tax treaties. It would be a good idea to keep this and also include an input on a higher level, such as the account or user.

Is automatic tax be applied always
No, it only applies when the field tax from the trade form remains empty. If inserting 0 or any other value, the automatic tax won't apply. Also it doesn't apply when the instrument doesn't have defined yet a tax rate.

Opening a position is usually tax free
On our tax regime there is a tax on every transaction, including the opening ones. So probably has to be added a dropdown allowing to switch between (on opening, on closing or on both) or use 2 different tax inputs.

Computing the tax in the background
I thought of this as well, but doing it on the front-end requires additional steps like clicking on Compute button. More than that it won't work if volume and price has not been filled in. I understand that in the background might be a bit hidden, but that will apply only when there is a tax percentage set on instrument and the trade tax is not already filled in.

@matthiasstraka
Copy link
Owner

Oh, I see - you are talking about a tax that is automatically charged on execution (both buy and sell). This makes sense with the 0.12% rate that you added in the tests.
So far, tax is a collective field for all types of tax (mostly capital gains tax for me, but also used to track tax credit refunded for a loss trade).
Would it make sense to turn the taxRate field into an executionTaxRate and make sure that the automatically computed tax is always negative then? I assume that you pay tax on all transactions and not get any a refund - similar to commissions.

Another thing I noticed: in the database, I like to keep percentages between 0-1 value wise like in instrument_terms (i was too lazy to scale that for easier user input so far). Can you please change this - both in the Assert of the database field and in the computation. Feel free to show the user a scaled value (e.g. 0.12% is shown to/edited by the user as 0.12% but stored as 0.0012 in the database)

I think we can leave the automatic computation in the background for now - since it only affects executions with a set tax rate anyways. In the long-term I will definitely move this computation to the front-end. The main reason is that I want to see the complete transaction in one form (currently you only see the effect of a transaction on your cash account if you change the view). My idea is to have some active code on the edit form that takes volume/price, direction, commission and tax into account to show the overall sum. Doing automatic tax compuations (as long as not overwritten by the user) fits into this concept nicely. But I have too many other side-projects at the moment, so php-invest is on pause for me (development wise).

Use a scaled value instead percentage
Tax is always negative
@marius-hi
Copy link
Contributor Author

Your suggestions make sense, I've implemented these changes.

Copy link
Owner

@matthiasstraka matthiasstraka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me except for the small issue with initializing $total. Please fix this and once all checks are green, I can merge your PR. Thank for you for your contribution.

@matthiasstraka matthiasstraka merged commit 47829d1 into matthiasstraka:main Feb 17, 2025
1 check passed
@matthiasstraka
Copy link
Owner

Thank you again for the PR and all the fixes!

@marius-hi marius-hi deleted the calculate-tax-rate branch February 19, 2025 20:17
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