-
-
Notifications
You must be signed in to change notification settings - Fork 502
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
Fix for a problem with makerdao ETH vaults recognition of withdraw events #7873
base: bugfixes
Are you sure you want to change the base?
Fix for a problem with makerdao ETH vaults recognition of withdraw events #7873
Conversation
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Hey @gianluca-pub-dev thanks for raising this PR for this fix! Could you rebase it to |
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 |
9492277
to
b72d278
Compare
b72d278
to
14f8e4d
Compare
…events Event history was not reporting withdraw collateral events because ETH vaults were mapped to native ETH tokens instead than WETH
095c34a
to
0d0e7b8
Compare
…e code and added as constant
0d0e7b8
to
ab7f28b
Compare
location_label='0xca482bCd75A6E0697aD6A1732aa187310b8372Df', | ||
notes='Withdraw 0.022255814 ETH from ETH-A MakerDAO vault', | ||
counterparty=CPT_VAULT, | ||
address=string_to_evm_address('0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2'), |
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.
@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.
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.
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.
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.
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.
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 |
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 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
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.
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.
@@ -13,15 +18,19 @@ | |||
|
|||
|
|||
@pytest.mark.vcr() | |||
@pytest.mark.parametrize('ethereum_accounts', [['0x648aA14e4424e0825A5cE739C8C68610e143FB79']]) | |||
# Analyze EOA and user's DSProxy address |
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.
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.
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.
- That currently doesn't work
"""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 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.
there may be errors in this test, but manually tracking the DSProxy is not a solution
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.
@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
…eterministic decoding. This time not coming from a bug
32b528e
to
1cc4edf
Compare
Event history was not reporting withdraw collateral events because ETH vaults were mapped to native ETH tokens instead than WETH