From 87f7a2cd4258bc668e55f8bd091862f5b5d9ec91 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 6 Sep 2023 04:19:21 +0200 Subject: [PATCH] Refactor Time library to use valueBefore/valueAfter (#4555) Co-authored-by: Francisco --- contracts/access/manager/AccessManager.sol | 4 +- .../governance/extensions/GovernorVotes.sol | 3 +- contracts/governance/utils/Votes.sol | 5 +- contracts/utils/types/Time.sol | 43 ++--- test/access/manager/AccessManager.test.js | 3 +- test/helpers/iterate.js | 16 ++ test/helpers/map-values.js | 7 - test/utils/cryptography/EIP712.test.js | 2 +- test/utils/structs/EnumerableMap.test.js | 2 +- test/utils/structs/EnumerableSet.test.js | 2 +- test/utils/types/Time.test.js | 161 ++++++++++++++++++ 11 files changed, 203 insertions(+), 45 deletions(-) create mode 100644 test/helpers/iterate.js delete mode 100644 test/helpers/map-values.js create mode 100644 test/utils/types/Time.test.js diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 66bca0890..be438536e 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/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); } } diff --git a/contracts/governance/extensions/GovernorVotes.sol b/contracts/governance/extensions/GovernorVotes.sol index 45447da51..d7be2ebc2 100644 --- a/contracts/governance/extensions/GovernorVotes.sol +++ b/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(); } } diff --git a/contracts/governance/utils/Votes.sol b/contracts/governance/utils/Votes.sol index d9f5e01bb..ad1074d77 100644 --- a/contracts/governance/utils/Votes.sol +++ b/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"; diff --git a/contracts/utils/types/Time.sol b/contracts/utils/types/Time.sol index 5e8a2d9e0..13bb0add5 100644 --- a/contracts/utils/types/Time.sol +++ b/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)); } } diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index dcf285c77..c157d6b7e 100644 --- a/test/access/manager/AccessManager.test.js +++ b/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], diff --git a/test/helpers/iterate.js b/test/helpers/iterate.js new file mode 100644 index 000000000..7f6e0e678 --- /dev/null +++ b/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, +}; diff --git a/test/helpers/map-values.js b/test/helpers/map-values.js deleted file mode 100644 index 84d95fc65..000000000 --- a/test/helpers/map-values.js +++ /dev/null @@ -1,7 +0,0 @@ -function mapValues(obj, fn) { - return Object.fromEntries(Object.entries(obj).map(([k, v]) => [k, fn(v)])); -} - -module.exports = { - mapValues, -}; diff --git a/test/utils/cryptography/EIP712.test.js b/test/utils/cryptography/EIP712.test.js index 7ea535b7f..faf01f1a3 100644 --- a/test/utils/cryptography/EIP712.test.js +++ b/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'); diff --git a/test/utils/structs/EnumerableMap.test.js b/test/utils/structs/EnumerableMap.test.js index 8861fa731..545e12a4f 100644 --- a/test/utils/structs/EnumerableMap.test.js +++ b/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'); diff --git a/test/utils/structs/EnumerableSet.test.js b/test/utils/structs/EnumerableSet.test.js index 3ba9d7ff7..a1840257b 100644 --- a/test/utils/structs/EnumerableSet.test.js +++ b/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'); diff --git a/test/utils/types/Time.test.js b/test/utils/types/Time.test.js new file mode 100644 index 000000000..b246f7b92 --- /dev/null +++ b/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)); + } + }); + }); +});