Sip 75 keeper isynths phase one #669

jjgonecrypto · 2020-08-17T20:12:32Z

Considerations

Rename ExchangeRates canFreezeRate() to isInverseRateAtBounds() which is more accurate
It could be better to return the inverse pricing struct, if any, in the ExchangeRates helper view

Ancillary Tasks

Update of SIP-75 with minor specification changes (to IExchangeRates)
Removal of all centralized oracle code (coming in a separate PR)
Add view to allow reverse lookup of address aggregator to bytes32 (see 2) below).

Notes

ExchangeRates.RatesUpdated no longer has inverse rate emitted but rather the companion long rate (to align it with AnswerUpdated). This event will be removed altogether in a separate PR
This change will require co-ordination with synthetix-subgraph to ensure that AnswerUpdated events on long synths cause a RateUpdate to the inverse (will require a reverse lookup on ExchangeRates - something like currenciesUsingAggregator(address aggregator) external view returns (bytes32[] currencies) to determine if a given aggregator update impacts an inverse

…etixio/synthetix into sip-76-warning-flags-invalid-rates

codecov · 2020-08-19T00:40:00Z

Codecov Report

Merging #669 into develop will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop     #669   +/-   ##
========================================
  Coverage    99.52%   99.52%           
========================================
  Files           51       51           
  Lines         3165     3191   +26     
  Branches       422      428    +6     
========================================
+ Hits          3150     3176   +26     
  Misses          15       15

Impacted Files	Coverage Δ
contracts/BinaryOptionMarketManager.sol	`100.00% <100.00%> (ø)`
contracts/ExchangeRates.sol	`98.78% <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 15ae9b2...d1ce697. Read the comment docs.

jacko125 · 2020-08-20T06:07:39Z

contracts/ExchangeRates.sol


-            _setRate(currencyKey, freezeAtUpperLimit ? upperLimit : lowerLimit, now);
+            inversePricing[currencyKey].frozenAtUpperLimit = freezeAtUpperLimit;


Is the intention here to reset both frozenAtUpperLimit and frozenAtLowerLimit to either false / true regardless during the resetting process? Ie, if frozenAtUpperLimit || frozenAtLowerLimit was true, we want them to be false after.

When setInversePricing is called, it can be frozen upper, lower, or not at all. Keep in mind this function is used both a) when repricing a synth and b) populating a new ExchangeRates to replicate the state in an older one, hence the need, on occasion, to be able to set an inverse as frozen directly.

jacko125 · 2020-08-20T06:28:40Z

contracts/ExchangeRates.sol

+        for (uint i = 0; i < aggregatorKeys.length; i++) {
+            bytes32 currencyKey = aggregatorKeys[i];
+            if (address(aggregators[currencyKey]) == aggregator) {
+                currencies[count++] = currencyKey;


@justinjmoses Is it going to be possible to have multiple currencies / prices based off the same aggregator address?

yes, so for example ETHUSD will support both sETH and iETH

contracts/ExchangeRates.sol

jacko125

Some points where gas efficiency can be improved for getting rates.

jjgonecrypto · 2020-08-20T17:41:28Z

RFAL

eternauta1337

Looks good. Only left minor comments.

contracts/ExchangeRates.sol

eternauta1337 · 2020-08-20T18:33:18Z

contracts/ExchangeRates.sol

+
+        uint rate = _getRate(currencyKey);
+
+        if (rate == inverse.upperLimit || rate == inverse.lowerLimit) {


By this code, I'm assuming that _getRate(...) returns a value bounded within the price limits. Even if that's the case, to improve readability and have a cero cost sanity check, use <= and >= instead?

Noted. I will adjust.

Actually then it will think 0 is a valid lowerLimit and it should not be. 0 will be returned for an inverse that has no underlying rate, and in which case it should not be frozen. I will add that condition as well

contracts/ExchangeRates.sol

publish/releases.json

eternauta1337 · 2020-08-20T19:01:40Z

publish/src/commands/deploy.js

@@ -1031,18 +1033,19 @@ const deploy = async ({
 				) {
 					if (oldExrates.options.address !== addressOf(exchangeRates)) {
 						const freezeAtUpperLimit = +w3utils.fromWei(currentRateForCurrency) === upperLimit;


Consider that Number.MAX_SAFE_INTEGER + 1 === Number.MAX_SAFE_INTEGER + 2 and MAX_SAFE_INTEGER = 9007199254740991, so might be a good idea to use big numbers here.

test/contracts/Exchanger.js

contracts/BinaryOptionMarketManager.sol

jjgonecrypto and others added 30 commits August 10, 2020 14:31

SIP-76 Warning flags interface for invalid rates

6a8c88b

Fixing broken tests

15638db

Adding coverage to SystemSetings

8734074

Allowing no flags to be set by default

a9f325d

Merge branch 'develop' into sip-76-warning-flags-invalid-rates

a7e01ff

Merge branch 'develop' into sip-76-warning-flags-invalid-rates

407bbbf

Merge branch 'sip-76-warning-flags-invalid-rates' of github.com:Synth…

727bb3b

…etixio/synthetix into sip-76-warning-flags-invalid-rates

Adding missing coverage

05b875e

Refactoring to use array getter of flags

57d1bc5

Moving mock to default of ExchangeRates test

b31d3b2

Tests for rateIsInvalid

c155a56

Fixing issues with mock

bd7f557

Merge branch 'develop' into sip-76-warning-flags-invalid-rates

8964359

Upgrading to use chainlink 0.0.9 interfaces

1469ebb

Minor optimizatoin

e2f03a3

Minor PR feedback on ExchangeRates logic

e6f94e3

Renaming require checks to show invalid

938ada1

Updating everything to invalid check with require everywhere

bc7e94d

Fixing broken tests

73ed8a2

SIP-75 Inverse rates determined on read, not on write

4a4a5a8

Testing WIP

7fe14d9

Merge branch 'develop' into sip-75-keeper-isynths-phase-one

095a1a1

Merge branch 'develop' into sip-75-keeper-isynths-phase-one

4d1bbfc

Fixing remainder of tests

c7b08a0

Adding view for ease of use

563c491

Migrating to new inverse pricing struct

011570d

Fixes to implementation

49eb986

Merge branch 'develop' into sip-75-keeper-isynths-phase-one

66694ed

Fixing publish tests

9bed144

Updating contracts to deploy

cbff5cf

Merge branch 'develop' into sip-75-keeper-isynths-phase-one

6ff71f3

jjgonecrypto added 4 commits August 18, 2020 21:58

Coverage for setInversePricing

806122c

More coverage

c8463bc

More coverage and fix

4991290

Test coverage for freezing rates

fb8701a

jjgonecrypto requested review from jacko125, eternauta1337 and zyzek August 19, 2020 05:42

jjgonecrypto marked this pull request as ready for review August 19, 2020 05:45

Adding function to get all currency keys for some aggregator address

3be9819

jacko125 reviewed Aug 20, 2020

View reviewed changes

contracts/ExchangeRates.sol Show resolved Hide resolved

jacko125 suggested changes Aug 20, 2020

View reviewed changes

PR feedback

89f8962

eternauta1337 approved these changes Aug 20, 2020

View reviewed changes

Further feedback around rate limits for frozen synths

026304a

jacko125 approved these changes Aug 21, 2020

View reviewed changes

zyzek suggested changes Aug 21, 2020

View reviewed changes

contracts/BinaryOptionMarketManager.sol Show resolved Hide resolved

Removing BOMM from contracts to deploy

d1ce697

zyzek approved these changes Aug 21, 2020

View reviewed changes

jjgonecrypto merged commit bf8829e into develop Aug 22, 2020

jjgonecrypto deleted the sip-75-keeper-isynths-phase-one branch August 22, 2020 00:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sip 75 keeper isynths phase one #669

Sip 75 keeper isynths phase one #669

jjgonecrypto commented Aug 17, 2020 •

edited

Loading

codecov bot commented Aug 19, 2020 •

edited

Loading

jacko125 Aug 20, 2020 •

edited

Loading

jjgonecrypto Aug 20, 2020

jacko125 Aug 20, 2020

jjgonecrypto Aug 20, 2020

jacko125 left a comment

jjgonecrypto commented Aug 20, 2020

eternauta1337 left a comment

eternauta1337 Aug 20, 2020

jjgonecrypto Aug 21, 2020

jjgonecrypto Aug 21, 2020

eternauta1337 Aug 20, 2020


		_setRate(currencyKey, freezeAtUpperLimit ? upperLimit : lowerLimit, now);
		inversePricing[currencyKey].frozenAtUpperLimit = freezeAtUpperLimit;


		uint rate = _getRate(currencyKey);

		if (rate == inverse.upperLimit \|\| rate == inverse.lowerLimit) {

Sip 75 keeper isynths phase one #669

Sip 75 keeper isynths phase one #669

Conversation

jjgonecrypto commented Aug 17, 2020 • edited Loading

Considerations

Ancillary Tasks

Notes

codecov bot commented Aug 19, 2020 • edited Loading

Codecov Report

jacko125 Aug 20, 2020 • edited Loading

Choose a reason for hiding this comment

jjgonecrypto Aug 20, 2020

Choose a reason for hiding this comment

jacko125 Aug 20, 2020

Choose a reason for hiding this comment

jjgonecrypto Aug 20, 2020

Choose a reason for hiding this comment

jacko125 left a comment

Choose a reason for hiding this comment

jjgonecrypto commented Aug 20, 2020

eternauta1337 left a comment

Choose a reason for hiding this comment

eternauta1337 Aug 20, 2020

Choose a reason for hiding this comment

jjgonecrypto Aug 21, 2020

Choose a reason for hiding this comment

jjgonecrypto Aug 21, 2020

Choose a reason for hiding this comment

eternauta1337 Aug 20, 2020

Choose a reason for hiding this comment

jjgonecrypto commented Aug 17, 2020 •

edited

Loading

codecov bot commented Aug 19, 2020 •

edited

Loading

jacko125 Aug 20, 2020 •

edited

Loading