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

Make liquity decoder work with DS Proxies #5791

Merged
merged 2 commits into from Mar 30, 2023

Conversation

nebolax
Copy link
Contributor

@nebolax nebolax commented Mar 20, 2023

Closes #5761

Checklist

  • The PR modified the frontend, and updated the user guide to reflect the changes.

DONT FORGET TO MERGE THE CASSETTES BRANCH

@codecov
Copy link

codecov bot commented Mar 20, 2023

Codecov Report

Merging #5791 (303fd81) into develop (2e41374) will decrease coverage by 7.50%.
The diff coverage is 62.93%.

❗ Current head 303fd81 differs from pull request most recent head def7723. Consider uploading reports for the commit def7723 to get more accurate results

@@             Coverage Diff             @@
##           develop    #5791      +/-   ##
===========================================
- Coverage    70.93%   63.44%   -7.50%     
===========================================
  Files         1146     1146              
  Lines        80637    80677      +40     
  Branches     10605    10609       +4     
===========================================
- Hits         57201    51185    -6016     
- Misses       21776    28018    +6242     
+ Partials      1660     1474     -186     
Flag Coverage Δ
backend 65.08% <62.93%> (-14.06%) ⬇️
frontend_integration 60.38% <ø> (+0.05%) ⬆️

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

Impacted Files Coverage Δ
rotkehlchen/api/rest.py 27.76% <ø> (-53.46%) ⬇️
rotkehlchen/api/server.py 80.76% <ø> (-9.62%) ⬇️
rotkehlchen/api/v1/resources.py 75.14% <ø> (-20.94%) ⬇️
rotkehlchen/chain/aggregator.py 55.20% <ø> (-21.59%) ⬇️
...otkehlchen/chain/ethereum/modules/curve/decoder.py 87.90% <ø> (-3.81%) ⬇️
rotkehlchen/chain/ethereum/modules/eth2/decoder.py 94.44% <ø> (ø)
rotkehlchen/chain/evm/decoding/decoder.py 81.95% <ø> (-0.89%) ⬇️
rotkehlchen/db/dbhandler.py 62.14% <ø> (-22.57%) ⬇️
rotkehlchen/db/history_events.py 66.16% <ø> (-17.18%) ⬇️
rotkehlchen/db/minimized_schema.py 100.00% <ø> (ø)
... and 12 more

... and 158 files with indirect coverage changes

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

Copy link
Contributor

@prettyirrelevant prettyirrelevant left a comment

Choose a reason for hiding this comment

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

a comment about the implementation & a test failed.
test_ds_proxy_liquity_deposit[ethereum_accounts0]

Copy link
Member

@yabirgb yabirgb left a comment

Choose a reason for hiding this comment

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

This is missing also the changelog entry

rotkehlchen/chain/ethereum/modules/liquity/decoder.py Outdated Show resolved Hide resolved
rotkehlchen/chain/ethereum/modules/liquity/decoder.py Outdated Show resolved Hide resolved
rotkehlchen/chain/ethereum/modules/liquity/decoder.py Outdated Show resolved Hide resolved
Copy link
Member

@yabirgb yabirgb left a comment

Choose a reason for hiding this comment

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

A couple of questions

rotkehlchen/chain/ethereum/modules/liquity/decoder.py Outdated Show resolved Hide resolved
rotkehlchen/chain/evm/decoding/base.py Outdated Show resolved Hide resolved
Comment on lines 181 to 182
proxy_owner = self.base.get_proxy_owner(user)
if proxy_owner is not None:
user = proxy_owner
Copy link
Member

Choose a reason for hiding this comment

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

Why this change? Isn't the user in the log event the ds proxy instead of the ds owner?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't the user in the log event the ds proxy instead of the ds owner?

Yes, this is true. However in location_label is the ds owner, so we have to reassign user to the owner of the proxy.

@yabirgb yabirgb added the ready for final review Backend PR ready to be reviewed by great Lefteris label Mar 29, 2023
docs/changelog.rst Outdated Show resolved Hide resolved
rotkehlchen/chain/evm/decoding/base.py Outdated Show resolved Hide resolved
rotkehlchen/chain/evm/decoding/base.py Outdated Show resolved Hide resolved
rotkehlchen/chain/ethereum/modules/liquity/decoder.py Outdated Show resolved Hide resolved
rotkehlchen/chain/ethereum/modules/liquity/decoder.py Outdated Show resolved Hide resolved
rotkehlchen/tests/unit/decoders/test_liquity.py Outdated Show resolved Hide resolved
rotkehlchen/tests/unit/decoders/test_liquity.py Outdated Show resolved Hide resolved
rotkehlchen/tests/unit/decoders/test_liquity.py Outdated Show resolved Hide resolved
rotkehlchen/tests/unit/decoders/test_liquity.py Outdated Show resolved Hide resolved
rotkehlchen/tests/unit/decoders/test_liquity.py Outdated Show resolved Hide resolved
Copy link
Member

@LefterisJP LefterisJP left a comment

Choose a reason for hiding this comment

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

So ehm .. only comment is a leftover from the previous PR not done correctly,.

@@ -70,6 +70,7 @@ def globaldb_get_general_cache_keys_and_values_like(
"""
Function to read globaldb cache.
Returns all pairs key-value where key starts with the provided `key_parts`.

`key_parts` should not contain neither `%` nor `.` symbols.
Copy link
Member

Choose a reason for hiding this comment

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

That's not what I had asked. Look at the review carefully.

#5807 (comment)

key_parts should contain neither the "%" nor the "." symbol.

What you have writen there is double negation. Both here and in the other function please.

Copy link
Member

@LefterisJP LefterisJP left a comment

Choose a reason for hiding this comment

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

lgtm

@LefterisJP LefterisJP merged commit ee13eea into rotki:develop Mar 30, 2023
9 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for final review Backend PR ready to be reviewed by great Lefteris
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decode Liquity events happening trough a DSProxy
4 participants