openzeppelin-contracts
openzeppelin-contracts copied to clipboard
OpenZeppelin
Return the current `_value` when incrementing or decrementing a `Counter`
🧐 Motivation
Instead of running a call to counter.increment() or counter.decrement() and then a subsequent call to counter.current(), wouldn't it be simpler to just return the count along with the relative call?
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Counters.sol
📝 Details
Currently the interface is:
current(struct Counters.Counter counter) → uint256 internal
increment(struct Counters.Counter counter) internal
decrement(struct Counters.Counter counter) internal
but I'm proposing it's changed to
current(struct Counters.Counter counter) → uint256 internal
increment(struct Counters.Counter counter) → uint256 internal
decrement(struct Counters.Counter counter) → uint256 internal
This could be implemented like so:
function increment(Counter storage counter) internal returns(uint256) {
unchecked {
counter._value += 1;
}
+ return counter._value;
}
Hello @hexcowboy and thank you for raising that issue.
This was already discussed as part of #2599. At the time we didn't feel comfortable with the proposition of the PR with added 2 functions with close names. Your approach seems more conservative and is backward compatible.
My issue with #2599 was that it is not obvious in the interface whether the return value is pre or post increment. I would oppose this suggestion for the same reason.
Intuitively we'd expect the return value to be the post-incremented one, i.e. the end result of calling the counter.increment() function.
The problem is that often that's not really the useful value. Imagine that your counter is a nonce, you generally want it to start at 0, so you'd want the pre-increment value.
Hello I think two types of functions can be added to the Counters contract (the names were given as examples):
/// @dev returns the current value and increments the value in the storage
function returnAndIncrement(Counter storage counter) internal returns(uint256 counter_) {
unchecked {
counter_ = counter._value++;
}
}
and
/// @dev increments the value in the storage and returns the new value
function incrementAndReturn(Counter storage counter) internal returns(uint256 counter_) {
unchecked {
counter_ = ++counter._value;
}
}
The same with decrement:
/// @dev returns the current value and decrements the value in the storage
function returnAndDecrement(Counter storage counter) internal returns(uint256 counter_) {
require(counter._value > 0, "Counter: decrement overflow");
unchecked {
counter_ = counter._value--;
}
}
and
/// @dev decrements the value in the storage and returns the new value
function decrementAndReturn(Counter storage counter) internal returns(uint256 counter_) {
require(counter._value > 0, "Counter: decrement overflow");
unchecked {
counter_ = --counter._value;
}
}
It also consumes less gas than going the standard way. If there is something wrong with my idea, please let me know.
I support something like this, but returnAndIncrement is a confusing name, as "returning" is always the last thing done in a function.
We should brainstorm other options. What about getCurrentAndIncrement together with incrementAndGet? Hoping to hear other options as well though.
A way to sidestep this issue would be to return both pre and post values from the function:
function increment(Counter storage counter) returns (uint previousValue, uint currentValue);
Although this sidesteps the issue of naming, it has the issue that return parameters are not really named so the callee needs to know the order in which to interpret them. I'd imagine previous, current is the intuitive order though.
Maybe do it as in _useNonce?
Something like: useCurrentIncrementValue & useNextIncrementValue and useCurrentDecrementValue & useNextDecrementValue.
or an abridged version: useCurrentInc & useNextInc and useCurrentDec & useNextDec.
Sorry, I'm not very good at naming. I think your "getCurrentAndIncrement together with incrementAndGet" are better than my suggestion.
About your suggestion about returning both values from the increment and decrement functions I think is a good solution, but I would be confused about what I should get from the functions, i.e.you suggest calling them previousValue and currentValue, but I intuitively think that the current value is the one before the increment/decrement, and what comes after is the next value. I think in this case, universality can be bad for the clarity of what the user of these functions should get.
I think this is a good choice: functions will return data and developers can choose the value they need for their current purposes.
But can you answer why you don't want to create a separate function for each case?
I'm ok with creating separate functions, we just need good names for them. I don't think the alternatives proposed so far are good enough. Maybe we should just go with preIncrement and postIncrement.
preIncrement and postIncrement looks nice, but does not make it clear by itself what exactly they do.
As I wrote before, I'm not very good at naming functions. I can only suggest something like:
getAndIncrementValue, incrementAndGetValue, getAndDecrementValue, decrementAndGetValue.
maybe getAndIncrement, incrementAndGet, getAndDecrement, decrementAndGet.
maybe
getAndIncrement,incrementAndGet,getAndDecrement,decrementAndGet.
Let's go with this. Feel free to submit a PR.