feat:add the collateral ratio of the Liquity protocol in troves.py #6578

3scava1i3r · 2023-09-07T05:25:00Z

Checklist

The PR modified the backend for getting collateral ratio of liquity protocol and changes the way get_position() data is retrieved

CLAassistant · 2023-09-07T05:25:05Z

All committers have signed the CLA.

yabirgb

You forgot to update the docs/api.rst file that contains information about the response of the endpoint that you modified, sadly we don't have autogenerated docs

rotkehlchen/chain/ethereum/modules/liquity/trove.py

codecov · 2023-09-07T10:11:27Z

Codecov Report

Merging #6578 (6b1dedd) into develop (d03900f) will decrease coverage by 0.20%.
Report is 9 commits behind head on develop.
The diff coverage is 50.00%.

@@             Coverage Diff             @@
##           develop    #6578       /-   ##
===========================================
- Coverage    71.44%   71.25%   -0.20%     
===========================================
  Files         1127     1130        3     
  Lines        99948   100063      115     
  Branches     11630    11640       10     
===========================================
- Hits         71412    71298     -114     
- Misses       26906    27124      218     
- Partials      1630     1641       11

Flag	Coverage Δ
backend	`63.73% <50.00%> (-0.42%)`	⬇️

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

Files Changed	Coverage Δ
rotkehlchen/chain/aggregator.py	`73.52% <0.00%> (-0.44%)`	⬇️
...otkehlchen/chain/ethereum/modules/liquity/trove.py	`59.62% <52.63%> (-18.46%)`	⬇️

... and 52 files with indirect coverage changes

3scava1i3r · 2023-09-09T04:26:29Z

You forgot to update the docs/api.rst file that contains information about the response of the endpoint that you modified, sadly we don't have autogenerated docs

added the same

…to feat/liquity

yabirgb

Thank you very much and sorry for taking so long to look at this. Just a few minor comments. Was also just checking yesterday on the frontend team in case there was an issue for the api design and if it would make sense to show the collateral with the troves, they said that is a good place for it and then the api design is okey.

If you notice there is a test that failed to execute. It has some special mocking that we were doing before introducing VCR. The reason why it fails is because there is a remote call to the contract (for the two values that you request) that are not found there. Maybe it would make sense to remove this and migrate to VCR as we do for other tests, the setup is a bit more complicated since you need to create a PR in other repo with the cassette. THis would help you https://rotki.readthedocs.io/en/stable/contribute.html#mocking-networking-in-the-tests

Please if you find any issue with this ping me in discord and I'll check with to answer asap. Also sorry for the delay in the review again.

yabirgb · 2023-09-12T07:51:34Z

rotkehlchen/chain/ethereum/modules/liquity/trove.py

@@ -52,6  52,11 @@ def serialize(self) -> dict[str, Any]:
        return result


+class GetPositionsResult(TypedDict):
+    balances: dict[ChecksumEvmAddress, Trove]
+    total_collateral_ratio: Optional[float]


we use FVal for flating point values. Is a special class that prevents us from decimal point errors

Suggested change

total_collateral_ratio: Optional[float]

total_collateral_ratio: Optional[FVal]

yabirgb · 2023-09-12T07:53:40Z

rotkehlchen/chain/ethereum/modules/liquity/trove.py

+    def _calculate_total_collateral_ratio(
+            self, eth_price: Price, lusd_price: Price,
+    ) -> Optional[float]:


Suggested change

def _calculate_total_collateral_ratio(

self, eth_price: Price, lusd_price: Price,

) -> Optional[float]:

def _calculate_total_collateral_ratio(

self, eth_price: Price, lusd_price: Price,

) -> Optional[FVal]:

Also can you add a small docstring explaining what this function does? it helps for people reding in the future and also people not familiar with liquity

Just as a note in the docstrings we include possible errors that can be raised by functions

yabirgb · 2023-09-12T07:55:48Z

rotkehlchen/chain/ethereum/modules/liquity/trove.py

@@ -102,13  101,25 @@ def get_positions(
                arguments=[],
            )



Please remove this empty line

yabirgb · 2023-09-12T07:56:58Z

rotkehlchen/chain/ethereum/modules/liquity/trove.py

+        if system_debt != 0:
+            total_collateral_ratio = eth_price * system_collateral / system_debt * lusd_price
+        else:
+            total_collateral_ratio = None


as it is defined now if the call for system_collateral raises RemoteError it will happen that system_debt won't be defined and this will raises an unknown variable error. There should be a return None on the except RemoteError

yabirgb · 2023-09-12T07:58:09Z

rotkehlchen/chain/ethereum/modules/liquity/trove.py

+        final_result = GetPositionsResult(
+            balances=data,
+            total_collateral_ratio=total_collateral_ratio,
+        )


I was thinking that maybe is clear to do

Suggested change

final_result = GetPositionsResult(

balances=data,

total_collateral_ratio=total_collateral_ratio,

)

final_result = GetPositionsResult(

balances=data,

total_collateral_ratio=self._calculate_total_collateral_ratio(eth_price, lusd_price),

)

what do you think? and move the price queries just before they are needed

yabirgb · 2023-09-12T08:09:13Z

rotkehlchen/chain/ethereum/modules/liquity/trove.py

+            log.error(f'Failed to query liquity contracts for protocol collateral ratio: {e}')
+
+        if system_debt != 0:
+            total_collateral_ratio = eth_price * system_collateral / system_debt * lusd_price


When doing operations with decimal numbers we use floats. Irc call only returns primitive types (can you confirm this). What we do in the code is transform this in FVal to always work with instances of FVal

Suggested change

total_collateral_ratio = eth_price * system_collateral / system_debt * lusd_price

total_collateral_ratio = eth_price * FVal(system_collateral) / FVal(system_debt) * lusd_price

yabirgb · 2023-09-12T08:20:53Z

docs/api.rst

+                  },
+            "total_collateral_ratio": "0.003138414276398289779741030787"


Please can you add the two keys total_collateral_ratio and balances to the under result explaining what they are. You can check other endpoints to see how we do it.

Also can you use a more realistic value? when calling it I got

"total_collateral_ratio": "2.217361196766775300136584335"

I believe you got this value because comes from the tests and there the prices are mocked. When I checked other frontends they show 222% what seems correct 🥳

3scava1i3r · 2023-10-05T18:01:03Z

Just confirming that I am still working on the same

LefterisJP · 2024-05-13T12:08:14Z

Just confirming that I am still working on the same

hey @3scava1i3r the PR has been open for 9 months with no activity. Please respond within this week if you can finish it or we can either close it or take it over and finish it ourselves.

feat:add the collateral ratio of the Liquity protocol in troves.py

7d346e7

Merge branch 'rotki:develop' into feat/liquity

bc584d9

yabirgb reviewed Sep 7, 2023

View reviewed changes

3scava1i3r added 2 commits September 9, 2023 11:14

fix: PR issues

f73138c

Merge branch 'feat/liquity' of https://github.com/3scava1i3r/rotki in…

6b1dedd

…to feat/liquity

yabirgb reviewed Sep 12, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat:add the collateral ratio of the Liquity protocol in troves.py #6578

feat:add the collateral ratio of the Liquity protocol in troves.py #6578

3scava1i3r commented Sep 7, 2023

CLAassistant commented Sep 7, 2023 •

edited

Loading

yabirgb left a comment

codecov bot commented Sep 7, 2023 •

edited

Loading

3scava1i3r commented Sep 9, 2023

yabirgb left a comment

yabirgb Sep 12, 2023

yabirgb Sep 12, 2023

yabirgb Sep 12, 2023

yabirgb Sep 12, 2023

yabirgb Sep 12, 2023

yabirgb Sep 12, 2023

yabirgb Sep 12, 2023

yabirgb Sep 12, 2023

3scava1i3r commented Oct 5, 2023

LefterisJP commented May 13, 2024

	total_collateral_ratio: Optional[float]
	total_collateral_ratio: Optional[FVal]

	total_collateral_ratio = eth_price * system_collateral / system_debt * lusd_price
	total_collateral_ratio = eth_price * FVal(system_collateral) / FVal(system_debt) * lusd_price

		},
		"total_collateral_ratio": "0.003138414276398289779741030787"

feat:add the collateral ratio of the Liquity protocol in troves.py #6578

Are you sure you want to change the base?

feat:add the collateral ratio of the Liquity protocol in troves.py #6578

Conversation

3scava1i3r commented Sep 7, 2023

Checklist

CLAassistant commented Sep 7, 2023 • edited Loading

yabirgb left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 7, 2023 • edited Loading

Codecov Report

3scava1i3r commented Sep 9, 2023

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

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

Choose a reason for hiding this comment

3scava1i3r commented Oct 5, 2023

LefterisJP commented May 13, 2024

CLAassistant commented Sep 7, 2023 •

edited

Loading

codecov bot commented Sep 7, 2023 •

edited

Loading