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

Remove Counters.sol #4233

Closed
ernestognw opened this issue May 10, 2023 · 22 comments · Fixed by #4289
Closed

Remove Counters.sol #4233

ernestognw opened this issue May 10, 2023 · 22 comments · Fixed by #4289
Labels
breaking change Changes that break backwards compatibility of the public API.
Milestone

Comments

@ernestognw
Copy link
Member

🧐 Motivation

The Counters.sol contract initially served as an abstraction for a number that guarantees two main properties:

  • Provide a number that can only be incremented, decremented or reset
  • Abstract its behavior into a utility

Also, it allows to use unchecked arithmetic assuming no overflow

However, its usage has been reduced across OpenZeppelin Contracts and there are some relevant reasons to remove it, such as:

  • New announced Solidity features might allow replicating the behavior without the Counters.sol contract (eg. User Defined Types)
  • It apparently doesn't add much value against using a regular variable
  • It increments the maintenance surface
  • Can be confusing for newcomers

📝 Details

We're proposing removing it for Contracts 5.0, but we want to hear comments against removing it (if any), so we can have a clear view of what to do in the future. An alternative we're strongly considering is removing it for 5.0 but re-adding in the future some variation if people consider it useful and it makes sense.

@ernestognw ernestognw added the breaking change Changes that break backwards compatibility of the public API. label May 10, 2023
@ernestognw ernestognw added this to the 5.0 milestone May 10, 2023
@QuentinMerabet
Copy link

All in! Just wanted to add gas efficiency as another good reason to remove it.

@Amxx
Copy link
Collaborator

Amxx commented May 22, 2023

All in! Just wanted to add gas efficiency as another good reason to remove it.

@QuentinMerabet Can you tell more about the gas inefficiency you noticed? Is that related to storage packing (counter could be smaller than uint256) ?

@Garito
Copy link

Garito commented May 22, 2023

Please provide an example on how to migrate Counters to the new way to achieve its functionality
Thanks

@ernestognw
Copy link
Member Author

Please provide an example on how to migrate Counters to the new way to achieve its functionality Thanks

@Garito, if we remove it we'll provide a guide for migrating to 5.0.

Curious to know, would you like the migration example to be in another contract abstraction or to be a guide to implement it with raw solidity?

We're removing it because we have the feeling that multiple people in the community feel it like an unnecessary abstraction, so having your input on what you expect as a migration guide will help us understand better how to approach this removal.

@balajipachai
Copy link
Contributor

Let us consider an example of creating an ERC721 contract and making use of Counters library.

// SPDX-License-Identifier: MIT
pragma solidity 0.7.5;

import {Counters} from "@openzeppelin/contracts/utils/Counters.sol";

contract MyToken is ERC721, Pausable, Ownable {
        using Counters for Counters.Counter;
        Counters.Counter private _tokenIdCounter;

        function safeMint(address receiver) public onlyOwner {
            uint256 tokenId = _tokenIdCounter.current();
            _safeMint(_receiver, tokenId);
            _tokenIdCounter.increment();
        }

Here we see that Counters library is particularly used to get current value & to increment value within the struct Counter, thus, in my opinion for new contracts to not make use of Counters library I think of below possible solution:

  1. We can add a uint256 tokenId state variable right in the ERC721 smart contract, however this would impact all other functions that take in tokenId as the parameter plus it would restrict to have only incremental tokenIds i.e. 1, 2, 3, ..... n
contract ERC721 {
        uint256 tokenId = 0;

       // tokenId will be used within `mint` function and will be incremented by 1 upon each successful mint
}

@frangio
Copy link
Contributor

frangio commented May 24, 2023

@balajipachai Without Counters the code would be as follows:

contract MyToken is ERC721, Pausable, Ownable {
        uint256 private _tokenIdCounter;

        function safeMint(address receiver) public onlyOwner {
            uint256 tokenId = _tokenIdCounter;
            _safeMint(_receiver, tokenId);
            _tokenIdCounter += 1;
        }
}

Alternatively, simply _safeMint(_receiver, _tokenIdCounter++);

@Garito
Copy link

Garito commented May 24, 2023

Giving this examples: what was the point of Counters then?

@balajipachai
Copy link
Contributor

balajipachai commented May 25, 2023

@balajipachai Without Counters the code would be as follows:

contract MyToken is ERC721, Pausable, Ownable {
        uint256 private _tokenIdCounter;

        function safeMint(address receiver) public onlyOwner {
            uint256 tokenId = _tokenIdCounter;
            _safeMint(_receiver, tokenId);
            _tokenIdCounter += 1;
        }
}

Alternatively, simply _safeMint(_receiver, _tokenIdCounter++);

Got it, let me make the necessary changes wherever Counters was used, update the tests and submit a PR for the same.

@frangio
Copy link
Contributor

frangio commented May 29, 2023

what was the point of Counters then?

The point of Counters was to provide an "object" that was guaranteed to behave in a way that will not overflow, therefore can be safely incremented in an "unchecked" way. It was introduced before Solidity had native checked arithmetic, so it made slightly more sense back then. There are a few issues though. This "guarantee" was always a soft guarantee, because the inner struct members are directly modifiable bypassing the struct interface. Additionally, a Counter being a uint256 value occupies an entire storage slot whereas in most occasions packing a counter in storage with other values will be a far larger optimization than removing overflow checks; the significance of this has also changed since the introduction of Counters due to changes in the EVM gas prices.

@frangio
Copy link
Contributor

frangio commented Oct 19, 2023

You're looking at 4.x docs, notice the URL. 5.x is here: https://docs.openzeppelin.com/contracts/5.x/

@elvisisvan
Copy link

no wonder i couldn't resolve

Error HH404: File @openzeppelin/contracts/utils/Counters.sol, imported from contracts/MyNFT.sol, not found.
and

      using Counters for Counters.Counter;
   |        ^^^^^^^^
Error HH600: Compilation failed

no matter how many times i reinstalled @openzeppelin/contracts, it simply doesn't exist since version 5.0.0 anymore, which consequently makes this ethereum guide deprecated because its MyNFT.sol code is:

//Contract based on [https://docs.openzeppelin.com/contracts/3.x/erc721](https://docs.openzeppelin.com/contracts/3.x/erc721)
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import "@openzeppelin/contracts/utils/Counters.sol";
import "@openzeppelin/contracts/access/Ownable.sol";
import "@openzeppelin/contracts/token/ERC721/extensions/ERC721URIStorage.sol";

contract MyNFT is ERC721URIStorage, Ownable {
    using Counters for Counters.Counter;
    Counters.Counter private _tokenIds;

    constructor() ERC721("MyNFT", "NFT") {}

    function mintNFT(address recipient, string memory tokenURI) public onlyOwner returns (uint256) {
        _tokenIds.increment();

        uint256 newItemId = _tokenIds.current();
        _mint(recipient, newItemId);
        _setTokenURI(newItemId, tokenURI);

        return newItemId;
    }
    function totalSupply() public view returns (uint256) {
        return _tokenIds.current();
    }
}

so i tweeted the guide's last editor to suggest an update on it

@bra0xit
Copy link

bra0xit commented Dec 1, 2023

Screenshot 2023-12-01 at 15 15 07

It's also in the OpenZeppelin docs - should perhaps be a note there for it not being included post version 5.

@ernestognw
Copy link
Member Author

@bra0xit

You're looking at 4.x docs, notice the URL. 5.x is here: https://docs.openzeppelin.com/contracts/5.x/

@bra0xit
Copy link

bra0xit commented Dec 2, 2023

Noted. Thanks for letting me know! :)

Screenshot 2023-12-02 at 09 50 17

Btw - I think this dropdown can be improved - tried pressing 'Current version' probably 3 or 4 times before realizing Version 5 is the current version.

I think the first dropdown should instead look like this (not current version implicitly being older version)

Version 5 (Current version)
Version 4
Version 3
Version 2

etc.

@otarasovqs
Copy link

otarasovqs commented Dec 7, 2023

Still, there are huge of guides with Counters.sol.
And there is no one word for how to replace it?

@QuentinMerabet
Copy link

QuentinMerabet commented Dec 7, 2023

Still, there are huge of guides with Counters.sol. And there is no one word for how to replace it?

@otarasovqs You can simply use a simple uint256 variable and increment or decrement it.

uint256 counter;

// Increment the counter
++counter;
// Decrement the counter
--counter;

Note that even though we usually put ++ and -- after the variable, they're slightly more gas efficient when placed before the variable.

And of course...

// Get the current value
uint256 current = counter;
// Reset the value
counter = 0;

@M4x28
Copy link

M4x28 commented Dec 30, 2023

Please provide an example on how to migrate Counters to the new way to achieve its functionality Thanks
@Garito Take a look at my suggestion here.

@OpenZeppelin OpenZeppelin deleted a comment from Hamza49s Mar 11, 2024
@MohamedBenKedim
Copy link

import "@openzeppelin/contracts/token/ERC721/extensions/ERC721URIStorage.sol";
import "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import "@openzeppelin/contracts/utils/Counters.sol";

i have these imports but only the Counters import is not accepted
Error message : Source "@openzeppelin/contracts/utils/Counters.sol" not found: File import callback not supported(6275)
I need a help with thease Please !

@AlexAjit
Copy link

AlexAjit commented Apr 2, 2024

import "@openzeppelin/contracts/token/ERC721/extensions/ERC721URIStorage.sol"; import "@openzeppelin/contracts/token/ERC721/ERC721.sol"; import "@openzeppelin/contracts/utils/Counters.sol";

i have these imports but only the Counters import is not accepted Error message : Source "@openzeppelin/contracts/utils/Counters.sol" not found: File import callback not supported(6275) I need a help with thease Please !

Same to me, I have imported the that but in openzeppelin file it does not have any counter.sol file. please help with that anyone.

@vanshika-srivastava
Copy link

any luck here

@iitKD
Copy link

iitKD commented Apr 5, 2024

for anyone having error with import "@openzeppelin/contracts/utils/Counters.sol"
you can use install previous version of openzeppelin/contract that has the Counters.sol
example : npm install @openzeppelin/contracts@4.x

@etabebe1
Copy link

etabebe1 commented May 3, 2024

use

@openzeppelin/contract@(older version) such as <= 4.1.0

it'll work but the older versions have some vulnerabilities

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Changes that break backwards compatibility of the public API.
Projects
None yet