-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add accounting rules for new wrapped event subtypes #161
Conversation
389103c
to
dcc7bda
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this file has a deletions
key. I guess we should delete all the combinations that aren't relevant anymore for yearm, aave, compound, thegraph and so on
"event_subtype": "deposit for wrapped", | ||
"counterparty": "aave-v3", | ||
"taxable": false, | ||
"count_entire_amount_spend": false, | ||
"count_cost_basis_pnl": true, | ||
"accounting_treatment": "SWAP" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aave v2 is missing here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've skipped aave-v2 on purpose here. If you notice the aave-v2 deposit rule is being modified here: 73c7160 to not be treated as a SWAP, since aave v2 has its own accountant logic.
This actually makes it identical to the general (null counterparty) deposit/deposit for wrapped
rule - so we shouldn't need any special rule for aave-v2.
Now I'm also realizing that the "edit" to the deposit asset
rule for aave-v2 added in the PR I linked is no longer actually needed since its now deposit for wrapped
and will be handled correctly by the general deposit for wrapped
rule as explained above, so I'm removing that.
"event_type": "deposit", | ||
"event_subtype": "deposit for wrapped", | ||
"counterparty": "Locked GNO", | ||
"taxable": false, | ||
"count_entire_amount_spend": false, | ||
"count_cost_basis_pnl": true, | ||
"accounting_treatment": "SWAP" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems that we have a rule also for remove asset... since you get a wrapped I guess you also need the redeem wrapped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the existing rule for Locked GNO withdraw/remove asset
is identical to the general withdraw/remove asset
rule. I'm not sure why this counterparty specific rule was added when it doesn't specify anything different than what the general rule does. So I don't think we need anything special for Locked GNO
with the new subtype. Also the old Locked GNO
withdraw/remove asset
rule should probably also be removed in the db upgrade you mentioned. Or is there something I'm not considering here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, then we need to remove it if it is identical to the general rule
dcc7bda
to
59dd67b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
No description provided.