Decode Liquidity additions & removal for Uniswap(V2 & V3) & Sushiswap #5337

prettyirrelevant · 2023-01-02T17:20:40Z

Checklist

Decode LP additions & removal for Uniswap V3
Decode LP additions & removal for Uniswap V2
Decode LP additions & removal for Sushiswap

codecov · 2023-01-06T18:56:21Z

Codecov Report

Merging #5337 (f9065a8) into develop (73b82aa) will decrease coverage by 0.39%.
The diff coverage is 70.23%.

@@             Coverage Diff             @@
##           develop    #5337      +/-   ##
===========================================
- Coverage    71.45%   71.07%   -0.39%     
===========================================
  Files          988     1005      +17     
  Lines        74172    74916     +744     
  Branches      9156     9238      +82     
===========================================
+ Hits         53001    53247     +246     
- Misses       19566    20027     +461     
- Partials      1605     1642      +37

Flag	Coverage Δ
frontend_integration	`60.22% <59.45%> (+0.14%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ents/accounts/management/types/XpubAccountForm.vue	`0.00% <0.00%> (ø)`
frontend/app/src/components/dialogs/BigDialog.vue	`100.00% <ø> (ø)`
...tend/app/src/components/display/AccountDisplay.vue	`6.25% <ø> (ø)`
frontend/app/src/components/helper/ListItem.vue	`90.47% <ø> (ø)`
.../components/settings/general/rpc/DotRpcSetting.vue	`0.00% <0.00%> (ø)`
.../components/settings/general/rpc/KsmRpcSetting.vue	`0.00% <0.00%> (ø)`
...onents/settings/general/rpc/OptimismRpcSetting.vue	`0.00% <0.00%> (ø)`
...d/app/src/components/status/update/AssetUpdate.vue	`41.33% <ø> (ø)`
frontend/app/src/pages/settings/general/index.vue	`100.00% <ø> (ø)`
frontend/app/src/premium/premium.ts	`34.56% <ø> (-1.07%)`	⬇️
... and 258 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

prettyirrelevant · 2023-01-06T19:04:13Z

rotkehlchen/chain/ethereum/modules/uniswap/v2/common.py

+                    to_notes=notes.format(amount, asset.symbol, counterparty, pool_address),
+                    to_counterparty=counterparty,
+                )
+                action_items.append(action_item)


This approach might be frowned upon because I'm directly mutating action_items.

However, this is quite unavoidable because the other decoding method that supports returning a list of ActionItem requires key-value pair of addresses to a decoder function.

The above won't work as we don't keep a store of all UNISWAP lp tokens. We store the ones users encounter, but this would mean that users have to manually re-decode transactions. Terrible UX.

Another option would be to create a new method in DecoderInterface for this use case.

I have been checking and I believe it would be better to allow returning events from here. The problem with mutating the action_items list here is that you introduce unexpected behavior in some of the decoders and this will surely be an issue in the future.

This decoding methods are executed here if I'm correct

rotki/rotkehlchen/chain/evm/decoding/decoder.py

Line 271 in 7b84e04

event = self.try_all_rules(

if you read that function the rules by address returns both events and new action items and then the list of action items is extended in that place. Is for that why I say that mutating here introduces unexpected behavior.

My idea would be that, return both event and new action items and extend in the same way. It shouldn't be a difficult change since we have few of these decoders. What do you think?

yabirgb

Thanks Isaac. Left you some comments and things to improve

rotkehlchen/chain/ethereum/constants.py

rotkehlchen/chain/ethereum/modules/uniswap/v2/common.py

yabirgb · 2023-01-08T11:21:56Z

rotkehlchen/chain/ethereum/modules/uniswap/v2/common.py

+                    to_notes=notes.format(amount, asset.symbol, counterparty, pool_address),
+                    to_counterparty=counterparty,
+                )
+                action_items.append(action_item)


I have been checking and I believe it would be better to allow returning events from here. The problem with mutating the action_items list here is that you introduce unexpected behavior in some of the decoders and this will surely be an issue in the future.

This decoding methods are executed here if I'm correct

rotki/rotkehlchen/chain/evm/decoding/decoder.py

Line 271 in 7b84e04

event = self.try_all_rules(

if you read that function the rules by address returns both events and new action items and then the list of action items is extended in that place. Is for that why I say that mutating here introduces unexpected behavior.

My idea would be that, return both event and new action items and extend in the same way. It shouldn't be a difficult change since we have few of these decoders. What do you think?

rotkehlchen/chain/ethereum/modules/uniswap/v2/common.py

rotkehlchen/tests/unit/decoders/test_uniswapv3.py

rotkehlchen/chain/ethereum/modules/uniswap/v3/decoder.py

rotkehlchen/chain/ethereum/modules/uniswap/v2/common.py

yabirgb

Just two small comments but can be addressed later

yabirgb · 2023-01-12T16:22:35Z

rotkehlchen/tests/unit/decoders/test_uniswapv3.py

+ADDY = '0xb63e0C506dDBa7b0dd106d1937d6D13BE2C62aE2'
+ADDY_2 = '0xeB312F4921aEbbE99faCaCFE92f22b942Cbd7599'
+ADDY_3 = '0xdD84Ce1aDcb3A4908Db61A1dFA3353C3974c5a2B'
+ADDY_4 = '0x354304234329A8d2425965C647e701A72d3438e5'
+ADDY_5 = '0xa931b486F661540c6D709aE6DfC8BcEF347ea437'


Those should have string_to_evm_address calls but since we are not checking types is fine

Still do it please @prettyirrelevant

rotkehlchen/chain/evm/decoding/decoder.py

LefterisJP

Left some comments

rotkehlchen/chain/ethereum/modules/sushiswap/decoder.py

LefterisJP · 2023-01-12T23:31:22Z

rotkehlchen/chain/ethereum/modules/uniswap/v2/accountant.py

+            get_tx_event_type_identifier(HistoryEventType.RECEIVE, HistoryEventSubType.RECEIVE_WRAPPED, CPT_UNISWAP_V2): TxEventSettings(  # noqa: E501
+                taxable=False,
+                count_entire_amount_spend=False,
+                count_cost_basis_pnl=False,


here and in all the others why do you count cost basis pnl as False? Doesn't this determine asset spends and counts how much of something you have at any point? Are you not making this for all uniswa/sushiswap pools deposit/withdrawals? Taxable or not the spends should be counted.

Please understand what these are and what they do. Same thing for the reviewer @yabirgb.
Don't just blindly copy paste shit around.

Sidenote: We need tests for these as stated here: #5260
ALL of these accountants should have tests, otherwise you guys are throwing shit at the wall and hope it sticks.

Sidenote2: Need to abstract all those better. We copy too much stuff around to create these accounting rules. Probably there is a better way ... but need to think about it.

Won't leave the same comment in the other accountants. Please understand it applies there too though

Quite frankly, in as much as the argument names are pretty descriptive, it's still quite difficult to get the hang of how to combine the arguments to suit different types of transactions.

This comment helped though, thanks.

rotkehlchen/chain/ethereum/modules/uniswap/v2/common.py

LefterisJP · 2023-01-13T00:30:33Z

rotkehlchen/chain/ethereum/modules/uniswap/v3/decoder.py

+        for token, amount in zip(liquidity_pool_position_info[2:4], (amount0_raw, amount1_raw)):
+            token_with_data: 'CryptoAsset' = get_or_create_evm_token(


I also think this needs refactoring here. I get that it looks and reads nice, but we should not sacrifice speed for readability to such an extent. I was gonna try and come up with a nice alternative but it's more tricky than what I can do at 01:30 at night.

So please think on this a bit and try to find a way to have this read as nicely as this does but also not iterate the events twice.

rotkehlchen/chain/evm/decoding/decoder.py

rotkehlchen/tests/api/test_uniswap.py

LefterisJP · 2023-01-13T00:34:53Z

rotkehlchen/tests/unit/decoders/test_uniswapv3.py

+ADDY = '0xb63e0C506dDBa7b0dd106d1937d6D13BE2C62aE2'
+ADDY_2 = '0xeB312F4921aEbbE99faCaCFE92f22b942Cbd7599'
+ADDY_3 = '0xdD84Ce1aDcb3A4908Db61A1dFA3353C3974c5a2B'
+ADDY_4 = '0x354304234329A8d2425965C647e701A72d3438e5'
+ADDY_5 = '0xa931b486F661540c6D709aE6DfC8BcEF347ea437'


Still do it please @prettyirrelevant

rotkehlchen/tests/unit/decoders/test_weth.py

LefterisJP

LGTM. Will merge though I left 2 smaller comments.

IT's too big to leave it out.

LefterisJP · 2023-01-20T12:08:04Z

rotkehlchen/tests/unit/decoders/test_sushiswap.py

+@pytest.mark.parametrize('ethereum_accounts', [[ADDY_2]])
+def test_sushiswap_v2_remove_liquidity(database, ethereum_inquirer, eth_transactions):
+    """
+    This checks that removing liquidity to Sushiswap V2 pool is decoded properly.


Suggested change

This checks that removing liquidity to Sushiswap V2 pool is decoded properly.

This checks that removing liquidity from a Sushiswap V2 pool is decoded properly.

gitpoap-bot · 2023-01-20T12:21:27Z

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2023 rotki Contributor:

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

prettyirrelevant force-pushed the decode-uni-sushi-deposit-and-withdrawals branch from 25be8e7 to 378d030 Compare January 6, 2023 18:42

prettyirrelevant marked this pull request as ready for review January 6, 2023 18:43

prettyirrelevant temporarily deployed to test January 6, 2023 18:43 — with GitHub Actions Inactive

prettyirrelevant temporarily deployed to test January 6, 2023 18:44 — with GitHub Actions Inactive

prettyirrelevant commented Jan 6, 2023

View reviewed changes

prettyirrelevant added the ready for peer review Backend PR ready to be reviewed by colleagues label Jan 6, 2023

prettyirrelevant temporarily deployed to test January 7, 2023 11:18 — with GitHub Actions Inactive

yabirgb reviewed Jan 8, 2023

View reviewed changes

prettyirrelevant force-pushed the decode-uni-sushi-deposit-and-withdrawals branch from 12e4596 to c3d1b03 Compare January 11, 2023 12:07

prettyirrelevant added PR review work work on PR review comments and removed ready for peer review Backend PR ready to be reviewed by colleagues labels Jan 11, 2023

prettyirrelevant force-pushed the decode-uni-sushi-deposit-and-withdrawals branch from c3d1b03 to 6cc167f Compare January 12, 2023 12:15

yabirgb reviewed Jan 12, 2023

View reviewed changes

LefterisJP reviewed Jan 13, 2023

View reviewed changes

prettyirrelevant added 5 commits January 19, 2023 12:55

feat: add decoder support for uniswap v3 liquidity addition & removal

bc439e2

feat: add decoder for uniswap v2 liquidity addition & removal

252a234

feat: add decoders for sushiswap liquidity addition and removal

ab0a13a

update accountant module for sushiswap & uniswap

49e9a1d

fix failing tests

8f635aa

prettyirrelevant force-pushed the decode-uni-sushi-deposit-and-withdrawals branch from 6cc167f to 045e821 Compare January 19, 2023 13:23

address pr comments & refactor decoding interface

f9065a8

prettyirrelevant force-pushed the decode-uni-sushi-deposit-and-withdrawals branch from 045e821 to f9065a8 Compare January 19, 2023 13:44

LefterisJP approved these changes Jan 20, 2023

View reviewed changes

LefterisJP merged commit 9b59165 into rotki:develop Jan 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decode Liquidity additions & removal for Uniswap(V2 & V3) & Sushiswap #5337

Decode Liquidity additions & removal for Uniswap(V2 & V3) & Sushiswap #5337

prettyirrelevant commented Jan 2, 2023 •

edited

codecov bot commented Jan 6, 2023 •

edited

prettyirrelevant Jan 6, 2023

yabirgb Jan 8, 2023

yabirgb left a comment

yabirgb Jan 8, 2023

yabirgb left a comment

yabirgb Jan 12, 2023

LefterisJP Jan 13, 2023

LefterisJP left a comment

LefterisJP Jan 12, 2023

prettyirrelevant Jan 19, 2023

LefterisJP Jan 13, 2023

LefterisJP Jan 13, 2023

LefterisJP left a comment

LefterisJP Jan 20, 2023

gitpoap-bot bot commented Jan 20, 2023

		for token, amount in zip(liquidity_pool_position_info[2:4], (amount0_raw, amount1_raw)):
		token_with_data: 'CryptoAsset' = get_or_create_evm_token(

	This checks that removing liquidity to Sushiswap V2 pool is decoded properly.
	This checks that removing liquidity from a Sushiswap V2 pool is decoded properly.

Decode Liquidity additions & removal for Uniswap(V2 & V3) & Sushiswap #5337

Decode Liquidity additions & removal for Uniswap(V2 & V3) & Sushiswap #5337

Conversation

prettyirrelevant commented Jan 2, 2023 • edited

Checklist

codecov bot commented Jan 6, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yabirgb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yabirgb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LefterisJP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LefterisJP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitpoap-bot bot commented Jan 20, 2023

prettyirrelevant commented Jan 2, 2023 •

edited

codecov bot commented Jan 6, 2023 •

edited