diff --git a/.changeset/smooth-cougars-jump.md b/.changeset/smooth-cougars-jump.md new file mode 100644 index 000000000..337101cd0 --- /dev/null +++ b/.changeset/smooth-cougars-jump.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`ReentrancyGuard`, `Pausable`: Moved to `utils` directory. diff --git a/.changeset/wild-peas-remain.md b/.changeset/wild-peas-remain.md new file mode 100644 index 000000000..83b4bb307 --- /dev/null +++ b/.changeset/wild-peas-remain.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': major +--- + +`Votes`: Use Trace208 for checkpoints. This enables EIP-6372 clock support for keys but reduces the max supported voting power to uint208. diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 41719b142..e8a82f13a 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -65,6 +65,7 @@ jobs: - name: Check storage layout uses: ./.github/actions/storage-layout if: github.base_ref == 'master' + continue-on-error: ${{ contains(github.event.pull_request.labels.*.name, 'breaking change') }} with: token: ${{ github.token }} diff --git a/GUIDELINES.md b/GUIDELINES.md index 71f166405..4b7f5e76e 100644 --- a/GUIDELINES.md +++ b/GUIDELINES.md @@ -95,8 +95,18 @@ In addition to the official Solidity Style Guide we have a number of other conve } ``` -* Events should be emitted immediately after the state change that they - represent, and should be named in the past tense. +* Functions should be declared virtual, with few exceptions listed below. The + contract logic should be written considering that these functions may be + overridden by developers, e.g. getting a value using an internal getter rather + than reading directly from a state variable. + + If function A is an "alias" of function B, i.e. it invokes function B without + significant additional logic, then function A should not be virtual so that + any user overrides are implemented on B, preventing inconsistencies. + +* Events should generally be emitted immediately after the state change that they + represent, and should be named in the past tense. Some exceptions may be made for gas + efficiency if the result doesn't affect observable ordering of events. ```solidity function _burn(address who, uint256 value) internal { diff --git a/README.md b/README.md index 825c6ea7d..11ee615bb 100644 --- a/README.md +++ b/README.md @@ -72,7 +72,7 @@ The guides in the [documentation site](https://docs.openzeppelin.com/contracts) The [full API](https://docs.openzeppelin.com/contracts/api/token/ERC20) is also thoroughly documented, and serves as a great reference when developing your smart contract application. You can also ask for help or follow Contracts's development in the [community forum](https://forum.openzeppelin.com). -Finally, you may want to take a look at the [guides on our blog](https://blog.openzeppelin.com/guides), which cover several common use cases and good practices. The following articles provide great background reading, though please note that some of the referenced tools have changed, as the tooling in the ecosystem continues to rapidly evolve. +Finally, you may want to take a look at the [guides on our blog](https://blog.openzeppelin.com/), which cover several common use cases and good practices. The following articles provide great background reading, though please note that some of the referenced tools have changed, as the tooling in the ecosystem continues to rapidly evolve. * [The Hitchhiker’s Guide to Smart Contracts in Ethereum](https://blog.openzeppelin.com/the-hitchhikers-guide-to-smart-contracts-in-ethereum-848f08001f05) will help you get an overview of the various tools available for smart contract development, and help you set up your environment. * [A Gentle Introduction to Ethereum Programming, Part 1](https://blog.openzeppelin.com/a-gentle-introduction-to-ethereum-programming-part-1-783cc7796094) provides very useful information on an introductory level, including many basic concepts from the Ethereum platform. diff --git a/certora/harnesses/PausableHarness.sol b/certora/harnesses/PausableHarness.sol index b9c15cf54..12f946709 100644 --- a/certora/harnesses/PausableHarness.sol +++ b/certora/harnesses/PausableHarness.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.20; -import "../patched/security/Pausable.sol"; +import "../patched/utils/Pausable.sol"; contract PausableHarness is Pausable { function pause() external { diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index f5b22b4b3..62bee7de3 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -43,7 +43,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 uint48 eta; } - bytes32 private constant _ALL_PROPOSAL_STATES_BITMAP = bytes32((2 ** (uint8(type(ProposalState).max) + 1)) - 1); + bytes32 private constant ALL_PROPOSAL_STATES_BITMAP = bytes32((2 ** (uint8(type(ProposalState).max) + 1)) - 1); string private _name; mapping(uint256 proposalId => ProposalCore) private _proposals; @@ -486,7 +486,7 @@ abstract contract Governor is Context, ERC165, EIP712, Nonces, IGovernor, IERC72 _validateStateBitmap( proposalId, - _ALL_PROPOSAL_STATES_BITMAP ^ + ALL_PROPOSAL_STATES_BITMAP ^ _encodeStateBitmap(ProposalState.Canceled) ^ _encodeStateBitmap(ProposalState.Expired) ^ _encodeStateBitmap(ProposalState.Executed) diff --git a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol index f7def8529..cc2e85b45 100644 --- a/contracts/governance/extensions/GovernorVotesQuorumFraction.sol +++ b/contracts/governance/extensions/GovernorVotesQuorumFraction.sol @@ -12,9 +12,9 @@ import {Checkpoints} from "../../utils/structs/Checkpoints.sol"; * fraction of the total supply. */ abstract contract GovernorVotesQuorumFraction is GovernorVotes { - using Checkpoints for Checkpoints.Trace224; + using Checkpoints for Checkpoints.Trace208; - Checkpoints.Trace224 private _quorumNumeratorHistory; + Checkpoints.Trace208 private _quorumNumeratorHistory; event QuorumNumeratorUpdated(uint256 oldQuorumNumerator, uint256 newQuorumNumerator); @@ -49,13 +49,15 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { uint256 length = _quorumNumeratorHistory._checkpoints.length; // Optimistic search, check the latest checkpoint - Checkpoints.Checkpoint224 memory latest = _quorumNumeratorHistory._checkpoints[length - 1]; - if (latest._key <= timepoint) { - return latest._value; + Checkpoints.Checkpoint208 storage latest = _quorumNumeratorHistory._checkpoints[length - 1]; + uint48 latestKey = latest._key; + uint208 latestValue = latest._value; + if (latestKey <= timepoint) { + return latestValue; } // Otherwise, do the binary search - return _quorumNumeratorHistory.upperLookupRecent(SafeCast.toUint32(timepoint)); + return _quorumNumeratorHistory.upperLookupRecent(SafeCast.toUint48(timepoint)); } /** @@ -102,7 +104,7 @@ abstract contract GovernorVotesQuorumFraction is GovernorVotes { } uint256 oldQuorumNumerator = quorumNumerator(); - _quorumNumeratorHistory.push(SafeCast.toUint32(clock()), SafeCast.toUint224(newQuorumNumerator)); + _quorumNumeratorHistory.push(clock(), SafeCast.toUint208(newQuorumNumerator)); emit QuorumNumeratorUpdated(oldQuorumNumerator, newQuorumNumerator); } diff --git a/contracts/governance/utils/Votes.sol b/contracts/governance/utils/Votes.sol index 96d89cb8e..d9f5e01bb 100644 --- a/contracts/governance/utils/Votes.sol +++ b/contracts/governance/utils/Votes.sol @@ -29,16 +29,16 @@ import {ECDSA} from "../../utils/cryptography/ECDSA.sol"; * previous example, it would be included in {ERC721-_beforeTokenTransfer}). */ abstract contract Votes is Context, EIP712, Nonces, IERC5805 { - using Checkpoints for Checkpoints.Trace224; + using Checkpoints for Checkpoints.Trace208; - bytes32 private constant _DELEGATION_TYPEHASH = + bytes32 private constant DELEGATION_TYPEHASH = keccak256("Delegation(address delegatee,uint256 nonce,uint256 expiry)"); mapping(address account => address) private _delegatee; - mapping(address delegatee => Checkpoints.Trace224) private _delegateCheckpoints; + mapping(address delegatee => Checkpoints.Trace208) private _delegateCheckpoints; - Checkpoints.Trace224 private _totalCheckpoints; + Checkpoints.Trace208 private _totalCheckpoints; /** * @dev The clock was incorrectly modified. @@ -90,7 +90,7 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 { if (timepoint >= currentTimepoint) { revert ERC5805FutureLookup(timepoint, currentTimepoint); } - return _delegateCheckpoints[account].upperLookupRecent(SafeCast.toUint32(timepoint)); + return _delegateCheckpoints[account].upperLookupRecent(SafeCast.toUint48(timepoint)); } /** @@ -110,7 +110,7 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 { if (timepoint >= currentTimepoint) { revert ERC5805FutureLookup(timepoint, currentTimepoint); } - return _totalCheckpoints.upperLookupRecent(SafeCast.toUint32(timepoint)); + return _totalCheckpoints.upperLookupRecent(SafeCast.toUint48(timepoint)); } /** @@ -150,7 +150,7 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 { revert VotesExpiredSignature(expiry); } address signer = ECDSA.recover( - _hashTypedDataV4(keccak256(abi.encode(_DELEGATION_TYPEHASH, delegatee, nonce, expiry))), + _hashTypedDataV4(keccak256(abi.encode(DELEGATION_TYPEHASH, delegatee, nonce, expiry))), v, r, s @@ -178,10 +178,10 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 { */ function _transferVotingUnits(address from, address to, uint256 amount) internal virtual { if (from == address(0)) { - _push(_totalCheckpoints, _add, SafeCast.toUint224(amount)); + _push(_totalCheckpoints, _add, SafeCast.toUint208(amount)); } if (to == address(0)) { - _push(_totalCheckpoints, _subtract, SafeCast.toUint224(amount)); + _push(_totalCheckpoints, _subtract, SafeCast.toUint208(amount)); } _moveDelegateVotes(delegates(from), delegates(to), amount); } @@ -195,7 +195,7 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 { (uint256 oldValue, uint256 newValue) = _push( _delegateCheckpoints[from], _subtract, - SafeCast.toUint224(amount) + SafeCast.toUint208(amount) ); emit DelegateVotesChanged(from, oldValue, newValue); } @@ -203,7 +203,7 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 { (uint256 oldValue, uint256 newValue) = _push( _delegateCheckpoints[to], _add, - SafeCast.toUint224(amount) + SafeCast.toUint208(amount) ); emit DelegateVotesChanged(to, oldValue, newValue); } @@ -223,23 +223,23 @@ abstract contract Votes is Context, EIP712, Nonces, IERC5805 { function _checkpoints( address account, uint32 pos - ) internal view virtual returns (Checkpoints.Checkpoint224 memory) { + ) internal view virtual returns (Checkpoints.Checkpoint208 memory) { return _delegateCheckpoints[account].at(pos); } function _push( - Checkpoints.Trace224 storage store, - function(uint224, uint224) view returns (uint224) op, - uint224 delta - ) private returns (uint224, uint224) { - return store.push(SafeCast.toUint32(clock()), op(store.latest(), delta)); + Checkpoints.Trace208 storage store, + function(uint208, uint208) view returns (uint208) op, + uint208 delta + ) private returns (uint208, uint208) { + return store.push(clock(), op(store.latest(), delta)); } - function _add(uint224 a, uint224 b) private pure returns (uint224) { + function _add(uint208 a, uint208 b) private pure returns (uint208) { return a + b; } - function _subtract(uint224 a, uint224 b) private pure returns (uint224) { + function _subtract(uint208 a, uint208 b) private pure returns (uint208) { return a - b; } diff --git a/contracts/mocks/PausableMock.sol b/contracts/mocks/PausableMock.sol index abe50c6c9..fa701e2c7 100644 --- a/contracts/mocks/PausableMock.sol +++ b/contracts/mocks/PausableMock.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.20; -import {Pausable} from "../security/Pausable.sol"; +import {Pausable} from "../utils/Pausable.sol"; contract PausableMock is Pausable { bool public drasticMeasureTaken; diff --git a/contracts/mocks/ReentrancyMock.sol b/contracts/mocks/ReentrancyMock.sol index f275c88e2..39e2d5ed8 100644 --- a/contracts/mocks/ReentrancyMock.sol +++ b/contracts/mocks/ReentrancyMock.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.20; -import {ReentrancyGuard} from "../security/ReentrancyGuard.sol"; +import {ReentrancyGuard} from "../utils/ReentrancyGuard.sol"; import {ReentrancyAttack} from "./ReentrancyAttack.sol"; contract ReentrancyMock is ReentrancyGuard { diff --git a/contracts/proxy/utils/Initializable.sol b/contracts/proxy/utils/Initializable.sol index 7f33a6267..cc6d9c962 100644 --- a/contracts/proxy/utils/Initializable.sol +++ b/contracts/proxy/utils/Initializable.sol @@ -73,9 +73,8 @@ abstract contract Initializable { bool _initializing; } - // keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.Initializable")) - 1)) - bytes32 private constant _INITIALIZABLE_STORAGE = - 0xf0c57e16840df040f15088dc2f81fe391c3923bec73e23a9662efc9c229c6a0e; + // keccak256(abi.encode(uint256(keccak256("openzeppelin.storage.Initializable")) - 1)) & ~bytes32(uint256(0xff)) + bytes32 private constant INITIALIZABLE_STORAGE = 0xf0c57e16840df040f15088dc2f81fe391c3923bec73e23a9662efc9c229c6a00; /** * @dev The contract is already initialized. @@ -106,7 +105,8 @@ abstract contract Initializable { InitializableStorage storage $ = _getInitializableStorage(); bool isTopLevelCall = !$._initializing; - if (!(isTopLevelCall && $._initialized < 1) && !(address(this).code.length == 0 && $._initialized == 1)) { + uint64 initialized = $._initialized; + if (!(isTopLevelCall && initialized < 1) && !(address(this).code.length == 0 && initialized == 1)) { revert AlreadyInitialized(); } $._initialized = 1; @@ -211,7 +211,7 @@ abstract contract Initializable { // solhint-disable-next-line var-name-mixedcase function _getInitializableStorage() private pure returns (InitializableStorage storage $) { assembly { - $.slot := _INITIALIZABLE_STORAGE + $.slot := INITIALIZABLE_STORAGE } } } diff --git a/contracts/security/README.adoc b/contracts/security/README.adoc deleted file mode 100644 index 7f4799eb8..000000000 --- a/contracts/security/README.adoc +++ /dev/null @@ -1,17 +0,0 @@ -= Security - -[.readme-notice] -NOTE: This document is better viewed at https://docs.openzeppelin.com/contracts/api/security - -These contracts aim to cover common security practices. - -* {ReentrancyGuard}: A modifier that can prevent reentrancy during certain functions. -* {Pausable}: A common emergency response mechanism that can pause functionality while a remediation is pending. - -TIP: For an overview on reentrancy and the possible mechanisms to prevent it, read our article https://blog.openzeppelin.com/reentrancy-after-istanbul/[Reentrancy After Istanbul]. - -== Contracts - -{{ReentrancyGuard}} - -{{Pausable}} diff --git a/contracts/token/ERC1155/extensions/ERC1155Pausable.sol b/contracts/token/ERC1155/extensions/ERC1155Pausable.sol index 914420ccc..96f2400f8 100644 --- a/contracts/token/ERC1155/extensions/ERC1155Pausable.sol +++ b/contracts/token/ERC1155/extensions/ERC1155Pausable.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.20; import {ERC1155} from "../ERC1155.sol"; -import {Pausable} from "../../../security/Pausable.sol"; +import {Pausable} from "../../../utils/Pausable.sol"; /** * @dev ERC1155 token with pausable token transfers, minting and burning. diff --git a/contracts/token/ERC20/ERC20.sol b/contracts/token/ERC20/ERC20.sol index e8212e92b..343881ff3 100644 --- a/contracts/token/ERC20/ERC20.sol +++ b/contracts/token/ERC20/ERC20.sol @@ -313,13 +313,15 @@ abstract contract ERC20 is Context, IERC20, IERC20Metadata, IERC20Errors { * * - `owner` cannot be the zero address. * - `spender` cannot be the zero address. + * + * Overrides to this logic should be done to the variant with an additional `bool emitEvent` argument. */ - function _approve(address owner, address spender, uint256 value) internal virtual { + function _approve(address owner, address spender, uint256 value) internal { _approve(owner, spender, value, true); } /** - * @dev Alternative version of {_approve} with an optional flag that can enable or disable the Approval event. + * @dev Variant of {_approve} with an optional flag to enable or disable the {Approval} event. * * By default (when calling {_approve}) the flag is set to true. On the other hand, approval changes made by * `_spendAllowance` during the `transferFrom` operation set the flag to false. This saves gas by not emitting any diff --git a/contracts/token/ERC20/extensions/ERC20FlashMint.sol b/contracts/token/ERC20/extensions/ERC20FlashMint.sol index d98dcf397..2cba0e2a5 100644 --- a/contracts/token/ERC20/extensions/ERC20FlashMint.sol +++ b/contracts/token/ERC20/extensions/ERC20FlashMint.sol @@ -19,7 +19,7 @@ import {ERC20} from "../ERC20.sol"; * overriding {maxFlashLoan} so that it correctly reflects the supply cap. */ abstract contract ERC20FlashMint is ERC20, IERC3156FlashLender { - bytes32 private constant _RETURN_VALUE = keccak256("ERC3156FlashBorrower.onFlashLoan"); + bytes32 private constant RETURN_VALUE = keccak256("ERC3156FlashBorrower.onFlashLoan"); /** * @dev The loan token is not valid. @@ -118,7 +118,7 @@ abstract contract ERC20FlashMint is ERC20, IERC3156FlashLender { } uint256 fee = flashFee(token, value); _mint(address(receiver), value); - if (receiver.onFlashLoan(_msgSender(), token, value, fee, data) != _RETURN_VALUE) { + if (receiver.onFlashLoan(_msgSender(), token, value, fee, data) != RETURN_VALUE) { revert ERC3156InvalidReceiver(address(receiver)); } address flashFeeReceiver = _flashFeeReceiver(); diff --git a/contracts/token/ERC20/extensions/ERC20Pausable.sol b/contracts/token/ERC20/extensions/ERC20Pausable.sol index 6ac0db2e2..e7c311cc1 100644 --- a/contracts/token/ERC20/extensions/ERC20Pausable.sol +++ b/contracts/token/ERC20/extensions/ERC20Pausable.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.20; import {ERC20} from "../ERC20.sol"; -import {Pausable} from "../../../security/Pausable.sol"; +import {Pausable} from "../../../utils/Pausable.sol"; /** * @dev ERC20 token with pausable token transfers, minting and burning. diff --git a/contracts/token/ERC20/extensions/ERC20Permit.sol b/contracts/token/ERC20/extensions/ERC20Permit.sol index 8acb23ddb..d6efb477e 100644 --- a/contracts/token/ERC20/extensions/ERC20Permit.sol +++ b/contracts/token/ERC20/extensions/ERC20Permit.sol @@ -18,8 +18,7 @@ import {Nonces} from "../../../utils/Nonces.sol"; * need to send a transaction, and thus is not required to hold Ether at all. */ abstract contract ERC20Permit is ERC20, IERC20Permit, EIP712, Nonces { - // solhint-disable-next-line var-name-mixedcase - bytes32 private constant _PERMIT_TYPEHASH = + bytes32 private constant PERMIT_TYPEHASH = keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)"); /** @@ -55,7 +54,7 @@ abstract contract ERC20Permit is ERC20, IERC20Permit, EIP712, Nonces { revert ERC2612ExpiredSignature(deadline); } - bytes32 structHash = keccak256(abi.encode(_PERMIT_TYPEHASH, owner, spender, value, _useNonce(owner), deadline)); + bytes32 structHash = keccak256(abi.encode(PERMIT_TYPEHASH, owner, spender, value, _useNonce(owner), deadline)); bytes32 hash = _hashTypedDataV4(structHash); diff --git a/contracts/token/ERC20/extensions/ERC20Votes.sol b/contracts/token/ERC20/extensions/ERC20Votes.sol index a4ded6b24..0ec1e6059 100644 --- a/contracts/token/ERC20/extensions/ERC20Votes.sol +++ b/contracts/token/ERC20/extensions/ERC20Votes.sol @@ -9,7 +9,7 @@ import {Checkpoints} from "../../../utils/structs/Checkpoints.sol"; /** * @dev Extension of ERC20 to support Compound-like voting and delegation. This version is more generic than Compound's, - * and supports token supply up to 2^224^ - 1, while COMP is limited to 2^96^ - 1. + * and supports token supply up to 2^208^ - 1, while COMP is limited to 2^96^ - 1. * * NOTE: This contract does not provide interface compatibility with Compound's COMP token. * @@ -27,10 +27,17 @@ abstract contract ERC20Votes is ERC20, Votes { error ERC20ExceededSafeSupply(uint256 increasedSupply, uint256 cap); /** - * @dev Maximum token supply. Defaults to `type(uint224).max` (2^224^ - 1). + * @dev Maximum token supply. Defaults to `type(uint208).max` (2^208^ - 1). + * + * This maximum is enforced in {_update}. It limits the total supply of the token, which is otherwise a uint256, + * so that checkpoints can be stored in the Trace208 structure used by {{Votes}}. Increasing this value will not + * remove the underlying limitation, and will cause {_update} to fail because of a math overflow in + * {_transferVotingUnits}. An override could be used to further restrict the total supply (to a lower value) if + * additional logic requires it. When resolving override conflicts on this function, the minimum should be + * returned. */ - function _maxSupply() internal view virtual returns (uint224) { - return type(uint224).max; + function _maxSupply() internal view virtual returns (uint256) { + return type(uint208).max; } /** @@ -70,7 +77,7 @@ abstract contract ERC20Votes is ERC20, Votes { /** * @dev Get the `pos`-th checkpoint for `account`. */ - function checkpoints(address account, uint32 pos) public view virtual returns (Checkpoints.Checkpoint224 memory) { + function checkpoints(address account, uint32 pos) public view virtual returns (Checkpoints.Checkpoint208 memory) { return _checkpoints(account, pos); } } diff --git a/contracts/token/ERC721/ERC721.sol b/contracts/token/ERC721/ERC721.sol index d56e33775..e72c459f3 100644 --- a/contracts/token/ERC721/ERC721.sol +++ b/contracts/token/ERC721/ERC721.sol @@ -251,7 +251,9 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er // Execute the update if (from != address(0)) { - delete _tokenApprovals[tokenId]; + // Clear approval. No need to re-authorize or emit the Approval event + _approve(address(0), tokenId, address(0), false); + unchecked { _balances[from] -= 1; } @@ -395,19 +397,33 @@ abstract contract ERC721 is Context, ERC165, IERC721, IERC721Metadata, IERC721Er * either the owner of the token, or approved to operate on all tokens held by this owner. * * Emits an {Approval} event. + * + * Overrides to this logic should be done to the variant with an additional `bool emitEvent` argument. */ - function _approve(address to, uint256 tokenId, address auth) internal virtual returns (address) { - address owner = ownerOf(tokenId); + function _approve(address to, uint256 tokenId, address auth) internal { + _approve(to, tokenId, auth, true); + } - // We do not use _isAuthorized because single-token approvals should not be able to call approve - if (auth != address(0) && owner != auth && !isApprovedForAll(owner, auth)) { - revert ERC721InvalidApprover(auth); + /** + * @dev Variant of `_approve` with an optional flag to enable or disable the {Approval} event. The event is not + * emitted in the context of transfers. + */ + function _approve(address to, uint256 tokenId, address auth, bool emitEvent) internal virtual { + // Avoid reading the owner unless necessary + if (emitEvent || auth != address(0)) { + address owner = ownerOf(tokenId); + + // We do not use _isAuthorized because single-token approvals should not be able to call approve + if (auth != address(0) && owner != auth && !isApprovedForAll(owner, auth)) { + revert ERC721InvalidApprover(auth); + } + + if (emitEvent) { + emit Approval(owner, to, tokenId); + } } _tokenApprovals[tokenId] = to; - emit Approval(owner, to, tokenId); - - return owner; } /** diff --git a/contracts/token/ERC721/extensions/ERC721Pausable.sol b/contracts/token/ERC721/extensions/ERC721Pausable.sol index 301469d0a..420edab22 100644 --- a/contracts/token/ERC721/extensions/ERC721Pausable.sol +++ b/contracts/token/ERC721/extensions/ERC721Pausable.sol @@ -4,7 +4,7 @@ pragma solidity ^0.8.20; import {ERC721} from "../ERC721.sol"; -import {Pausable} from "../../../security/Pausable.sol"; +import {Pausable} from "../../../utils/Pausable.sol"; /** * @dev ERC721 token with pausable token transfers, minting and burning. diff --git a/contracts/security/Pausable.sol b/contracts/utils/Pausable.sol similarity index 100% rename from contracts/security/Pausable.sol rename to contracts/utils/Pausable.sol diff --git a/contracts/utils/README.adoc b/contracts/utils/README.adoc index d95f4dad4..d88b00199 100644 --- a/contracts/utils/README.adoc +++ b/contracts/utils/README.adoc @@ -5,23 +5,20 @@ NOTE: This document is better viewed at https://docs.openzeppelin.com/contracts/ Miscellaneous contracts and libraries containing utility functions you can use to improve security, work with new data types, or safely use low-level primitives. -The {Address}, {Arrays}, {Base64} and {Strings} libraries provide more operations related to these native data types, while {SafeCast} adds ways to safely convert between the different signed and unsigned numeric types. -{Multicall} provides a function to batch together multiple calls in a single external call. - -For new data types: - - * {EnumerableMap}: like Solidity's https://solidity.readthedocs.io/en/latest/types.html#mapping-types[`mapping`] type, but with key-value _enumeration_: this will let you know how many entries a mapping has, and iterate over them (which is not possible with `mapping`). - * {EnumerableSet}: like {EnumerableMap}, but for https://en.wikipedia.org/wiki/Set_(abstract_data_type)[sets]. Can be used to store privileged accounts, issued IDs, etc. + * {ReentrancyGuard}: A modifier that can prevent reentrancy during certain functions. + * {Pausable}: A common emergency response mechanism that can pause functionality while a remediation is pending. + * {SafeCast}: Checked downcasting functions to avoid silent truncation. + * {Math}, {SignedMath}: Implementation of various arithmetic functions. + * {Multicall}: Simple way to batch together multiple calls in a single external call. + * {Create2}: Wrapper around the https://blog.openzeppelin.com/getting-the-most-out-of-create2/[`CREATE2` EVM opcode] for safe use without having to deal with low-level assembly. + * {EnumerableMap}: A type like Solidity's https://solidity.readthedocs.io/en/latest/types.html#mapping-types[`mapping`], but with key-value _enumeration_: this will let you know how many entries a mapping has, and iterate over them (which is not possible with `mapping`). + * {EnumerableSet}: Like {EnumerableMap}, but for https://en.wikipedia.org/wiki/Set_(abstract_data_type)[sets]. Can be used to store privileged accounts, issued IDs, etc. [NOTE] ==== Because Solidity does not support generic types, {EnumerableMap} and {EnumerableSet} are specialized to a limited number of key-value types. - -As of v3.0, {EnumerableMap} supports `uint256 -> address` (`UintToAddressMap`), and {EnumerableSet} supports `address` and `uint256` (`AddressSet` and `UintSet`). ==== -Finally, {Create2} contains all necessary utilities to safely use the https://blog.openzeppelin.com/getting-the-most-out-of-create2/[`CREATE2` EVM opcode], without having to deal with low-level assembly. - == Math {{Math}} @@ -42,6 +39,12 @@ Finally, {Create2} contains all necessary utilities to safely use the https://bl {{EIP712}} +== Security + +{{ReentrancyGuard}} + +{{Pausable}} + == Introspection This set of interfaces and contracts deal with https://en.wikipedia.org/wiki/Type_introspection[type introspection] of contracts, that is, examining which functions can be called on them. This is usually referred to as a contract's _interface_. diff --git a/contracts/security/ReentrancyGuard.sol b/contracts/utils/ReentrancyGuard.sol similarity index 89% rename from contracts/security/ReentrancyGuard.sol rename to contracts/utils/ReentrancyGuard.sol index 37a63d763..d2de919f5 100644 --- a/contracts/security/ReentrancyGuard.sol +++ b/contracts/utils/ReentrancyGuard.sol @@ -31,8 +31,8 @@ abstract contract ReentrancyGuard { // amount. Since refunds are capped to a percentage of the total // transaction's gas, it is best to keep them low in cases like this one, to // increase the likelihood of the full refund coming into effect. - uint256 private constant _NOT_ENTERED = 1; - uint256 private constant _ENTERED = 2; + uint256 private constant NOT_ENTERED = 1; + uint256 private constant ENTERED = 2; uint256 private _status; @@ -42,7 +42,7 @@ abstract contract ReentrancyGuard { error ReentrancyGuardReentrantCall(); constructor() { - _status = _NOT_ENTERED; + _status = NOT_ENTERED; } /** @@ -59,19 +59,19 @@ abstract contract ReentrancyGuard { } function _nonReentrantBefore() private { - // On the first call to nonReentrant, _status will be _NOT_ENTERED - if (_status == _ENTERED) { + // On the first call to nonReentrant, _status will be NOT_ENTERED + if (_status == ENTERED) { revert ReentrancyGuardReentrantCall(); } // Any calls to nonReentrant after this point will fail - _status = _ENTERED; + _status = ENTERED; } function _nonReentrantAfter() private { // By storing the original value once again, a refund is triggered (see // https://eips.ethereum.org/EIPS/eip-2200) - _status = _NOT_ENTERED; + _status = NOT_ENTERED; } /** @@ -79,6 +79,6 @@ abstract contract ReentrancyGuard { * `nonReentrant` function in the call stack. */ function _reentrancyGuardEntered() internal view returns (bool) { - return _status == _ENTERED; + return _status == ENTERED; } } diff --git a/contracts/utils/ShortStrings.sol b/contracts/utils/ShortStrings.sol index 2fb97824d..f1f33280c 100644 --- a/contracts/utils/ShortStrings.sol +++ b/contracts/utils/ShortStrings.sol @@ -39,7 +39,7 @@ type ShortString is bytes32; */ library ShortStrings { // Used as an identifier for strings longer than 31 bytes. - bytes32 private constant _FALLBACK_SENTINEL = 0x00000000000000000000000000000000000000000000000000000000000000FF; + bytes32 private constant FALLBACK_SENTINEL = 0x00000000000000000000000000000000000000000000000000000000000000FF; error StringTooLong(string str); error InvalidShortString(); @@ -91,7 +91,7 @@ library ShortStrings { return toShortString(value); } else { StorageSlot.getStringSlot(store).value = value; - return ShortString.wrap(_FALLBACK_SENTINEL); + return ShortString.wrap(FALLBACK_SENTINEL); } } @@ -99,7 +99,7 @@ library ShortStrings { * @dev Decode a string that was encoded to `ShortString` or written to storage using {setWithFallback}. */ function toStringWithFallback(ShortString value, string storage store) internal pure returns (string memory) { - if (ShortString.unwrap(value) != _FALLBACK_SENTINEL) { + if (ShortString.unwrap(value) != FALLBACK_SENTINEL) { return toString(value); } else { return store; @@ -113,7 +113,7 @@ library ShortStrings { * actual characters as the UTF-8 encoding of a single character can span over multiple bytes. */ function byteLengthWithFallback(ShortString value, string storage store) internal view returns (uint256) { - if (ShortString.unwrap(value) != _FALLBACK_SENTINEL) { + if (ShortString.unwrap(value) != FALLBACK_SENTINEL) { return byteLength(value); } else { return bytes(store).length; diff --git a/contracts/utils/Strings.sol b/contracts/utils/Strings.sol index 24c22d53d..6d657f7dd 100644 --- a/contracts/utils/Strings.sol +++ b/contracts/utils/Strings.sol @@ -10,8 +10,8 @@ import {SignedMath} from "./math/SignedMath.sol"; * @dev String operations. */ library Strings { - bytes16 private constant _HEX_DIGITS = "0123456789abcdef"; - uint8 private constant _ADDRESS_LENGTH = 20; + bytes16 private constant HEX_DIGITS = "0123456789abcdef"; + uint8 private constant ADDRESS_LENGTH = 20; /** * @dev The `value` string doesn't fit in the specified `length`. @@ -34,7 +34,7 @@ library Strings { ptr--; /// @solidity memory-safe-assembly assembly { - mstore8(ptr, byte(mod(value, 10), _HEX_DIGITS)) + mstore8(ptr, byte(mod(value, 10), HEX_DIGITS)) } value /= 10; if (value == 0) break; @@ -68,7 +68,7 @@ library Strings { buffer[0] = "0"; buffer[1] = "x"; for (uint256 i = 2 * length + 1; i > 1; --i) { - buffer[i] = _HEX_DIGITS[localValue & 0xf]; + buffer[i] = HEX_DIGITS[localValue & 0xf]; localValue >>= 4; } if (localValue != 0) { @@ -81,7 +81,7 @@ library Strings { * @dev Converts an `address` with fixed length of 20 bytes to its not checksummed ASCII `string` hexadecimal representation. */ function toHexString(address addr) internal pure returns (string memory) { - return toHexString(uint256(uint160(addr)), _ADDRESS_LENGTH); + return toHexString(uint256(uint160(addr)), ADDRESS_LENGTH); } /** diff --git a/contracts/utils/cryptography/EIP712.sol b/contracts/utils/cryptography/EIP712.sol index 58f8a2180..644f6f531 100644 --- a/contracts/utils/cryptography/EIP712.sol +++ b/contracts/utils/cryptography/EIP712.sol @@ -34,7 +34,7 @@ import {IERC5267} from "../../interfaces/IERC5267.sol"; abstract contract EIP712 is IERC5267 { using ShortStrings for *; - bytes32 private constant _TYPE_HASH = + bytes32 private constant TYPE_HASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); // Cache the domain separator as an immutable value, but also store the chain id that it corresponds to, in order to @@ -86,7 +86,7 @@ abstract contract EIP712 is IERC5267 { } function _buildDomainSeparator() private view returns (bytes32) { - return keccak256(abi.encode(_TYPE_HASH, _hashedName, _hashedVersion, block.chainid, address(this))); + return keccak256(abi.encode(TYPE_HASH, _hashedName, _hashedVersion, block.chainid, address(this))); } /** diff --git a/contracts/utils/introspection/ERC165Checker.sol b/contracts/utils/introspection/ERC165Checker.sol index 4d3948f0d..bfa59e686 100644 --- a/contracts/utils/introspection/ERC165Checker.sol +++ b/contracts/utils/introspection/ERC165Checker.sol @@ -14,7 +14,7 @@ import {IERC165} from "./IERC165.sol"; */ library ERC165Checker { // As per the EIP-165 spec, no interface should ever match 0xffffffff - bytes4 private constant _INTERFACE_ID_INVALID = 0xffffffff; + bytes4 private constant INTERFACE_ID_INVALID = 0xffffffff; /** * @dev Returns true if `account` supports the {IERC165} interface. @@ -24,7 +24,7 @@ library ERC165Checker { // InterfaceId_ERC165 and explicitly indicate non-support of InterfaceId_Invalid return supportsERC165InterfaceUnchecked(account, type(IERC165).interfaceId) && - !supportsERC165InterfaceUnchecked(account, _INTERFACE_ID_INVALID); + !supportsERC165InterfaceUnchecked(account, INTERFACE_ID_INVALID); } /** diff --git a/contracts/utils/structs/Checkpoints.sol b/contracts/utils/structs/Checkpoints.sol index 383f01af8..d0579f899 100644 --- a/contracts/utils/structs/Checkpoints.sol +++ b/contracts/utils/structs/Checkpoints.sol @@ -206,6 +206,193 @@ library Checkpoints { } } + struct Trace208 { + Checkpoint208[] _checkpoints; + } + + struct Checkpoint208 { + uint48 _key; + uint208 _value; + } + + /** + * @dev Pushes a (`key`, `value`) pair into a Trace208 so that it is stored as the checkpoint. + * + * Returns previous value and new value. + * + * IMPORTANT: Never accept `key` as a user input, since an arbitrary `type(uint48).max` key set will disable the library. + */ + function push(Trace208 storage self, uint48 key, uint208 value) internal returns (uint208, uint208) { + return _insert(self._checkpoints, key, value); + } + + /** + * @dev Returns the value in the first (oldest) checkpoint with key greater or equal than the search key, or zero if there is none. + */ + function lowerLookup(Trace208 storage self, uint48 key) internal view returns (uint208) { + uint256 len = self._checkpoints.length; + uint256 pos = _lowerBinaryLookup(self._checkpoints, key, 0, len); + return pos == len ? 0 : _unsafeAccess(self._checkpoints, pos)._value; + } + + /** + * @dev Returns the value in the last (most recent) checkpoint with key lower or equal than the search key, or zero if there is none. + */ + function upperLookup(Trace208 storage self, uint48 key) internal view returns (uint208) { + uint256 len = self._checkpoints.length; + uint256 pos = _upperBinaryLookup(self._checkpoints, key, 0, len); + return pos == 0 ? 0 : _unsafeAccess(self._checkpoints, pos - 1)._value; + } + + /** + * @dev Returns the value in the last (most recent) checkpoint with key lower or equal than the search key, or zero if there is none. + * + * NOTE: This is a variant of {upperLookup} that is optimised to find "recent" checkpoint (checkpoints with high keys). + */ + function upperLookupRecent(Trace208 storage self, uint48 key) internal view returns (uint208) { + uint256 len = self._checkpoints.length; + + uint256 low = 0; + uint256 high = len; + + if (len > 5) { + uint256 mid = len - Math.sqrt(len); + if (key < _unsafeAccess(self._checkpoints, mid)._key) { + high = mid; + } else { + low = mid + 1; + } + } + + uint256 pos = _upperBinaryLookup(self._checkpoints, key, low, high); + + return pos == 0 ? 0 : _unsafeAccess(self._checkpoints, pos - 1)._value; + } + + /** + * @dev Returns the value in the most recent checkpoint, or zero if there are no checkpoints. + */ + function latest(Trace208 storage self) internal view returns (uint208) { + uint256 pos = self._checkpoints.length; + return pos == 0 ? 0 : _unsafeAccess(self._checkpoints, pos - 1)._value; + } + + /** + * @dev Returns whether there is a checkpoint in the structure (i.e. it is not empty), and if so the key and value + * in the most recent checkpoint. + */ + function latestCheckpoint(Trace208 storage self) internal view returns (bool exists, uint48 _key, uint208 _value) { + uint256 pos = self._checkpoints.length; + if (pos == 0) { + return (false, 0, 0); + } else { + Checkpoint208 memory ckpt = _unsafeAccess(self._checkpoints, pos - 1); + return (true, ckpt._key, ckpt._value); + } + } + + /** + * @dev Returns the number of checkpoint. + */ + function length(Trace208 storage self) internal view returns (uint256) { + return self._checkpoints.length; + } + + /** + * @dev Returns checkpoint at given position. + */ + function at(Trace208 storage self, uint32 pos) internal view returns (Checkpoint208 memory) { + return self._checkpoints[pos]; + } + + /** + * @dev Pushes a (`key`, `value`) pair into an ordered list of checkpoints, either by inserting a new checkpoint, + * or by updating the last one. + */ + function _insert(Checkpoint208[] storage self, uint48 key, uint208 value) private returns (uint208, uint208) { + uint256 pos = self.length; + + if (pos > 0) { + // Copying to memory is important here. + Checkpoint208 memory last = _unsafeAccess(self, pos - 1); + + // Checkpoint keys must be non-decreasing. + if (last._key > key) { + revert CheckpointUnorderedInsertion(); + } + + // Update or push new checkpoint + if (last._key == key) { + _unsafeAccess(self, pos - 1)._value = value; + } else { + self.push(Checkpoint208({_key: key, _value: value})); + } + return (last._value, value); + } else { + self.push(Checkpoint208({_key: key, _value: value})); + return (0, value); + } + } + + /** + * @dev Return the index of the last (most recent) checkpoint with key lower or equal than the search key, or `high` if there is none. + * `low` and `high` define a section where to do the search, with inclusive `low` and exclusive `high`. + * + * WARNING: `high` should not be greater than the array's length. + */ + function _upperBinaryLookup( + Checkpoint208[] storage self, + uint48 key, + uint256 low, + uint256 high + ) private view returns (uint256) { + while (low < high) { + uint256 mid = Math.average(low, high); + if (_unsafeAccess(self, mid)._key > key) { + high = mid; + } else { + low = mid + 1; + } + } + return high; + } + + /** + * @dev Return the index of the first (oldest) checkpoint with key is greater or equal than the search key, or `high` if there is none. + * `low` and `high` define a section where to do the search, with inclusive `low` and exclusive `high`. + * + * WARNING: `high` should not be greater than the array's length. + */ + function _lowerBinaryLookup( + Checkpoint208[] storage self, + uint48 key, + uint256 low, + uint256 high + ) private view returns (uint256) { + while (low < high) { + uint256 mid = Math.average(low, high); + if (_unsafeAccess(self, mid)._key < key) { + low = mid + 1; + } else { + high = mid; + } + } + return high; + } + + /** + * @dev Access an element of the array without performing bounds check. The position is assumed to be within bounds. + */ + function _unsafeAccess( + Checkpoint208[] storage self, + uint256 pos + ) private pure returns (Checkpoint208 storage result) { + assembly { + mstore(0, self.slot) + result.slot := add(keccak256(0, 0x20), pos) + } + } + struct Trace160 { Checkpoint160[] _checkpoints; } diff --git a/scripts/generate/templates/Checkpoints.opts.js b/scripts/generate/templates/Checkpoints.opts.js index b8be23104..08b7b910b 100644 --- a/scripts/generate/templates/Checkpoints.opts.js +++ b/scripts/generate/templates/Checkpoints.opts.js @@ -1,5 +1,5 @@ // OPTIONS -const VALUE_SIZES = [224, 160]; +const VALUE_SIZES = [224, 208, 160]; const defaultOpts = size => ({ historyTypeName: `Trace${size}`, diff --git a/scripts/solhint-custom/index.js b/scripts/solhint-custom/index.js index a4cba1a93..9625027ee 100644 --- a/scripts/solhint-custom/index.js +++ b/scripts/solhint-custom/index.js @@ -48,9 +48,9 @@ module.exports = [ VariableDeclaration(node) { if (node.isDeclaredConst) { - if (/^_/.test(node.name)) { - // TODO: re-enable and fix - // this.error(node, 'Constant variables should not have leading underscore'); + // TODO: expand visibility and fix + if (node.visibility === 'private' && /^_/.test(node.name)) { + this.error(node, 'Constant variables should not have leading underscore'); } } else if (node.visibility === 'private' && !/^_/.test(node.name)) { this.error(node, 'Non-constant private variables must have leading underscore'); diff --git a/scripts/upgradeable/upgradeable.patch b/scripts/upgradeable/upgradeable.patch index 6508212c5..125272a25 100644 --- a/scripts/upgradeable/upgradeable.patch +++ b/scripts/upgradeable/upgradeable.patch @@ -151,7 +151,7 @@ index 3800804a..90c1db78 100644 abstract contract EIP712 is IERC5267 { - using ShortStrings for *; - - bytes32 private constant _TYPE_HASH = + bytes32 private constant TYPE_HASH = keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); - // Cache the domain separator as an immutable value, but also store the chain id that it corresponds to, in order to @@ -207,8 +207,8 @@ index 3800804a..90c1db78 100644 } function _buildDomainSeparator() private view returns (bytes32) { -- return keccak256(abi.encode(_TYPE_HASH, _hashedName, _hashedVersion, block.chainid, address(this))); -+ return keccak256(abi.encode(_TYPE_HASH, _EIP712NameHash(), _EIP712VersionHash(), block.chainid, address(this))); +- return keccak256(abi.encode(TYPE_HASH, _hashedName, _hashedVersion, block.chainid, address(this))); ++ return keccak256(abi.encode(TYPE_HASH, _EIP712NameHash(), _EIP712VersionHash(), block.chainid, address(this))); } /** diff --git a/test/token/ERC20/extensions/ERC20Votes.test.js b/test/token/ERC20/extensions/ERC20Votes.test.js index a6abb5f9f..faf1a15ad 100644 --- a/test/token/ERC20/extensions/ERC20Votes.test.js +++ b/test/token/ERC20/extensions/ERC20Votes.test.js @@ -48,7 +48,7 @@ contract('ERC20Votes', function (accounts) { }); it('minting restriction', async function () { - const value = web3.utils.toBN(1).shln(224); + const value = web3.utils.toBN(1).shln(208); await expectRevertCustomError(this.token.$_mint(holder, value), 'ERC20ExceededSafeSupply', [ value, value.subn(1), diff --git a/test/token/ERC721/ERC721.behavior.js b/test/token/ERC721/ERC721.behavior.js index 4dd6e234c..10f848265 100644 --- a/test/token/ERC721/ERC721.behavior.js +++ b/test/token/ERC721/ERC721.behavior.js @@ -87,8 +87,9 @@ function shouldBehaveLikeERC721(owner, newOwner, approved, anotherApproved, oper expectEvent(receipt, 'Transfer', { from: owner, to: this.toWhom, tokenId: tokenId }); }); - it('clears the approval for the token ID', async function () { + it('clears the approval for the token ID with no event', async function () { expect(await this.token.getApproved(tokenId)).to.be.equal(ZERO_ADDRESS); + expectEvent.notEmitted(receipt, 'Approval'); }); it('adjusts owners balances', async function () { diff --git a/test/security/Pausable.test.js b/test/utils/Pausable.test.js similarity index 100% rename from test/security/Pausable.test.js rename to test/utils/Pausable.test.js diff --git a/test/security/ReentrancyGuard.test.js b/test/utils/ReentrancyGuard.test.js similarity index 100% rename from test/security/ReentrancyGuard.test.js rename to test/utils/ReentrancyGuard.test.js diff --git a/test/utils/structs/Checkpoints.t.sol b/test/utils/structs/Checkpoints.t.sol index afda2423e..7bdbcfddf 100644 --- a/test/utils/structs/Checkpoints.t.sol +++ b/test/utils/structs/Checkpoints.t.sol @@ -115,6 +115,114 @@ contract CheckpointsTrace224Test is Test { } } +contract CheckpointsTrace208Test is Test { + using Checkpoints for Checkpoints.Trace208; + + // Maximum gap between keys used during the fuzzing tests: the `_prepareKeys` function with make sure that + // key#n+1 is in the [key#n, key#n + _KEY_MAX_GAP] range. + uint8 internal constant _KEY_MAX_GAP = 64; + + Checkpoints.Trace208 internal _ckpts; + + // helpers + function _boundUint48(uint48 x, uint48 min, uint48 max) internal view returns (uint48) { + return SafeCast.toUint48(bound(uint256(x), uint256(min), uint256(max))); + } + + function _prepareKeys(uint48[] memory keys, uint48 maxSpread) internal view { + uint48 lastKey = 0; + for (uint256 i = 0; i < keys.length; ++i) { + uint48 key = _boundUint48(keys[i], lastKey, lastKey + maxSpread); + keys[i] = key; + lastKey = key; + } + } + + function _assertLatestCheckpoint(bool exist, uint48 key, uint208 value) internal { + (bool _exist, uint48 _key, uint208 _value) = _ckpts.latestCheckpoint(); + assertEq(_exist, exist); + assertEq(_key, key); + assertEq(_value, value); + } + + // tests + function testPush(uint48[] memory keys, uint208[] memory values, uint48 pastKey) public { + vm.assume(values.length > 0 && values.length <= keys.length); + _prepareKeys(keys, _KEY_MAX_GAP); + + // initial state + assertEq(_ckpts.length(), 0); + assertEq(_ckpts.latest(), 0); + _assertLatestCheckpoint(false, 0, 0); + + uint256 duplicates = 0; + for (uint256 i = 0; i < keys.length; ++i) { + uint48 key = keys[i]; + uint208 value = values[i % values.length]; + if (i > 0 && key == keys[i - 1]) ++duplicates; + + // push + _ckpts.push(key, value); + + // check length & latest + assertEq(_ckpts.length(), i + 1 - duplicates); + assertEq(_ckpts.latest(), value); + _assertLatestCheckpoint(true, key, value); + } + + if (keys.length > 0) { + uint48 lastKey = keys[keys.length - 1]; + if (lastKey > 0) { + pastKey = _boundUint48(pastKey, 0, lastKey - 1); + + vm.expectRevert(); + this.push(pastKey, values[keys.length % values.length]); + } + } + } + + // used to test reverts + function push(uint48 key, uint208 value) external { + _ckpts.push(key, value); + } + + function testLookup(uint48[] memory keys, uint208[] memory values, uint48 lookup) public { + vm.assume(values.length > 0 && values.length <= keys.length); + _prepareKeys(keys, _KEY_MAX_GAP); + + uint48 lastKey = keys.length == 0 ? 0 : keys[keys.length - 1]; + lookup = _boundUint48(lookup, 0, lastKey + _KEY_MAX_GAP); + + uint208 upper = 0; + uint208 lower = 0; + uint48 lowerKey = type(uint48).max; + for (uint256 i = 0; i < keys.length; ++i) { + uint48 key = keys[i]; + uint208 value = values[i % values.length]; + + // push + _ckpts.push(key, value); + + // track expected result of lookups + if (key <= lookup) { + upper = value; + } + // find the first key that is not smaller than the lookup key + if (key >= lookup && (i == 0 || keys[i - 1] < lookup)) { + lowerKey = key; + } + if (key == lowerKey) { + lower = value; + } + } + + // check lookup + assertEq(_ckpts.lowerLookup(lookup), lower); + assertEq(_ckpts.upperLookup(lookup), upper); + assertEq(_ckpts.upperLookupRecent(lookup), upper); + } +} + contract CheckpointsTrace160Test is Test { using Checkpoints for Checkpoints.Trace160;