Implementation of tickets 1604, 1597 and 1365 #2368

BatiGencho · 2020-01-06T14:51:52Z

Description

Implementation of tickets 1604, 1597 and 1365

Related issues

Fixes Internal audit of parameter validation in contract initialization #1604, Stability fee should be zero then kick in automatically after ~18months #1597, Replace Numerator/Denominator for fixidity in SortedOracles #1365

Note

Regarding SortedOracles.sol, we are changing the type of values stored in the state variable r"ates" to unwrapped Fixidity fractions meaning those changes are not backwards compatible and shall remain as so onwards. The latter is to take effect on all interacting package levels.

packages/protocol/contracts/common/MultiSig.sol

packages/protocol/contracts/governance/EpochRewards.sol

packages/protocol/test/stability/sortedoracles.ts

packages/protocol/contracts/stability/StableToken.sol

jfoutts-celo · 2020-01-07T18:11:02Z

packages/protocol/contracts/stability/SortedOracles.sol

@@ -121,23 +124,19 @@ contract SortedOracles is ISortedOracles, Ownable, Initializable {
  /**
   * @notice Updates an oracle value and the median.
   * @param token The address of the token for which the Celo Gold exchange rate is being reported.
-   * @param numerator The amount of tokens equal to `denominator` Celo Gold.
-   * @param denominator The amount of Celo Gold equal to `numerator` tokens.
+   * @param value The amount of tokens equal to Celo Gold represented as a FixidityLib fraction


It this comment accurate? It seems value is also expected to have been multiplied by getDenominator(). Is this what we want? Now that we're using Fixidity, could we keep the internal denominator private and unexposed?

param description fixed, internal denominator is private, but I have added a getter needed by the cli and contractskit

packages/protocol/contracts/stability/SortedOracles.sol

jfoutts-celo · 2020-01-07T18:18:55Z

packages/protocol/contracts/stability/SortedOracles.sol

+   * @notice Returns the common fixidity denominator.
+   * @return The common fixidity denominator.
+   */
+  function getDenominator() public view returns (uint256) {


Do we need this?

since we have the int. denominator as private, I have added a public method that is to be used by external sources e.g. contractskit and cli. Please let me know if any objections to that

mcortesi

Reviewed changes for 1604

mcortesi · 2020-01-09T16:45:04Z

packages/protocol/contracts/common/MultiSig.sol

@@ -109,15 +110,34 @@ contract MultiSig is Initializable {
    initializer
    validRequirement(_owners.length, _required)
  {
+    setInitialOwners(_owners);


I think the idea here is to call addOwner and changeRequirement instead of creating new setters that will only be used on initialize.

And since those functions are external probably makes sense to have an _addOwner internal function that both initialize and addOwner calls. Some for the other function

mcortesi · 2020-01-09T16:49:20Z

packages/protocol/contracts/governance/EpochRewards.sol

+   * @return True upon success.
+   * @dev unwrapped Fixidity value expected for _targetVotingYield
+   */
+  function setTargetVotingYield(uint256 _targetVotingYield) public onlyOwner returns (bool) {


I'm not sure for this contract, but we are changing the semantics here... since dureing the lifetime of the contract we can change the target voting yield.
I wonder if that's what we want.

@asaj @marekolszewski comments?

+1, the scope of this ticket was more about reducing tech debt than exposing setters for values that were previously not possible to set externally

mcortesi · 2020-01-09T16:49:25Z

packages/protocol/contracts/governance/EpochRewards.sol

+   * @param _startTime The time in unix.
+   * @return True upon success.
+   */
+  function setStartTime(uint256 _startTime) public onlyOwner returns (bool) {


Same
@asaj @marekolszewski comments?

mcortesi · 2020-01-09T16:50:59Z

packages/protocol/contracts/governance/Governance.sol

+   * @notice Updates the last dequeue timestamp.
+   * @param _lastDequeue The last dequeue timestamp.
+   */
+  function setLastDequeue(uint256 _lastDequeue) public onlyOwner {


Same...
@asaj @marekolszewski comments?

mcortesi · 2020-01-09T16:54:07Z

packages/protocol/contracts/stability/StableToken.sol

+   * @notice Setter for the total supply.
+   * @param totalSupply The total supply to set.
+   */
+  function setTotalSupply(uint256 totalSupply) public {


I don't think we want a public function that lets you change the total supply

@asaj @marekolszewski comments?

mcortesi · 2020-01-09T16:54:50Z

packages/protocol/contracts/stability/StableToken.sol

+   * @notice Setter for the decimals of the token.
+   * @param decimals The decimals to set.
+   */
+  function setDecimals(uint8 decimals) public {


Same here... you can't change decimal at half of the lifecycle

mcortesi · 2020-01-09T16:55:11Z

packages/protocol/contracts/stability/StableToken.sol

+   * @notice Setter for the name of the token.
+   * @param name The name to set.
+   */
+  function setName(string memory name) public {


What about changing name or symbol during the lifecycle?

@asaj @marekolszewski comments?

mcortesi

@BatiGencho where's 1597 implemented??

mcortesi · 2020-01-09T17:20:14Z

packages/protocol/contracts/stability/SortedOracles.sol

@@ -121,23 +124,19 @@ contract SortedOracles is ISortedOracles, Ownable, Initializable {
  /**
   * @notice Updates an oracle value and the median.
   * @param token The address of the token for which the Celo Gold exchange rate is being reported.
-   * @param numerator The amount of tokens equal to `denominator` Celo Gold.
-   * @param denominator The amount of Celo Gold equal to `numerator` tokens.
+   * @param value The amount of tokens equal to Celo Gold represented as a FixidityLib fraction, premultiplied with the denominator


The idea of #1365 is not to premultiply with denominator, but instead make users report rates as a fractional number (encoded as a fixidity number).

So if rate is 1, it should be encoded as 1000000000000000000000000
and if it's 0.5 it should be 500000000000000000000000

mcortesi · 2020-01-09T17:20:45Z

packages/protocol/contracts/stability/SortedOracles.sol

  // All oracle rates are assumed to have a denominator of 2 ^ 64.
-  uint256 public constant DENOMINATOR = 0x10000000000000000;
+  uint256 private constant DENOMINATOR = 0x10000000000000000;


thus, this should be removed

…nt naming fixes, EpochRewards -> few setters switched to privet, Governance -> setLastDequeue() switched to private, StableToken -> setters for name, symbol, and decimals removed

… limechain/t_1604_1597_1365 # Conflicts: # packages/protocol/contracts/common/MultiSig.sol

numerator, denominator replaced with medianValue, unit test update

asaj · 2020-01-22T00:43:54Z

packages/protocol/contracts/common/GasPriceMinimum.sol

@@ -82,7 +82,11 @@ contract GasPriceMinimum is Ownable, Initializable, UsingRegistry {
      uint256 rateNumerator;
      uint256 rateDenominator;
      (rateNumerator, rateDenominator) = sortedOracles.medianRate(tokenAddress);
-      return (gasPriceMinimum.mul(rateNumerator).div(rateDenominator));
+      FixidityLib.Fraction memory ratio = FixidityLib.newFixedFraction(


nit: s/ratio/rate

asaj · 2020-01-22T00:46:41Z

packages/protocol/contracts/common/MultiSig.sol

@@ -20,6 +20,7 @@ contract MultiSig is Initializable {
  event OwnerAddition(address indexed owner);
  event OwnerRemoval(address indexed owner);
  event RequirementChange(uint256 required);
+  event RequirementSet(uint256 required);


Where is this used?

asaj · 2020-01-22T00:55:58Z

packages/protocol/contracts/stability/SortedOracles.sol

@@ -182,7 +172,7 @@ contract SortedOracles is ISortedOracles, Ownable, Initializable {
   * @return The median exchange rate for `token`.
   */
  function medianRate(address token) external view returns (uint256, uint256) {
-    return (rates[token].getMedianValue(), numRates(token) == 0 ? 0 : DENOMINATOR);
+    return (rates[token].getMedianValue(), numRates(token));


It feels dangerous to change the behavior here unless you're 100% certain you've made changes everywhere the output of this function is consumed

asaj · 2020-01-22T01:02:05Z

packages/protocol/migrations/08_stabletoken.ts

    await sortedOracles.report(
      stableToken.address,
-      config.stableToken.goldPrice,
-      1,
+      new BigNumber(config.stableToken.goldPrice / 1),


Shouldn't we multiply by 10^24? i.e. use toFixed here?

asaj · 2020-01-22T01:02:30Z

packages/protocol/scripts/truffle/set_exchange_rate.ts

-    await oracles.report(stableToken.address, numerator, denominator, lesserKey, greaterKey)
+    await oracles.report(
+      stableToken.address,
+      numerator.dividedBy(denominator),


Ditto, we need to use toFixed here

asaj · 2020-01-22T01:04:19Z

packages/protocol/test/stability/sortedoracles.ts

@@ -115,7 +117,9 @@ contract('SortedOracles', (accounts: string[]) => {

    describe('when a report has been made', () => {
      beforeEach(async () => {
-        await sortedOracles.report(aToken, 1, 1, NULL_ADDRESS, NULL_ADDRESS, { from: anOracle })
+        await sortedOracles.report(aToken, new BigNumber(1), NULL_ADDRESS, NULL_ADDRESS, {


I think you want toFixed(1) here

asaj · 2020-01-22T01:04:25Z

packages/protocol/test/stability/sortedoracles.ts

@@ -128,7 +132,7 @@ contract('SortedOracles', (accounts: string[]) => {
          for (let i = 7; i > 3; i--) {
            const anotherOracle = accounts[i]
            await sortedOracles.addOracle(aToken, anotherOracle)
-            await sortedOracles.report(aToken, 2, 1, anOracle, NULL_ADDRESS, {
+            await sortedOracles.report(aToken, new BigNumber(2), anOracle, NULL_ADDRESS, {


asaj · 2020-01-22T01:04:32Z

packages/protocol/test/stability/sortedoracles.ts

@@ -170,7 +174,7 @@ contract('SortedOracles', (accounts: string[]) => {

    describe('when a report has been made', () => {
      beforeEach(async () => {
-        await sortedOracles.report(aToken, 10, 1, NULL_ADDRESS, NULL_ADDRESS, {
+        await sortedOracles.report(aToken, new BigNumber(10), NULL_ADDRESS, NULL_ADDRESS, {


toFixed(10)

asaj · 2020-01-22T01:05:06Z

packages/protocol/test/stability/sortedoracles.ts

    }

-    const numerator = 10
+    const numerator = 5


No need to define these as numerator and denominator now, you can just use a single "rate"

asaj · 2020-01-22T01:08:46Z

packages/protocol/test/stability/stabletoken.ts

@@ -178,13 +179,17 @@ contract('StableToken', (accounts: string[]) => {
      beforeEach(async () => {
        await stableToken.setInflationParameters(inflationRate, SECONDS_IN_A_WEEK)
        await timeTravel(SECONDS_IN_A_WEEK, web3)
+        await timeTravel(EighteenMonthsInSeconds, web3)


nit: Consider time traveling the eighteen months before the wek

…rate.ts the median rate is multiplied by 10^24 (toFixed), sortedoracles.ts (unittests) numerator / denomerator replaced with rate

sortedoracles unit tests small assertion correction

asaj · 2020-01-24T19:54:07Z

packages/protocol/contracts/common/GasPriceMinimum.sol

@@ -79,13 +79,10 @@ contract GasPriceMinimum is Ownable, Initializable, UsingRegistry {
      ISortedOracles sortedOracles = ISortedOracles(
        registry.getAddressForOrDie(SORTED_ORACLES_REGISTRY_ID)
      );
-      uint256 rateNumerator;
+      uint256 rate;


nit: prefer rateNumerator

asaj · 2020-01-24T19:56:07Z

packages/protocol/scripts/truffle/set_exchange_rate.ts

@@ -135,7 +136,7 @@ module.exports = async (callback: (error?: any) => number) => {
    // Report it
    await oracles.report(
      stableToken.address,
-      numerator.dividedBy(denominator),
+      toFixed(new BigNumber(numerator).dividedBy(new BigNumber(denominator))),


No need to make these bignumbers, you should be able to do toFixed(numerator / denominator)

asaj · 2020-01-24T19:56:27Z

packages/protocol/test/governance/epochrewards.ts

@@ -416,7 +416,7 @@ contract('EpochRewards', (accounts: string[]) => {
      })

      it('should return one', async () => {
-        assertEqualBN(await epochRewards.getRewardsMultiplier(), toFixed(1))
+        assertEqualBN(await epochRewards.getRewardsMultiplier(), toFixed(new BigNumber(1)))


ditto, toFixed(1)

asaj · 2020-01-24T19:56:31Z

packages/protocol/test/stability/sortedoracles.ts

@@ -117,7 +117,7 @@ contract('SortedOracles', (accounts: string[]) => {

    describe('when a report has been made', () => {
      beforeEach(async () => {
-        await sortedOracles.report(aToken, new BigNumber(1), NULL_ADDRESS, NULL_ADDRESS, {
+        await sortedOracles.report(aToken, toFixed(new BigNumber(1)), NULL_ADDRESS, NULL_ADDRESS, {


asaj · 2020-01-24T19:56:35Z

packages/protocol/test/stability/sortedoracles.ts

@@ -132,7 +132,7 @@ contract('SortedOracles', (accounts: string[]) => {
          for (let i = 7; i > 3; i--) {
            const anotherOracle = accounts[i]
            await sortedOracles.addOracle(aToken, anotherOracle)
-            await sortedOracles.report(aToken, new BigNumber(2), anOracle, NULL_ADDRESS, {
+            await sortedOracles.report(aToken, toFixed(new BigNumber(2)), anOracle, NULL_ADDRESS, {


asaj · 2020-01-24T19:56:40Z

packages/protocol/test/stability/sortedoracles.ts

@@ -174,7 +174,7 @@ contract('SortedOracles', (accounts: string[]) => {

    describe('when a report has been made', () => {
      beforeEach(async () => {
-        await sortedOracles.report(aToken, new BigNumber(10), NULL_ADDRESS, NULL_ADDRESS, {
+        await sortedOracles.report(aToken, toFixed(new BigNumber(10)), NULL_ADDRESS, NULL_ADDRESS, {


asaj · 2020-01-24T19:56:52Z

packages/protocol/test/stability/sortedoracles.ts

-    const numerator = 5
-    const denominator = 1
-    const expectedMedianRate = expectedMedianRateFromGiven(numerator, denominator)
+    const givenMedianRate = toFixed(new BigNumber(5).dividedBy(new BigNumber(1)))


ditto, toFixed(5)

asaj · 2020-01-24T19:57:07Z

packages/protocol/test/stability/sortedoracles.ts

-      assertEqualBN(actualMedianRate, expectedMedianRate)
-      assertEqualBN(numberOfRates.toNumber(), 1)
+      assertEqualBN(actualMedianRate, givenMedianRate)
+      assertEqualBN(numberOfRates, toFixed(new BigNumber(1)))


asaj · 2020-01-24T19:57:20Z

packages/protocol/test/stability/sortedoracles.ts

    })

    describe('when there exists exactly one other report, made by this oracle', () => {
-      const newMedianRate = 12
-      const newExpectedMedianRate = expectedMedianRateFromGiven(newMedianRate, denominator)
+      const newExpectedMedianRate = toFixed(new BigNumber(12).dividedBy(new BigNumber(1)))


ditto toFixed(12)

asaj · 2020-01-24T19:58:24Z

packages/protocol/test/stability/sortedoracles.ts

-      const anOracleExpectedMedianRate1 = expectedMedianRateFromGiven(
-        anOracleMedianRate1,
-        denominator
+      const anOracleExpectedMedianRate1 = toFixed(


Are these variables necessary anymore?

asaj · 2020-02-23T23:42:11Z

Closing as there are open PRs that address these issues individually.

added changes to the 3 tickets

493aed6

BatiGencho requested review from asaj and m-chrzan as code owners January 6, 2020 14:51

BatiGencho requested a review from mcortesi January 6, 2020 14:52

jfoutts-celo self-assigned this Jan 7, 2020

jfoutts-celo reviewed Jan 7, 2020

View reviewed changes

jfoutts-celo assigned BatiGencho and unassigned jfoutts-celo Jan 7, 2020

fixes acc. to PR comments

b090978

BatiGencho assigned jfoutts-celo Jan 9, 2020

mcortesi reviewed Jan 9, 2020

View reviewed changes

mcortesi requested changes Jan 9, 2020

View reviewed changes

Ventsislav Tsochev and others added 5 commits January 14, 2020 15:16

MultiSig -> simplify initialization with addOwner and changeRequirement

62cca9b

MultiSig -> simplify initialization with addOwner and changeRequireme…

735755f

…nt naming fixes, EpochRewards -> few setters switched to privet, Governance -> setLastDequeue() switched to private, StableToken -> setters for name, symbol, and decimals removed

Merge remote-tracking branch 'origin/limechain/t_1604_1597_1365' into…

144c8d2

… limechain/t_1604_1597_1365 # Conflicts: # packages/protocol/contracts/common/MultiSig.sol

DENOMINATOR / getDenominator removed,

b5f4935

numerator, denominator replaced with medianValue, unit test update

fixed numerator/denominatpr in soredoracles kit

8c6c65a

BatiGencho assigned asaj Jan 17, 2020

factor can be update after 18 months

978ae1e

asaj reviewed Jan 22, 2020

View reviewed changes

asaj removed their assignment Jan 22, 2020

Ventsislav Tsochev added 3 commits January 22, 2020 13:50

MultiSig -> removed unused event, 08_stabletoken.ts and set_exchange_…

22071bd

…rate.ts the median rate is multiplied by 10^24 (toFixed), sortedoracles.ts (unittests) numerator / denomerator replaced with rate

GasPriceMinimum namings -> rateNumerator to rate,

8028b05

sortedoracles unit tests small assertion correction

getInternalDenominator removed from SortedOracles.ts wrapper

c833e0c

BatiGencho assigned asaj Jan 24, 2020

asaj reviewed Jan 24, 2020

View reviewed changes

asaj unassigned asaj and jfoutts-celo Jan 24, 2020

unnecessary bigNumber parse removed

661043b

asaj assigned asaj and unassigned BatiGencho Feb 22, 2020

asaj added the audit label Feb 22, 2020

asaj closed this Feb 23, 2020

Implementation of tickets 1604, 1597 and 1365 #2368

Implementation of tickets 1604, 1597 and 1365 #2368

Conversation

BatiGencho commented Jan 6, 2020 • edited Loading

Description

Related issues

Note

jfoutts-celo Jan 7, 2020 • edited Loading

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

mcortesi 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

mcortesi 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

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

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

asaj commented Feb 23, 2020

BatiGencho commented Jan 6, 2020 •

edited

Loading

jfoutts-celo Jan 7, 2020 •

edited

Loading