SIP-184: Remove currentRound auto incrementation from ExchangeRates by leckylao · Pull Request #1630 · Synthetixio/synthetix

leckylao · 2021-12-11T08:25:11Z

setting sUSD on constructor;
moved currentRound auto increment into internalUpdateRates

- setting sUSD on constructor; - moved currentRound auto increment into internalUpdateRates

codecov · 2021-12-11T09:51:18Z

Codecov Report

Merging #1630 (07807d5) into feat/sip-184-dynamic-fees (e905aca) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@                    Coverage Diff                     @@
##           feat/sip-184-dynamic-fees    #1630   +/-   ##
==========================================================
  Coverage                      95.73%   95.74%           
==========================================================
  Files                             83       83           
  Lines                           1945     1949    +4     
  Branches                         609      610    +1     
==========================================================
+ Hits                            1862     1866    +4     
  Misses                            83       83

Impacted Files	Coverage Δ
contracts/ExchangeRates.sol	`100.00% <100.00%> (ø)`
contracts/Exchanger.sol	`96.80% <100.00%> (+0.14%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e905aca...07807d5. Read the comment docs.

artdgn

I was sure we said that removing the curreRate state variable is the best course of action.

The only place it's used right now is the exchanger for getting the last cached rate for dynamic fee to get the correct set of prices. It can be removed there because you already have the roundIds in the _exchange method that you can pass into the fees method to get the historic prices.

This way, this whole issue with coupling, and incrementing, etc can be avoided.

artdgn · 2021-12-12T23:51:57Z

contracts/ExchangeRates.sol

-        // The sUSD rate is always 1 and is never stale.
-        _setRate("sUSD", 0, SafeDecimalMath.unit(), now);
+        // The sUSD rate is always 1 on round 1 and is never stale.
+        currentRoundForRate["sUSD"]++;


nit: can be just = 1 probably

artdgn · 2021-12-13T00:04:36Z

contracts/ExchangeRates.sol

        (sourceRate, time) = _getRateAndTimestampAtRound(sourceCurrencyKey, roundIdForSrc);
        // cacheing to save external call
-        _setRate(sourceCurrencyKey, roundIdForSrc, sourceRate, time);
+        if (sourceCurrencyKey != "sUSD") _setRate(sourceCurrencyKey, roundIdForSrc, sourceRate, time);


the != sUSD check should probably me moved into the _setRate method to avoid the duplicate code and inline ifs. It's probably belongs there anyway because that's the method that has handles the caching and knows how sUSD rate should be cached.

nit: I'd prefer to avoid inline ifs, they're easy to miss when reading, so impact readability and clarity. Aren't used in other places in code. [If there are too many blocks due to nested ifs, it probably means that the code is written in a complex way and is doing too many things and can be refactored, shortening the syntax is only making it worse IMO]

It does has the != sUSD check in _setRate, here is when reading the sUSD rate don't try to set it. Will put a comment there.

I meant to remove the require in _setRate, and instead just return if it's sUSD.

So in _setRate instead of this:

require(currencyKey != "sUSD", "Rate of sUSD cannot be updated, it's always UNIT.");

maybe replace with this:

if (currencyKey == "sUSD") { // no caching is done for sUSD return; }

This way, there is no need to have those ifs elsewhere and:

code is easier to read (less ifs)

whoever is calling setRate doesn't need to check the currency

Also, nit: "sUSD" should probably be a named constant.

artdgn · 2021-12-13T01:06:17Z

contracts/ExchangeRates.sol

+        require(currencyKey != "sUSD", "Rate of sUSD cannot be updated, it's always UNIT.");
+        require(roundId > 0, "Round id must be greater than 0.");
+
+        currentRoundForRate[currencyKey] = roundId;


I was under the impression that we've agreed that currentRoundForRate should be removed because it introduces more risks, potential for bugs, and too much coupling (between rates and exchanger contract).

This is the first iteration. And currentRoundForRate is more for improvement due to the tightly coupled code and timeframe.

- Using constant for sUSD; - no cache for sUSD in setRate

leckylao · 2021-12-13T13:01:38Z

I was sure we said that removing the curreRate state variable is the best course of action.

The only place it's used right now is the exchanger for getting the last cached rate for dynamic fee to get the correct set of prices. It can be removed there because you already have the roundIds in the _exchange method that you can pass into the fees method to get the historic prices.

This way, this whole issue with coupling, and incrementing, etc can be avoided.

Yes, it's pretty much doing the same, instead of creating more params in order to pass in the roundId, it's just reusing the currentRoundForRate that just set by the mutativeEffectiveValueAndRatesAtRound.

artdgn

That's great, now that currentRoundForRate is not used, I think it should be possible to remove it also from the ExchangeRates contract.

artdgn · 2021-12-13T22:57:45Z

contracts/Exchanger.sol

        returns (uint exchangeFeeRate)
    {
-        exchangeFeeRate = _feeRateForExchange(sourceCurrencyKey, destinationCurrencyKey);
+        exchangeFeeRate = _feeRateForExchange(sourceCurrencyKey, destinationCurrencyKey, 0, 0);


I see you're using 0, 0 to mean latest rounds. I think it's better to use the correct latestRounds here, and avoid having special logic for 0 there. Less implicit special logic for special values will reduce edge cases and surface area for bugs (explicit > implicit).

Alternatively you can create another method that doesn't expect roundId numbers: _feeRateForExchangeLatestRounds that doesn't expect roundIds and get them from exchangeRates.

artdgn · 2021-12-13T23:04:27Z

contracts/Exchanger.sol

-        // Note: We are using cache round ID here for cheap read
-        uint currentRoundId = exchangeRates().currentRoundForRate(currencyKey);
-        (prices, ) = exchangeRates().ratesAndUpdatedTimeForCurrencyLastNRounds(currencyKey, rounds, currentRoundId);
+        if (roundId == 0) roundId = exchangeRates().getCurrentRoundId(currencyKey);


I don't think this method should try to be smart about the roundId it's getting. It's better to avoid using 0 as a special case if we can. This will reduce coupling, edge cases, and surface area for bugs. This method should just work with the roundId it's getting IMO because it's a low level method.

artdgn · 2021-12-13T23:11:33Z

contracts/Exchanger.sol

        )
    {
-        exchangeFeeRate = _feeRateForExchange(sourceCurrencyKey, destinationCurrencyKey);
+        exchangeFeeRate = _feeRateForExchange(sourceCurrencyKey, destinationCurrencyKey, 0, 0);


same comment as above.

- Using level method to avoid explicite condition

jjgonecrypto · 2022-01-18T23:51:20Z

Closed in favor of #1649

leckylao added 3 commits December 11, 2021 19:24

fix(ExchangeRates): Remove currentRound auto incrementation

140ad99

- setting sUSD on constructor; - moved currentRound auto increment into internalUpdateRates

skip cache setRate for sUSD

083dba9

Allow setRate for 0 so that it could be trigger by circuit breaker

32ba730

leckylao requested review from artdgn and jjgonecrypto December 11, 2021 10:57

jjgonecrypto changed the title ~~fix(ExchangeRates): Remove currentRound auto incrementation~~ SIP-184: Remove currentRound auto incrementation from ExchangeRates Dec 13, 2021

artdgn reviewed Dec 13, 2021

View reviewed changes

leckylao added 3 commits December 13, 2021 13:55

Fixed comment

6eda259

fix comment:

15b5578

- Using constant for sUSD; - no cache for sUSD in setRate

Fix test for skip setting sUSD

11c20f2

leckylao requested a review from artdgn December 13, 2021 13:12

Remove using of currentRoundForRate

59f52b7

artdgn suggested changes Dec 13, 2021

View reviewed changes

Fix PR comment

07807d5

- Using level method to avoid explicite condition

jjgonecrypto closed this Jan 18, 2022

jjgonecrypto deleted the fix/remove-current-round-auto-incrementation branch January 18, 2022 23:51

Conversation

leckylao commented Dec 11, 2021

Uh oh!

codecov bot commented Dec 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

artdgn left a comment

Choose a reason for hiding this comment

Uh oh!

artdgn Dec 12, 2021

Choose a reason for hiding this comment

Uh oh!

artdgn Dec 13, 2021

Choose a reason for hiding this comment

Uh oh!

leckylao Dec 13, 2021

Choose a reason for hiding this comment

Uh oh!

artdgn Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artdgn Dec 13, 2021

Choose a reason for hiding this comment

Uh oh!

leckylao Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leckylao commented Dec 13, 2021

Uh oh!

artdgn left a comment

Choose a reason for hiding this comment

Uh oh!

artdgn Dec 13, 2021

Choose a reason for hiding this comment

Uh oh!

artdgn Dec 13, 2021

Choose a reason for hiding this comment

Uh oh!

artdgn Dec 13, 2021

Choose a reason for hiding this comment

Uh oh!

jjgonecrypto commented Jan 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Dec 11, 2021 •

edited

Loading

artdgn Dec 13, 2021 •

edited

Loading

leckylao Dec 13, 2021 •

edited

Loading