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

StandardToken encapsulation #1197

Merged
merged 13 commits into from Aug 16, 2018

Conversation

@frangio
Copy link
Member

commented Aug 12, 2018

Fixes #1097.
Fixes #1208.

Makes all state variables of StandardToken private, and adds internal functions to modify them correctly emitting events.

@frangio frangio requested a review from nventuro Aug 12, 2018
@frangio frangio self-assigned this Aug 12, 2018
emit Mint(_to, _amount);
emit Transfer(address(0), _to, _amount);
_mint(_to, _amount);

This comment has been minimized.

Copy link
@nventuro

nventuro Aug 12, 2018

Member

Shouldn't the event be emitted after the action took place, according to our guideline?

This comment has been minimized.

Copy link
@frangio

frangio Aug 12, 2018

Author Member

Ah, yes. I initially did it like this because the test was relying on the previous order: Mint followed by Transfer. I'll fix the test.

@frangio

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2018

Addressed previous comments and added tests for the new functions. @nventuro Please review again! 🙂

Copy link
Member

left a comment

I noticed you skipped some of the requires for which we already have corresponding ones (e.g. transferFrom and burnFrom), leaving SafeMath as the only line of defense. Is this something we want to do?


balances[_who] = balances[_who].sub(_value);
totalSupply_ = totalSupply_.sub(_value);
super._burn(_who, _value);

This comment has been minimized.

Copy link
@nventuro

nventuro Aug 13, 2018

Member

This is confusing: the public burn function calls the internal _burn function, which then calls super._burn and emits an event. Why not remove the internal _burn and just call base function from the public one?

edit: you did it right in the mint function, so I'm assuming this wasn't intentional :)

This comment has been minimized.

Copy link
@frangio

frangio Aug 14, 2018

Author Member

I did this because both burn and burnFrom need to add the Burn event. I felt it was better than adding the event in both. What do you think?

This comment has been minimized.

Copy link
@nventuro

nventuro Aug 14, 2018

Member

That's even worse now that I realize it lol. burnFrom calls _burnFrom, which calls the overriden _burn, which calls the original burn: unless you know the details of StandardToken, you can't tell that burnFrom will emit a Burn event. The reason why you did it makes sense though.

I think the problem is the missing comment: if the overridden function said something like 'adds the Burn event when burning', it may read more clearly.

* @param _amount The amount that will be created.
*/
function _mint(address _account, uint256 _amount) internal {
totalSupply_ = totalSupply_.add(_amount);

This comment has been minimized.

Copy link
@nventuro

nventuro Aug 13, 2018

Member

Input validation on amount > 0 and account != 0?

This comment has been minimized.

Copy link
@frangio

frangio Aug 14, 2018

Author Member

_amount > 0 is a validation that we have decided in the past to not do, because it forces users of the function to special-case amount == 0 to avoid triggering a revert.

_account != 0 I guess we should add.

* @param _amount The amount that will be burnt.
*/
function _burn(address _account, uint256 _amount) internal {
totalSupply_ = totalSupply_.sub(_amount);

This comment has been minimized.

Copy link
@nventuro

nventuro Aug 13, 2018

Member

Input validation on amount > 0, account != 0, and balances[account] > amount?

This comment has been minimized.

Copy link
@frangio

frangio Aug 14, 2018

Author Member

I'm not sure about balances[account] > amount. I thought we were using SafeMath for this kind of thing, now that it uses require.

Is this something we want to do?

I'm not sure.

function _burnFrom(address _account, uint256 _amount) internal {
// Should https://github.com/OpenZeppelin/zeppelin-solidity/issues/707 be accepted,
// this function needs to emit an event with the updated approval.
allowed[_account][msg.sender] = allowed[_account][msg.sender].sub(_amount);

This comment has been minimized.

Copy link
@nventuro

nventuro Aug 13, 2018

Member

Input validation on amount > 0, account != 0, balances[account] > amount and allowance[acount][msg.sender] > amount?

@barakman

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2018

Are you sure you want to declare private stuff in your contracts?
They are generally inherited by (I assume) every user out there, so internal feels much more appropriate here (in any any other base contract for that matter).

@nventuro

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

@barakman if the variables are private, auditing becomes that much easier, because you can be sure all accesses to it are restricted to the current contract. Additionally, it prevents accidental mistakes (e.g. adding balance to a user but forgetting to update totalSupply). See how the _mint and _burn internal functions give access to that functionality, but in a safe manner.

@frangio

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2018

@nventuro Fixed new comments. I added in all the checks for balance and allowance, though I'd consider removing all those in a different issue.

I'm requesting another review since this is a breaking change.

@frangio frangio requested a review from shrugs Aug 14, 2018
@frangio frangio added this to the v2.0 milestone Aug 14, 2018
@frangio frangio referenced this pull request Aug 15, 2018
4 of 4 tasks complete
* @param _amount The amount that will be burnt.
*/
function _burnFrom(address _account, uint256 _amount) internal {
require(_account != 0);

This comment has been minimized.

Copy link
@shrugs

shrugs Aug 15, 2018

Contributor

this line and the last require are duplicate assertions since we're falling back to _burn. Would save some gas, presumably, if we omit them but mention this in a comment.

const mintEvent = expectEvent.inLogs(logs, 'Mint', {
to: owner,
});
mintEvent.args.amount.should.be.bignumber.equal(amount);

This comment has been minimized.

Copy link
@shrugs

shrugs Aug 15, 2018

Contributor

you can actually put that in the args object as well:

const mintEvent = expectEvent.inLogs(logs, 'Mint', {
  to: owner,
  amount,
});

and it will do the bignumber equals check. likewise below

This comment has been minimized.

Copy link
@frangio

frangio Aug 15, 2018

Author Member

Are you sure about this? The code for the helper looks like it uses JavaScript's equality operator.

https://github.com/OpenZeppelin/openzeppelin-solidity/blob/8d11dcc0e5fb0fd50e018d7786750fa45e54d4c5/test/helpers/expectEvent.js#L6-L9

This was actually one of the items @nventuro mentioned in #1026.

@shrugs

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2018

@frangio

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2018

Addressed all comments! Can I get a ✔️? 😬

Copy link
Contributor

left a comment

LGTM

Copy link
Member

left a comment

Could you double check CappedToken.behavior.js? It looks like there may be whitespace changes on that one (probably the line endings).

@frangio frangio dismissed stale reviews from nventuro and shrugs via 24cb9cf Aug 16, 2018
@frangio frangio force-pushed the frangio:erc20-encapsulation branch from 51ca533 to 24cb9cf Aug 16, 2018
@frangio

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2018

@nventuro You were right. I had to force push that last merge commit which dismissed the reviews.

@frangio

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2018

Thanks @nventuro and @shrugs! 😄

@frangio frangio merged commit 4dcdd29 into OpenZeppelin:master Aug 16, 2018
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 97.84%
Details
@frangio frangio deleted the frangio:erc20-encapsulation branch Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.