Consolidated ERC20 Interface and Implementation Files#1125
Conversation
There was a problem hiding this comment.
This is great work @dougiebuckets, thanks! There's some minor stuff that needs to be addressed before merging, but this is in quite a good place!
Also, looks like some tests (like CanReclaimToken's) use BasicTokenMock: we should migrate them to use StandardTokenMock.
| * @dev Adds burnFrom method to ERC20 implementations | ||
| */ | ||
| contract StandardBurnableToken is BurnableToken, StandardToken { | ||
| contract StandardBurnableToken is StandardToken, BurnableToken { |
There was a problem hiding this comment.
Since BurnableToken now inherits from StandardToken (and therefore has the allowed state variable, shouldn't we merge this contract with BurnableToken`?
|
|
||
| // ERC20 basic token contract being held | ||
| ERC20Basic public token; | ||
| StandardToken public token; |
There was a problem hiding this comment.
Shouldn't this (and all other references to the type in this contract) be an ERC20?
| const { assertRevert } = require('../../helpers/assertRevert'); | ||
| const BasicToken = artifacts.require('BasicTokenMock'); | ||
|
|
||
| contract('StandardToken', function ([_, owner, recipient, anotherAccount]) { |
There was a problem hiding this comment.
We shouldn't delete the BasicToken tests, rather, they should be added to the StandardToken test suite.
There was a problem hiding this comment.
(don't be confused by the fact that this test was wrongly named StandardToken)
There was a problem hiding this comment.
@nventuro Thanks for the feedback! Now that BasicToken has been merged w/ StandardToken, I went through BasicToken.test.js and noticed the exact same units tests are already in StandardToken.test.js.
Am I missing something?
There was a problem hiding this comment.
No, you're right, I didn't notice the tests were the same. Thanks for removing that code duplication :)
|
By the way, please try to address review comments by committing on top of your branch instead of rebasing: that way, it's much easier for us to figure out what changed in each commit. Thanks :) |
…rits from StandardToken; Fixed TokenTimelock so it uses SafeERC20 for ERC20
|
|
||
| constructor( | ||
| ERC20Basic _token, | ||
| StandardToken _token, |
There was a problem hiding this comment.
Good catch! ty
34bd333 to
57fb04e
Compare
57fb04e to
cc0b44e
Compare
|
@nventuro I'm not sure why I'm still getting this conflict? I've merged my branch w/ master in order to get everything up-to-date (including the updates to HasNoTokens and ReentrancyGuard and I've removed BasicToken.) Any thoughts? Caveat: My git skills could definitely use some work :) |
|
Looks like you managed to solve it though! Maybe GitHub's UI took a while to update? There's a small linter error causing the build to fail (you need to add a newline character at the end of |
frangio
left a comment
There was a problem hiding this comment.
Great work @dougiebuckets! Thanks!
|
Awesome! @dougiebuckets let me care of the merge conflicts and we'll merge this, thanks again! |
|
Thanks for your help :) |
Fixes #1098
🚀 Description
Went ahead and consolidated ERC20 interfaces and implementation files in an effort to simply OZ's contract offerings for developer/user experience purposes.
npm run lint:all:fix).