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 unnecessary SafeMath call #1610

Merged
merged 11 commits into from
Jan 21, 2019

Conversation

nventuro
Copy link
Contributor

@nventuro nventuro commented Jan 16, 2019

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.

@nventuro nventuro added feature New contracts, functions, or helpers. improvement contracts Smart contract code. labels Jan 16, 2019
@nventuro nventuro added this to the v2.2 milestone Jan 16, 2019
@nventuro nventuro requested a review from frangio January 16, 2019 19:50
@nventuro nventuro changed the title Remove safe math Remove unnecessary SafeMath call Jan 16, 2019
Co-Authored-By: nventuro <nicolas.venturo@gmail.com>
@nventuro nventuro self-assigned this Jan 16, 2019
Copy link
Contributor

@tinchoabbate tinchoabbate left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

nventuro commented Jan 16, 2019

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
Copy link
Contributor

frangio commented Jan 17, 2019

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
Copy link
Contributor Author

Agreed, I moved it back to drafts.

frangio
frangio previously approved these changes Jan 21, 2019
Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@nventuro nventuro merged commit 07603d5 into OpenZeppelin:master Jan 21, 2019
@nventuro nventuro deleted the remove-safe-math branch January 21, 2019 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contracts Smart contract code. feature New contracts, functions, or helpers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants