Fix for a problem with makerdao ETH vaults recognition of withdraw events #7873

gianluca-pub-dev · 2024-05-01T22:25:05Z

Event history was not reporting withdraw collateral events because ETH vaults were mapped to native ETH tokens instead than WETH

codecov · 2024-05-01T22:49:24Z

Codecov Report

Attention: Patch coverage is 88.88889% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 80.28%. Comparing base (82342a4) to head (ab7f28b).
Report is 30 commits behind head on bugfixes.

Files	Patch %	Lines
...ehlchen/chain/ethereum/modules/makerdao/decoder.py	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           bugfixes    #7873      +/-   ##
============================================
+ Coverage     80.26%   80.28%   +0.02%     
============================================
  Files          1098     1098              
  Lines        106799   106772      -27     
  Branches      13360    13358       -2     
============================================
+ Hits          85717    85721       +4     
+ Misses        18995    18964      -31     
  Partials       2087     2087

Flag	Coverage Δ
backend	`80.58% <88.88%> (+0.04%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

OjusWiZard · 2024-05-02T14:39:41Z

Hey @gianluca-pub-dev thanks for raising this PR for this fix! Could you rebase it to bugfixes branch because we'll put this in the next bugfix release, also please fix the tests that broke in the CI.

gianluca-pub-dev · 2024-05-02T19:24:49Z

Hey @gianluca-pub-dev thanks for raising this PR for this fix! Could you rebase it to bugfixes branch because we'll put this in the next bugfix release, also please fix the tests that broke in the CI.

Hello @OjusWiZard , first of all, thanks for the review. For rebase I can do, and I will shortly, instead for updating the test cases I need to find the time. Maybe during the weekend but I am not sure, because it needs time to me because I should analyze those test cases to see how they were written and the related transactions. Let's see, but if it is urgent for your project, again I would ask to take over.

CC: @LefterisJP

Thanks

…events Event history was not reporting withdraw collateral events because ETH vaults were mapped to native ETH tokens instead than WETH

…e code and added as constant

gianluca-pub-dev · 2024-05-05T02:07:33Z

rotkehlchen/tests/unit/decoders/test_makerdao_sai.py

- location_label='0xca482bCd75A6E0697aD6A1732aa187310b8372Df',
- notes='Withdraw 0.022255814 ETH from ETH-A MakerDAO vault',
- counterparty=CPT_VAULT,
- address=string_to_evm_address('0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2'),


@LefterisJP , you can see from this, how the processing of the event was bugged. This "0xC02aaA...." is the WETH contract address, and here the event was detected as one "withdraw from ETH-A MakerDAO vault" operation. Also notice the sequence_index number, 0. Basically the decoding here was working "by chance". I think the general decoding could be re-worked for being more deterministic (and reproducible). I have some idea in mind if you are interested on discussing possible sw requirements and architecture.

The address field is manually added per decoder and means different things depending on the decoder. Coce can have bugs.

Always open to suggestions, but only things that make sense but for things that make sense you would need a perfect understanding of how the current thing we have works. And that should be in discord, not here.

LefterisJP

So @gianluca-pub-dev we have more pressing issues we work on so we have no time to devote to this at the moment.

For when someone has time to look at it, please give us transaction hashes and what you expect vs what you get.

The issue you get with ds proxies should not be solved by tracking the DSproxy manually. That's not a solution. Also replacing ETH with WETH from a first look does not look correct to me. Botjh can be used. Your change would break for transactions that use pure ETH and get it auto wrapped to WETH.

LefterisJP · 2024-05-05T07:30:07Z

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

- string_to_evm_address('0x24e459F61cEAa7b1cE70Dbaea938940A7c5aD46e'): (self.decode_makerdao_vault_action, A_AAVE.resolve_to_crypto_asset(), 'AAVE-A'), # noqa: E501
- string_to_evm_address('0x885f16e177d45fC9e7C87e1DA9fd47A9cfcE8E13'): (self.decode_makerdao_vault_action, A_ETH_MATIC.resolve_to_crypto_asset(), 'MATIC-A'), # noqa: E501
+ string_to_evm_address('0x3D0B1912B66114d4096F48A8CEe3A56C231772cA'): (self.decode_makerdao_vault_action, string_to_evm_address('0x3D0B1912B66114d4096F48A8CEe3A56C231772cA'), A_BAT.resolve_to_crypto_asset(), 'BAT-A'), # noqa: E501
+ string_to_evm_address(MAKERDAO_GEM_JOIN_ETHA_ADDRESS): (self.decode_makerdao_vault_action, string_to_evm_address(MAKERDAO_GEM_JOIN_ETHA_ADDRESS), A_WETH.resolve_to_crypto_asset(), 'ETH-A'), # noqa: E501


I am not sure if simply replacing ETH with WETH works here. It may work for some cases and fail for others.

I think the solution is to work on the decode_makerdao_vault_action to treat ETH and WETH the same

Without it, events are not correctly decoded when transactions are complex. In this way instead the events are "deterministic" and they can be compared with the logs on blockchain for reviewing. I will push another PR on top of this branch for a fix on the WETH module, just for discussing. Because I prefer to show what I mean using code.

LefterisJP · 2024-05-05T07:31:56Z

rotkehlchen/tests/unit/decoders/test_makerdao.py

@@ -13,15 +18,19 @@


 @pytest.mark.vcr()
-@pytest.mark.parametrize('ethereum_accounts', [['0x648aA14e4424e0825A5cE739C8C68610e143FB79']])
+# Analyze EOA and user's DSProxy address


no this is not the way.

We don't put comments here, but only in docstrings of test

We never manually track DSProxies. This is not the solution. The solution is to make it get detected by rotki. Make sure the code that detects the proxies runs.

That currently doesn't work

rotkehlchen/tests/unit/decoders/test_makerdao_sai.py

LefterisJP · 2024-05-05T07:34:40Z

rotkehlchen/tests/unit/decoders/test_makerdao_sai.py

 """Check that a Sai CDP migration is decoded properly"""
 tx_hex = deserialize_evm_tx_hash('0x03620c6bf5edb7a7935953337ffcfac70d631cf2012d6c80d36828d636063318 ') # noqa: E501
 evmhash = deserialize_evm_tx_hash(tx_hex)
- user_address = ethereum_accounts[0]
+ # TODO: ROTKI TEAM: The address you used here is the DSProxy address, not the user address.


there may be errors in this test, but manually tracking the DSProxy is not a solution

@LefterisJP , yes, but this is what you had already in this test case, so this is the proof that automatic recognition of DSProxy addresses is not working as you intended

…ress

…eterministic decoding. This time not coming from a bug

gianluca-pub-dev force-pushed the bugfix/makerdao_eth_vaults_asset_type branch from 9492277 to b72d278 Compare May 2, 2024 20:25

gianluca-pub-dev changed the base branch from develop to bugfixes May 2, 2024 22:47

gianluca-pub-dev force-pushed the bugfix/makerdao_eth_vaults_asset_type branch from b72d278 to 14f8e4d Compare May 2, 2024 22:50

Fix for a problem with makerdao ETH vaults recognition of withdrawal …

6efd08b

…events Event history was not reporting withdraw collateral events because ETH vaults were mapped to native ETH tokens instead than WETH

gianluca-pub-dev force-pushed the bugfix/makerdao_eth_vaults_asset_type branch 2 times, most recently from 095c34a to 0d0e7b8 Compare May 5, 2024 00:46

Makerdao module partial clean-up: removed some "magic" string from th…

77582a7

…e code and added as constant

gianluca-pub-dev force-pushed the bugfix/makerdao_eth_vaults_asset_type branch from 0d0e7b8 to ab7f28b Compare May 5, 2024 00:52

gianluca-pub-dev commented May 5, 2024

View reviewed changes

LefterisJP reviewed May 5, 2024

View reviewed changes

gianluca-pub-dev added 3 commits May 7, 2024 21:10

MakerDao tests: correct decoding requires to consider the DSProxy add…

5ec1821

…ress

maxerdao_sai: fixed test_makerdao_sai_cdp_migration test case

794564a

Makerdao module: additional possible improvements for hardening and d…

1cc4edf

…eterministic decoding. This time not coming from a bug

gianluca-pub-dev force-pushed the bugfix/makerdao_eth_vaults_asset_type branch from 32b528e to 1cc4edf Compare May 7, 2024 19:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for a problem with makerdao ETH vaults recognition of withdraw events #7873

Fix for a problem with makerdao ETH vaults recognition of withdraw events #7873

gianluca-pub-dev commented May 1, 2024

codecov bot commented May 1, 2024 •

edited

OjusWiZard commented May 2, 2024

gianluca-pub-dev commented May 2, 2024 •

edited

gianluca-pub-dev May 5, 2024 •

edited

LefterisJP May 5, 2024

LefterisJP left a comment

LefterisJP May 5, 2024

gianluca-pub-dev May 5, 2024

LefterisJP May 5, 2024

gianluca-pub-dev May 5, 2024

LefterisJP May 5, 2024

gianluca-pub-dev May 5, 2024 •

edited

Fix for a problem with makerdao ETH vaults recognition of withdraw events #7873

Are you sure you want to change the base?

Fix for a problem with makerdao ETH vaults recognition of withdraw events #7873

Conversation

gianluca-pub-dev commented May 1, 2024

codecov bot commented May 1, 2024 • edited

Codecov Report

OjusWiZard commented May 2, 2024

gianluca-pub-dev commented May 2, 2024 • edited

gianluca-pub-dev May 5, 2024 • edited

Choose a reason for hiding this comment

LefterisJP May 5, 2024

Choose a reason for hiding this comment

LefterisJP left a comment

Choose a reason for hiding this comment

LefterisJP May 5, 2024

Choose a reason for hiding this comment

gianluca-pub-dev May 5, 2024

Choose a reason for hiding this comment

LefterisJP May 5, 2024

Choose a reason for hiding this comment

gianluca-pub-dev May 5, 2024

Choose a reason for hiding this comment

LefterisJP May 5, 2024

Choose a reason for hiding this comment

gianluca-pub-dev May 5, 2024 • edited

Choose a reason for hiding this comment

codecov bot commented May 1, 2024 •

edited

gianluca-pub-dev commented May 2, 2024 •

edited

gianluca-pub-dev May 5, 2024 •

edited

gianluca-pub-dev May 5, 2024 •

edited