Sip 184 merge develop by leckylao · Pull Request #1642 · Synthetixio/synthetix

leckylao · 2022-01-08T12:45:43Z

No description provided.

- created libraries folder - moving SafeDecimalMath into libraries

- adding decimals into SafeDecimalMath

- Adding dynamic fee constants;

- Adding require check for price; - Adding setup for test;

- Adding more rounds of price feeds for test; - Adding dynamic fee in systemSettings in test

- removed fastForward as startTime and Now is not equal

- Rename priceWeight to RoundDecay - adding cap 100% for max exchangeFeeRate - Added return for invalid rate - linking libraries in setup

…nthetix into feat/sip-184-dynamic-fees

- disable dynamic fee when price feeds less than 2 rounds - disable dynamic fee on exchanger spec settle

- move setPriceAggregator out of the updateRates loop

- Fix set price with no volatility on exchangeAtomically

- remove format from cacheRate as it's already been formatted

- Having the rounds rate loop inside the aggregator

- Using roundId 1 for sUSD to be consistent - Fix interface wrong compiler version warning

- adding --max-old-space-size

codecov · 2022-01-08T13:36:18Z

Codecov Report

Merging #1642 (1f500db) into develop (d34061c) will increase coverage by 0.09%.
The diff coverage is 100.00%.

❗ Current head 1f500db differs from pull request most recent head 3118295. Consider uploading reports for the commit 3118295 to get more accurate results

@@             Coverage Diff             @@
##           develop    #1642      +/-   ##
===========================================
+ Coverage    95.58%   95.68%   +0.09%     
===========================================
  Files           82       83       +1     
  Lines         1904     1946      +42     
  Branches       593      610      +17     
===========================================
+ Hits          1820     1862      +42     
  Misses          84       84

Impacted Files	Coverage Δ
contracts/DynamicFee.sol	`100.00% <100.00%> (ø)`
contracts/ExchangeRates.sol	`98.75% <100.00%> (+0.19%)`	⬆️
contracts/Exchanger.sol	`96.93% <100.00%> (+0.30%)`	⬆️
contracts/MixinSystemSettings.sol	`100.00% <100.00%> (ø)`
contracts/SystemSettings.sol	`100.00% <100.00%> (ø)`

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 54a9c52...3118295. Read the comment docs.

* Fix CI test - increase stake behaviour SNX mint - update setRate to deploy mockAggregator * Revert "Fix CI test" This reverts commit 182b34c. * Revert "Revert "Fix CI test"" This reverts commit 1be5282.

- update setMissingRates to always add all rates

- increase SNXAmount on stake

- increase SNX stake amount

artdgn · 2022-01-10T02:20:25Z

Should #1563 be closed in favour of this one @leckylao ?

artdgn · 2022-01-10T04:16:16Z

contracts/ExchangeRates.sol

+        uint rate,
+        uint time
+    ) internal {
+        if (rate > 0) {


should check if rate != cacheRates[currencyKey][roundId].rate to avoid SSTORE for rewriting same rate

artdgn · 2022-01-10T04:22:06Z

contracts/ExchangeRates.sol

        return false;
    }

+    function anyRateIsInvalidAtRound(bytes32[] calldata currencyKeys, uint[] calldata roundIds)


This seems to be used only in exchanger and always for two rates, does this need to accept arrays, maybe a single rate method can be used twice instead.

This checks for flags and time, but doesn't check for 0 rate. probably needs a test case as well. If method's intent is time only, than should be renamed to anyRateStaleAtRound or smth..

Would be good to add doc comments (and specify what invalid means in this case).

This function is meant the be consistent with function anyRateIsInvalid so the recommendation would likely be apply on both functions therefore would just leave it for now and mark as improvement.

It looks like the array variants are more gas efficient in terms of number of external calls vs. checking each rate separately.

artdgn · 2022-01-10T04:32:01Z

contracts/Exchanger.sol

+        uint[] memory roundIds = new uint[](2);
+        roundIds[0] = roundIdForSrc;
+        roundIds[1] = roundIdForDest;
+        require(!exchangeRates().anyRateIsInvalidAtRound(synthKeys, roundIds), "Src/dest rate invalid or not found");


As above, can probably check the two rates specificaly instead of creating and iterating on array of 2

Not sure if "or not found" part of message is needed or accurate.

Similar that the function is meant the be consistent with function _ensureCanExchange so not necessary changing the legacy function would just leave it for now and mark as improvement.

artdgn · 2022-01-10T04:35:03Z

contracts/Exchanger.sol

+    function _getDynamicFeeForExchange(bytes32 currencyKey) internal view returns (uint dynamicFee) {
+        // No dynamic fee for sUSD
+        if (currencyKey == sUSD) return 0;
+        (uint threshold, uint weightDecay, uint rounds) = getExchangeDynamicFeeData();


nit: getExchangeDynamicFeeData -> getExchangeDynamicFeeParams or getExchangeDynamicFeeConfig

artdgn · 2022-01-10T04:37:30Z

contracts/MixinSystemSettings.sol

+
+    /// @notice Get exchange max dynamic fee
+    /// @return max dynamic fee
+    function getExchangeMaxDynamicFee() internal view returns (uint) {


should this be added to getExchangeDynamicFeeData to query for them all together?

It's fetching from different functions though, might just leave it for now.

artdgn · 2022-01-10T04:58:06Z

test/dynamic-fee-calc.csv

@@ -0,0 +1,124 @@
+,,,,,,,
+,round,price,ΔP (bp),boost,dynamic_fee (bp),,deviation threshold (bp)


please add the formulas text so that numbers can be recreated / checked / adjusted

Would be refer back to the xlsx spreadsheet.

artdgn · 2022-01-10T04:58:39Z

test/integration/behaviors/stake.behavior.js

 function itCanStake({ ctx }) {
 	describe('staking and claiming', () => {
-		const SNXAmount = ethers.utils.parseEther('100');
+		const SNXAmount = ethers.utils.parseEther('100000');


why was this change needed? (in another PR it was changed to 1000 :)

Was trying to fix this but unfortunately works on the other PR but not this one
https://app.circleci.com/pipelines/github/Synthetixio/synthetix/9775/workflows/f2026f37-4e7f-4a23-9127-b1d31f20adac/jobs/101109?invite=true#step-109-1867

artdgn · 2022-01-10T04:59:50Z

test/integration/utils/rates.js

 	await setSystemSetting({ ctx, settingName: 'rateStalePeriod', newValue: '1000000000' });

+	// try to add the missing rates
+	await _setMissingRates({ ctx });


I don't think we should remove the check, instead we should check if the aggregators have enough rates for the dynamic fee, and if they don't add them. This always adds and overwrites aggregators, which makes the integration tests less "integration".

I am not too familiar with the aggregator stuff and how to change that. As you added this part would you mind having a new PR on top of this to change it?

artdgn · 2022-01-10T05:01:35Z

test/integration/utils/rates.js

-			aggregator = aggregator.connect(owner);
-			// set decimals
-			await (await aggregator.setDecimals(18)).wait();
+		// deploy an aggregator


As these are integration tests, this should probably try to add rates if they're missing, and not always. So probably the check needs to be altered and not removed - it should check probably if there are enough rates, and if there aren't - add them.

artdgn · 2022-01-10T05:27:28Z

test/integration/utils/rates.js


-	const { timestamp } = await ctx.provider.getBlock();
+	// factory for price aggregators contracts
+	const MockAggregatorFactory = await createMockAggregatorFactory(ctx.users.owner);


as in the other PR - this method shouldn't deploy and add aggregators, this should be a separate explicit setup step, otherwise this could hide a broken scenario or fork test

artdgn · 2022-01-10T05:29:33Z

Also, would be nice to see an updated gas usage report.

- _setCacheRate also check non-duplicate entry - remove duplicate rates in Exchanger.spec.js

This reverts commit e558ac9.

artdgn · 2022-01-17T22:19:13Z

Work continued in #1649

leckylao added 30 commits October 11, 2021 17:49

created DynamicFee library

6c2c814

- created libraries folder - moving SafeDecimalMath into libraries

Merge branch 'develop' into feat/sip-184-dynamic-fees

c853490

moving Math into libraries

aaf12e5

- adding decimals into SafeDecimalMath

fixing import Math path;

b707cbc

adding implementation for DynamicFee

6e9a995

Adding Natspec

82e71a5

Adding test for DynamicFee

82808cf

Adding more tests for DynamicFee

89116c3

Try to fix CI Cov by moving test file

278b3d7

revert libraries folder and move to new PR

bb26601

revert libraries path

f4f4218

fixed test with data from Afif

980bc20

feat(Exchanger): Adding dynamic fee into exchange fee

723c6e6

- Adding dynamic fee constants;

Merge branch 'develop' into feat/sip-184-dynamic-fees

c5c856d

fix(test): updateRates for rounds

8b1d491

- Adding require check for price; - Adding setup for test;

fix(test): Adding dynamic fee to 0 for sUSD

ff48fb9

- Adding more rounds of price feeds for test; - Adding dynamic fee in systemSettings in test

Fixing more tests and adding in more descriptions

849583a

fix test by adding rounds for price feeds

5cd88ec

fixed FeePool test

89b78c6

- removed fastForward as startTime and Now is not equal

fixing ExchangerWithVirtualSynth link in setup.js

c56aa8b

fixing setup library linking

5302a44

fixing setup in test

8aa8c5e

Merge branch 'develop' into feat/sip-184-dynamic-fees

c2f1ba2

fixing exhcnager test

34b92af

- Rename priceWeight to RoundDecay - adding cap 100% for max exchangeFeeRate - Added return for invalid rate - linking libraries in setup

Merge branch 'develop' into feat/sip-184-dynamic-fees

29ac65c

use iBTC as destinationCurrency as it got rate.

76451fe

Merge branch 'feat/sip-184-dynamic-fees' of github.com:Synthetixio/sy…

a999fe3

…nthetix into feat/sip-184-dynamic-fees

move cap max exchange fee into exchange

f30f8e3

fixing exchanger.spec.js

5a30543

- disable dynamic fee when price feeds less than 2 rounds - disable dynamic fee on exchanger spec settle

fixed ExchangerWithVirtualSynth linking in test

c636a28

leckylao added 8 commits January 4, 2022 14:09

Fix Exchanger.spec.js

d904b7a

- move setPriceAggregator out of the updateRates loop

Fix Exchanger.spec.js

1d1ef57

- Fix set price with no volatility on exchangeAtomically

Fix test/publish

b599187

- remove format from cacheRate as it's already been formatted

Using roundId 1 as last round to be consistent

d41d216

Fix integration test

2b4bd73

- Having the rounds rate loop inside the aggregator

Fix ExchangeRates.js

732e2cb

- Using roundId 1 for sUSD to be consistent - Fix interface wrong compiler version warning

Try to fix CI unit test

ab1b89c

- adding --max-old-space-size

Merge branch 'develop' into sip-184-merge-develop

9d381d2

leckylao changed the base branch from develop to feat/sip-184-dynamic-fees January 8, 2022 13:06

leckylao changed the base branch from feat/sip-184-dynamic-fees to develop January 8, 2022 13:06

leckylao added 4 commits January 9, 2022 20:12

Fix CI test (#1644)

d5635a7

* Fix CI test - increase stake behaviour SNX mint - update setRate to deploy mockAggregator * Revert "Fix CI test" This reverts commit 182b34c. * Revert "Revert "Fix CI test"" This reverts commit 1be5282.

Fix integration test

5b71a8d

- update setMissingRates to always add all rates

Fix stake.behavior.js

4223d85

- increase SNXAmount on stake

Fix stake integration test

e558ac9

- increase SNX stake amount

artdgn suggested changes Jan 10, 2022

View reviewed changes

leckylao and others added 4 commits January 10, 2022 17:51

Fix 184 comments

60fed58

- _setCacheRate also check non-duplicate entry - remove duplicate rates in Exchanger.spec.js

Revert "Fix stake integration test"

7c1d7c4

This reverts commit e558ac9.

Change stake.behavior.js SNXAmount back to 1000

2cca728

Merge branch 'develop' into sip-184-merge-develop

e628cd0

artdgn mentioned this pull request Jan 11, 2022

Feat(DynamicFee): SIP-184 dynamic fees #1563

Closed

11 tasks

Merge branch 'develop' into sip-184-merge-develop

1f500db

artdgn changed the base branch from develop to feat/reduce-system-settings-size January 12, 2022 23:45

artdgn changed the base branch from feat/reduce-system-settings-size to develop January 12, 2022 23:47

Merge branch 'develop' into sip-184-merge-develop

3118295

artdgn mentioned this pull request Jan 13, 2022

SIP-184 Dynamic Fees #1649

Merged

artdgn closed this Jan 17, 2022

		@@ -0,0 +1,124 @@
		,,,,,,,
		,round,price,ΔP (bp),boost,dynamic_fee (bp),,deviation threshold (bp)

Conversation

leckylao commented Jan 8, 2022

Uh oh!

codecov bot commented Jan 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

artdgn commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artdgn commented Jan 10, 2022

Uh oh!

artdgn commented Jan 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Jan 8, 2022 •

edited

Loading

artdgn commented Jan 10, 2022 •

edited

Loading