소스 검색

Refactor Time library to use valueBefore/valueAfter (#4555)

Co-authored-by: Francisco <fg@frang.io>
Hadrien Croubois 2 년 전
부모
커밋
87f7a2cd42

+ 2 - 2
contracts/access/manager/AccessManager.sol

@@ -60,7 +60,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
     // Structure that stores the details for a group/account pair. This structure fits into a single slot.
     struct Access {
         // Timepoint at which the user gets the permission. If this is either 0, or in the future, the group permission
-        // is not available. Should be checked using {Time-isSetAndPast}
+        // is not available.
         uint48 since;
         // delay for execution. Only applies to restricted() / relay() calls. This does not restrict access to
         // functions that use the `onlyGroup` modifier.
@@ -235,7 +235,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
             return (true, 0);
         } else {
             (uint48 inGroupSince, uint32 currentDelay, , ) = getAccess(groupId, account);
-            return (inGroupSince.isSetAndPast(Time.timestamp()), currentDelay);
+            return (inGroupSince != 0 && inGroupSince <= Time.timestamp(), currentDelay);
         }
     }
 

+ 2 - 1
contracts/governance/extensions/GovernorVotes.sol

@@ -7,6 +7,7 @@ import {Governor} from "../Governor.sol";
 import {IVotes} from "../utils/IVotes.sol";
 import {IERC5805} from "../../interfaces/IERC5805.sol";
 import {SafeCast} from "../../utils/math/SafeCast.sol";
+import {Time} from "../../utils/types/Time.sol";
 
 /**
  * @dev Extension of {Governor} for voting weight extraction from an {ERC20Votes} token, or since v4.5 an {ERC721Votes} token.
@@ -26,7 +27,7 @@ abstract contract GovernorVotes is Governor {
         try token.clock() returns (uint48 timepoint) {
             return timepoint;
         } catch {
-            return SafeCast.toUint48(block.number);
+            return Time.blockNumber();
         }
     }
 

+ 3 - 2
contracts/governance/utils/Votes.sol

@@ -9,6 +9,7 @@ import {EIP712} from "../../utils/cryptography/EIP712.sol";
 import {Checkpoints} from "../../utils/structs/Checkpoints.sol";
 import {SafeCast} from "../../utils/math/SafeCast.sol";
 import {ECDSA} from "../../utils/cryptography/ECDSA.sol";
+import {Time} from "../../utils/types/Time.sol";
 
 /**
  * @dev This is a base abstract contract that tracks voting units, which are a measure of voting power that can be
@@ -55,7 +56,7 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 {
      * checkpoints (and voting), in which case {CLOCK_MODE} should be overridden as well to match.
      */
     function clock() public view virtual returns (uint48) {
-        return SafeCast.toUint48(block.number);
+        return Time.blockNumber();
     }
 
     /**
@@ -64,7 +65,7 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 {
     // solhint-disable-next-line func-name-mixedcase
     function CLOCK_MODE() public view virtual returns (string memory) {
         // Check that the clock was not modified
-        if (clock() != block.number) {
+        if (clock() != Time.blockNumber()) {
             revert ERC6372InconsistentClock();
         }
         return "mode=blocknumber&from=default";

+ 15 - 28
contracts/utils/types/Time.sol

@@ -33,13 +33,6 @@ library Time {
         return SafeCast.toUint48(block.number);
     }
 
-    /**
-     * @dev Check if a timepoint is set, and in the past.
-     */
-    function isSetAndPast(uint48 timepoint, uint48 ref) internal pure returns (bool) {
-        return timepoint != 0 && timepoint <= ref;
-    }
-
     // ==================================================== Delay =====================================================
     /**
      * @dev A `Delay` is a uint32 duration that can be programmed to change value automatically at a given point in the
@@ -52,12 +45,12 @@ library Time {
      * still apply for some time.
      *
      *
-     * The `Delay` type is 128 bits long, and packs the following:
+     * The `Delay` type is 112 bits long, and packs the following:
      *
      * ```
      *   | [uint48]: effect date (timepoint)
-     *   |           | [uint32]: current value (duration)
-     *   ↓           ↓       ↓ [uint32]: pending value (duration)
+     *   |           | [uint32]: value before (duration)
+     *   ↓           ↓       ↓ [uint32]: value after (duration)
      * 0xAAAAAAAAAAAABBBBBBBBCCCCCCCC
      * ```
      *
@@ -78,8 +71,8 @@ library Time {
      * change after this timepoint. If the effect timepoint is 0, then the pending value should not be considered.
      */
     function getFullAt(Delay self, uint48 timepoint) internal pure returns (uint32, uint32, uint48) {
-        (uint32 oldValue, uint32 newValue, uint48 effect) = self.unpack();
-        return effect.isSetAndPast(timepoint) ? (newValue, 0, 0) : (oldValue, newValue, effect);
+        (uint32 valueBefore, uint32 valueAfter, uint48 effect) = self.unpack();
+        return effect <= timepoint ? (valueAfter, 0, 0) : (valueBefore, valueAfter, effect);
     }
 
     /**
@@ -105,13 +98,6 @@ library Time {
         return self.getAt(timestamp());
     }
 
-    /**
-     * @dev Update a Delay object so that a new duration takes effect at a given timepoint.
-     */
-    function withUpdateAt(Delay self, uint32 newValue, uint48 effect) internal view returns (Delay) {
-        return pack(self.get(), newValue, effect);
-    }
-
     /**
      * @dev Update a Delay object so that it takes a new duration after a timepoint that is automatically computed to
      * enforce the old delay at the moment of the update. Returns the updated Delay object and the timestamp when the
@@ -121,25 +107,26 @@ library Time {
         uint32 value = self.get();
         uint32 setback = uint32(Math.max(minSetback, value > newValue ? value - newValue : 0));
         uint48 effect = timestamp() + setback;
-        return (self.withUpdateAt(newValue, effect), effect);
+        return (pack(value, newValue, effect), effect);
     }
 
     /**
-     * @dev Split a delay into its components: oldValue, newValue and effect (transition timepoint).
+     * @dev Split a delay into its components: valueBefore, valueAfter and effect (transition timepoint).
      */
     function unpack(Delay self) internal pure returns (uint32, uint32, uint48) {
         uint112 raw = Delay.unwrap(self);
-        return (
-            uint32(raw), // oldValue
-            uint32(raw >> 32), // newValue
-            uint48(raw >> 64) // effect
-        );
+
+        uint32 valueAfter = uint32(raw);
+        uint32 valueBefore = uint32(raw >> 32);
+        uint48 effect = uint48(raw >> 64);
+
+        return (valueBefore, valueAfter, effect);
     }
 
     /**
      * @dev pack the components into a Delay object.
      */
-    function pack(uint32 oldValue, uint32 newValue, uint48 effect) internal pure returns (Delay) {
-        return Delay.wrap(uint112(oldValue) | (uint112(newValue) << 32) | (uint112(effect) << 64));
+    function pack(uint32 valueBefore, uint32 valueAfter, uint48 effect) internal pure returns (Delay) {
+        return Delay.wrap((uint112(effect) << 64) | (uint112(valueBefore) << 32) | uint112(valueAfter));
     }
 }

+ 1 - 2
test/access/manager/AccessManager.test.js

@@ -3,6 +3,7 @@ const { constants, expectEvent, time } = require('@openzeppelin/test-helpers');
 const { expectRevertCustomError } = require('../../helpers/customError');
 const { selector } = require('../../helpers/methods');
 const { clockFromReceipt } = require('../../helpers/time');
+const { product } = require('../../helpers/iterate');
 
 const AccessManager = artifacts.require('$AccessManager');
 const AccessManagedTarget = artifacts.require('$AccessManagedTarget');
@@ -620,8 +621,6 @@ contract('AccessManager', function (accounts) {
 
     // WIP
     describe('Calling restricted & unrestricted functions', function () {
-      const product = (...arrays) => arrays.reduce((a, b) => a.flatMap(ai => b.map(bi => [...ai, bi])), [[]]);
-
       for (const [callerGroups, fnGroup, closed, delay] of product(
         [[], [GROUPS.SOME]],
         [undefined, GROUPS.ADMIN, GROUPS.SOME, GROUPS.PUBLIC],

+ 16 - 0
test/helpers/iterate.js

@@ -0,0 +1,16 @@
+// Map values in an object
+const mapValues = (obj, fn) => Object.fromEntries(Object.entries(obj).map(([k, v]) => [k, fn(v)]));
+
+// Array of number or bigint
+const max = (...values) => values.slice(1).reduce((x, y) => (x > y ? x : y), values[0]);
+const min = (...values) => values.slice(1).reduce((x, y) => (x < y ? x : y), values[0]);
+
+// Cartesian product of a list of arrays
+const product = (...arrays) => arrays.reduce((a, b) => a.flatMap(ai => b.map(bi => [...ai, bi])), [[]]);
+
+module.exports = {
+  mapValues,
+  max,
+  min,
+  product,
+};

+ 0 - 7
test/helpers/map-values.js

@@ -1,7 +0,0 @@
-function mapValues(obj, fn) {
-  return Object.fromEntries(Object.entries(obj).map(([k, v]) => [k, fn(v)]));
-}
-
-module.exports = {
-  mapValues,
-};

+ 1 - 1
test/utils/cryptography/EIP712.test.js

@@ -3,7 +3,7 @@ const Wallet = require('ethereumjs-wallet').default;
 
 const { getDomain, domainType, domainSeparator, hashTypedData } = require('../../helpers/eip712');
 const { getChainId } = require('../../helpers/chainid');
-const { mapValues } = require('../../helpers/map-values');
+const { mapValues } = require('../../helpers/iterate');
 
 const EIP712Verifier = artifacts.require('$EIP712Verifier');
 const Clones = artifacts.require('$Clones');

+ 1 - 1
test/utils/structs/EnumerableMap.test.js

@@ -1,5 +1,5 @@
 const { BN, constants } = require('@openzeppelin/test-helpers');
-const { mapValues } = require('../../helpers/map-values');
+const { mapValues } = require('../../helpers/iterate');
 
 const EnumerableMap = artifacts.require('$EnumerableMap');
 

+ 1 - 1
test/utils/structs/EnumerableSet.test.js

@@ -1,5 +1,5 @@
 const EnumerableSet = artifacts.require('$EnumerableSet');
-const { mapValues } = require('../../helpers/map-values');
+const { mapValues } = require('../../helpers/iterate');
 
 const { shouldBehaveLikeSet } = require('./EnumerableSet.behavior');
 

+ 161 - 0
test/utils/types/Time.test.js

@@ -0,0 +1,161 @@
+require('@openzeppelin/test-helpers');
+
+const { expect } = require('chai');
+const { clock } = require('../../helpers/time');
+const { product, max } = require('../../helpers/iterate');
+
+const Time = artifacts.require('$Time');
+
+const MAX_UINT32 = 1n << (32n - 1n);
+const MAX_UINT48 = 1n << (48n - 1n);
+const SOME_VALUES = [0n, 1n, 2n, 15n, 16n, 17n, 42n];
+
+const asUint = (value, size) => {
+  if (typeof value != 'bigint') {
+    value = BigInt(value);
+  }
+  // chai does not support bigint :/
+  if (value < 0 || value >= 1n << BigInt(size)) {
+    throw new Error(`value is not a valid uint${size}`);
+  }
+  return value;
+};
+
+const unpackDelay = delay => ({
+  valueBefore: (asUint(delay, 112) >> 32n) % (1n << 32n),
+  valueAfter: (asUint(delay, 112) >> 0n) % (1n << 32n),
+  effect: (asUint(delay, 112) >> 64n) % (1n << 48n),
+});
+
+const packDelay = ({ valueBefore, valueAfter = 0n, effect = 0n }) =>
+  (asUint(valueAfter, 32) << 0n) + (asUint(valueBefore, 32) << 32n) + (asUint(effect, 48) << 64n);
+
+const effectSamplesForTimepoint = timepoint => [
+  0n,
+  timepoint,
+  ...product([-1n, 1n], [1n, 2n, 17n, 42n])
+    .map(([sign, shift]) => timepoint + sign * shift)
+    .filter(effect => effect > 0n && effect <= MAX_UINT48),
+  MAX_UINT48,
+];
+
+contract('Time', function () {
+  beforeEach(async function () {
+    this.mock = await Time.new();
+  });
+
+  describe('clocks', function () {
+    it('timestamp', async function () {
+      expect(await this.mock.$timestamp()).to.be.bignumber.equal(web3.utils.toBN(await clock.timestamp()));
+    });
+
+    it('block number', async function () {
+      expect(await this.mock.$blockNumber()).to.be.bignumber.equal(web3.utils.toBN(await clock.blocknumber()));
+    });
+  });
+
+  describe('Delay', function () {
+    describe('packing and unpacking', function () {
+      const valueBefore = 17n;
+      const valueAfter = 42n;
+      const effect = 69n;
+      const delay = 1272825341158973505578n;
+
+      it('pack', async function () {
+        const packed = await this.mock.$pack(valueBefore, valueAfter, effect);
+        expect(packed).to.be.bignumber.equal(delay.toString());
+
+        const packed2 = packDelay({ valueBefore, valueAfter, effect });
+        expect(packed2).to.be.equal(delay);
+      });
+
+      it('unpack', async function () {
+        const unpacked = await this.mock.$unpack(delay);
+        expect(unpacked[0]).to.be.bignumber.equal(valueBefore.toString());
+        expect(unpacked[1]).to.be.bignumber.equal(valueAfter.toString());
+        expect(unpacked[2]).to.be.bignumber.equal(effect.toString());
+
+        const unpacked2 = unpackDelay(delay);
+        expect(unpacked2).to.be.deep.equal({ valueBefore, valueAfter, effect });
+      });
+    });
+
+    it('toDelay', async function () {
+      for (const value of [...SOME_VALUES, MAX_UINT32]) {
+        const delay = await this.mock.$toDelay(value).then(unpackDelay);
+        expect(delay).to.be.deep.equal({ valueBefore: 0n, valueAfter: value, effect: 0n });
+      }
+    });
+
+    it('getAt & getFullAt', async function () {
+      const valueBefore = 24194n;
+      const valueAfter = 4214143n;
+
+      for (const timepoint of [...SOME_VALUES, MAX_UINT48])
+        for (const effect of effectSamplesForTimepoint(timepoint)) {
+          const isPast = effect <= timepoint;
+
+          const delay = packDelay({ valueBefore, valueAfter, effect });
+
+          expect(await this.mock.$getAt(delay, timepoint)).to.be.bignumber.equal(
+            String(isPast ? valueAfter : valueBefore),
+          );
+
+          const getFullAt = await this.mock.$getFullAt(delay, timepoint);
+          expect(getFullAt[0]).to.be.bignumber.equal(String(isPast ? valueAfter : valueBefore));
+          expect(getFullAt[1]).to.be.bignumber.equal(String(isPast ? 0n : valueAfter));
+          expect(getFullAt[2]).to.be.bignumber.equal(String(isPast ? 0n : effect));
+        }
+    });
+
+    it('get & getFull', async function () {
+      const timepoint = await clock.timestamp().then(BigInt);
+      const valueBefore = 24194n;
+      const valueAfter = 4214143n;
+
+      for (const effect of effectSamplesForTimepoint(timepoint)) {
+        const isPast = effect <= timepoint;
+
+        const delay = packDelay({ valueBefore, valueAfter, effect });
+
+        expect(await this.mock.$get(delay)).to.be.bignumber.equal(String(isPast ? valueAfter : valueBefore));
+
+        const result = await this.mock.$getFull(delay);
+        expect(result[0]).to.be.bignumber.equal(String(isPast ? valueAfter : valueBefore));
+        expect(result[1]).to.be.bignumber.equal(String(isPast ? 0n : valueAfter));
+        expect(result[2]).to.be.bignumber.equal(String(isPast ? 0n : effect));
+      }
+    });
+
+    it('withUpdate', async function () {
+      const timepoint = await clock.timestamp().then(BigInt);
+      const valueBefore = 24194n;
+      const valueAfter = 4214143n;
+      const newvalueAfter = 94716n;
+
+      for (const effect of effectSamplesForTimepoint(timepoint))
+        for (const minSetback of [...SOME_VALUES, MAX_UINT32]) {
+          const isPast = effect <= timepoint;
+          const expectedvalueBefore = isPast ? valueAfter : valueBefore;
+          const expectedSetback = max(minSetback, expectedvalueBefore - newvalueAfter, 0n);
+
+          const result = await this.mock.$withUpdate(
+            packDelay({ valueBefore, valueAfter, effect }),
+            newvalueAfter,
+            minSetback,
+          );
+
+          expect(result[0]).to.be.bignumber.equal(
+            String(
+              packDelay({
+                valueBefore: expectedvalueBefore,
+                valueAfter: newvalueAfter,
+                effect: timepoint + expectedSetback,
+              }),
+            ),
+          );
+          expect(result[1]).to.be.bignumber.equal(String(timepoint + expectedSetback));
+        }
+    });
+  });
+});