Allow Counter increment by more than just 1

[tab00]

After a batch mint of ERC1155 NFTs I want to be able to increment the total supply count like this:

_mintBatch(_receiver, ids, amounts, "");
supply.increment(ids.length);

However currently increment() can only increase the counter by 1:

openzeppelin-contracts/contracts/utils/Counters.sol

Line 26 in 4a9cc8b

function increment(Counter storage counter) internal {

So I have to resort to using a loop:

_mintBatch(_receiver, ids, amounts, "");
for (uint256 i = 0; i < ids.length; i++) supply.increment();

or not use Counters and do supply += ids.length;

Please allow increment() to increase the counter by more than just 1.

[mw2000]

I propose adding overloaded increment and decrement functionswith the following signatures:

function increment(Counter storage counter, uint256 amount)
function decrement(Counter storage counter, uint256 amount)

[tab00]

function increment(Counter storage counter, uint256 amount)
function decrement(Counter storage counter, uint256 amount)

I was thinking the same.

Here are examples of simple counters that take an amount value to increment by that I've used before:
https://github.com/awwx/meteor-mongo-counter
https://www.npmjs.com/package/mongodb-counter

[philipSKYBIT]

I need this too. I might try a pull request.

[Amxx]

Hello @tab00

I understand your request, but I'm not super comfortable with where this is heading.

We already have:

• increment(Counter storage counter)
• decrement(Counter storage counter)
• reset(Counter storage counter)

You'd like to add

• increment(Counter storage counter, uint256)
• decrement(Counter storage counter, uint256)

Then someone will ask for set(Counter storage counter, uint256) ... and we'll just have a simple uint256 wrapper ... which doesn't sound like it makes much sense (why not just use uint256?)

@frangio What do you think? Should we expand the Counters library further? Should we use UDVT instead of structs?

Note: it's not "clean" but if you really want to do that now, you can always do counter._value += 17; or counter._value -= 42;

[tab00]

why not just use uint256

You could ask the same with how Counters is now - why not just use count += 1, count -= 1, count = 0 instead of increment(), decrement(), reset()?

I was thinking a value of Counters is its safety checks. e.g. currently there is a check that prevents underflow.

reset(Counter storage counter, uint256 newValue) would actually be a good idea too. These proposed functions would be more generalized versions of what currently exists, and if implemented then the original functions could call them by passing 1 or 0 for the amounts.

Note: it's not "clean" but if you really want to do that now, you can always do counter._value += 17; or counter._value -= 42;

That's a possible workaround in the meantime and definitely more gas-efficient, though there is a comment that explicitly says _value should never be directly accessed:

openzeppelin-contracts/contracts/utils/Counters.sol

Line 16 in 76eee35

// This variable should never be directly accessed by users of the library: interactions must be restricted to

[mw2000]

I agree with @tab00 on this, I think the proposed new functions to the Counters library would be good in terms of the added functionality mixed in with the safety checks.

[dhxmo]

hi @Amxx I'd like to contribute to this issue. Are we thinking of changing increment, decrement and reset function to include a newValue or only the reset function?

also, is there any resource I can study to understand the necessary safety checks for this op? thank you.

[hack3r-0m]

A better solution would be to create a new library CounterExtended.sol which will not break existing apis.

[Amxx]

A better solution would be to create a new library CounterExtended.sol which will not break existing apis.

What would this new library provide over just using uint256? Adding code creates discoverability and maintainability issues.

[Amxx]

One important thing with the Counter is that by increasing by 1, we can very safely assume that increment will not overflow, and we can "uncheck" it. As soon as we increment by arbitrary values, we can no longer make this assumption, so we should re-add the overflow checks, which IMO defeats the whole point of having Counter.

[frangio]

Yes, the intent of the Counter abstraction is to represent a value that is known to never overflow. Incrementing by arbitrary uint256 removes that benefit entirely. We could consider incrementing by small integer types like uint8 if that's seen as valuable, but I don't think it's really necessary.

[frangio]

Counters are being removed in 5.0.