Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pauseUntil tested, coded, documented #57

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

CarlosAlegreUr
Copy link

As discussed in this issue on the official OpenZeppelin repository, this PR introduces a refactor of the Pausable.sol contract to implement a pauseUntil(uint48 deadline) functionality.


Changes

1. Pausable Contract

  • Refactored to include the pauseUntil(uint48 unpauseDeadline) functionality.
    • Backwards Compatibility: The implementation ensures that the same storage slots are used, preventing conflicts between old and new data.
    • ERC6372 Compatibility: Time-based functionality has been designed with ERC6372 in mind.
    • Potential Developer Consideration:
      • Developers updating from previous Pausable versions who independently implemented both ERC6372 and Pausable (e.g., via IERC5805) might face challenges if different time measurements are needed for pausing and other features.
      • Example: A Pausable contract implementing IERC5805 might use different time units for pausing and voting.

2. New Directory Structure

  • Created a new directory utils/pausability to accommodate the refactor:
    • Pausable.sol: General implementation of Pausable without a specific time measurement tool.
    • DefaultPausable.sol: A default implementation of Pausable using ERC6372 with block.timestamp as the time unit.

3. PausableMock Updates

  • Updated the mock contract to support:
    • Calling the new pauseUntil(uint48 deadline) function.
    • Accessing the new internal view function _unpauseDeadline().

4. Unit Tests

  • Added comprehensive unit tests to validate the new pauseUntil() functionality:
    • New test flows:
      • pause() -> pauseUntil()
      • pauseUntil() -> pause() (before and after the deadline)
      • pauseUntil() -> unpause() (before and after the deadline)
      • pauseUntil() -> pauseUntil() (before and after the previous deadline)

@CarlosAlegreUr CarlosAlegreUr requested a review from a team as a code owner December 30, 2024 11:33
Comment on lines +26 to +44
function pause() external {
_pause();
}

function unpause() external {
_unpause();
}

function pauseUntil(uint256 duration) external {
_pauseUntil(uint48(duration));
}

function getPausedUntilDeadline() external view returns (uint256) {
return _unpauseDeadline();
}

function getPausedUntilDeadlineAndTimestamp() external view returns (uint256, uint256) {
return (_unpauseDeadline(), clock());
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These functions to expose internal functions are not necessary since we compile exposed contracts. To deploy the exposed contract, append a dollar sign to the contract name (like $Pausable). The exposed contract has all the internal functions exposed externally with dollar signs prepended to the function names.

Example:
https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/dfe0973e94e137277cae220ef54eb66df60cbf92/test/token/ERC20/ERC20.test.js#L53

@@ -0,0 +1,34 @@
// SPDX-License-Identifier: MIT
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this contract is necessary. We can either have the default values in the implementation (virtual so consumers can specify if they want). Or we can leave the choice to the user.

@@ -0,0 +1,201 @@
// SPDX-License-Identifier: MIT
// OpenZeppelin Contracts (last updated v5.0.0) (utils/Pausable.sol)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should be removed--it will be added automatically if necessary.

Comment on lines +17 to +19
* @dev Clock is used here for time checkings on pauses with defined end-date.
*
* @dev IERC6372 implementation of a clock() based on native `block.timestamp`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't believe there can be 2 dev tags

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants