Remove unnecessary SafeMath call

[nventuro]

Fixes #1552

This also takes Counter out of drafts, and makes it part of our API, though I guess we technically don't need to do that.

[tinchoabbate]

Okey, I've taken a look at it. To be honest, the whole thing looks a bit like an overkill to me. After all, the whole point is to just +1 -1 a number. Having to import yet another library, and define something as using Counter for Counter.Counter, just to do that, does not seem so friendly to me. I've got to admit that just calling .increment() is more readable than incrementing by hand with SafeMath though.

Anyway, I understand that the rationale behind it is to "encapsulate" the counter's value in a struct and that, if that struct was opaque, nobody could modify it. Considering that for now, the struct's value can be modified, and if it can be done, it will certainly be done, I'd still rely on SafeMath when incrementing, just to prevent any careless developer from running into an overflow after manually overwriting the struct's value and calling increment.

If you still want to keep it this way, I'd rather have an inline comment in the struct definition (in Counter.sol) saying that the value is not to be modified directly, but just through increment and decrement. Also, I'd explicitly write a comment in the increment function explaining why it does not use SafeMath. Finally, it'd be nice, though maybe a bit tricky, to have tests for the overflow in increment().
Ah, and you should move the test file out of the drafts folder 😛

[nventuro]

Anyway, I understand that the rationale behind it is to "encapsulate" the counter's value in a struct and that, if that struct was opaque, nobody could modify it. Considering that for now, the struct's value can be modified, and if it can be done, it will certainly be done, I'd still rely on SafeMath when incrementing, just to prevent any careless developer from running into an overflow after manually overwriting the struct's value and calling increment.

Counter doesn't just remove the SafeMath call (which could've been removed without using it - after all, the value is only ever incremented or decremented by one): it helps correctness in that the count cannot be assigned any random value. The fact that Solidity doesn't enforce access through the methods is a pity, but still it'd only be a case of checking those accesses aren't present in the code (in fact, some sort of static analysis tool could easily do this). Without Counter, the surface of possible interactions with the value is higher.

tldr: a Counter value can only be used as, well, a counter, while an uint256's usage needs to be restricted to fit this behavior. Variables such as _ownedTokensCount are counts, and Counter is therefore better suited there.

If you still want to keep it this way, I'd rather have an inline comment in the struct definition (in Counter.sol) saying that the value is not to be modified directly, but just through increment and decrement. Also, I'd explicitly write a comment in the increment function explaining why it does not use SafeMath.

I had updated the documentation, but looks like I forgot to commit that 😛

Ah, and you should move the test file out of the drafts folder stuck_out_tongue

Thanks!

[frangio]

I agree with @tinchoabbate's observation that "if it can be done it will be done", so I would keep the library in drafts until we have opaque structs.

[nventuro]

Agreed, I moved it back to drafts.

[frangio]

LGTM. Thanks!