-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
Add ERC827 Token #518
Add ERC827 Token #518
Conversation
4b2e2d0
to
509743b
Compare
add missing public identifier in approveData in SmartToken contract remove constact from showMessage function in message helper contract move Message helper contract to mocks folder move SmartTokenMock contract to mocks folder
509743b
to
4ecdf31
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work. I left a few comments, the most important one on whether we want to support _data
versions of increase and decrease approval.
Also: should we have an interface
version of the contract, as we are doing with ERC20?
contracts/token/ERC827.sol
Outdated
|
||
@return true if the call function was executed successfully | ||
*/ | ||
function approve(address _spender, uint256 _value, bytes _data) public returns (bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we have increase and decrease approval methods in ERC20, should we add the same to ERC827?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
increase and decrease approval mitigate the race condition in the approve method right?
I think it makes sense to add it if it solves a problem in the standard, but if we add it we should add it with the _data
parameter too. To follow the same idea for the rest of the functions, that you can call it with or without _data
.
Lets propose it in the ERC827 standard for now and bring the discussion there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we should add it as part of the standard. Today, the increase and decrease approval (which help mitigate the approve race condition, as you say) are present in our ERC20 implementation, but are not part of the interface or standard.
I'd just add the _data
versions of them for completeness here, and keep that out of the standard for the time being.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I agree, lets do that, Im going to add them
test/ERC827Token.js
Outdated
// Use method #3 approve of the abi to encode the data tx | ||
const approveData = ethjsABI.encodeMethod(token.abi[3], | ||
[message.contract.address, 100, extraData] | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that the encoding must be done manually due to truffle's limitations for function overloading. That being said, I'd rather find
the desired function from the ABI array rather than relying on the ordering, as it is quite brittle; if we add another function later, these tests may break (or worse, may inadvertently test other functions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right! im going to change it :)
contracts/token/ERC827.sol
Outdated
return true; | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't believe how simple the code for this contract ended up. Congrats for the awesome work. 👏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks to community and maintainers 🎉
@spalladino how do you think we should have the interface?
|
Sounds good! @frangio WDYT? |
contracts/token/ERC827.sol
Outdated
function increaseApproval(address _spender, uint _addedValue, bytes _data) public returns (bool) { | ||
require(_spender != address(this)); | ||
|
||
super.approve(_spender, _addedValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be super.increaseApproval
, not approve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit worried that the tests did not catch this one. Maybe make sure that the increase approval test works on a spender that already has a certain approval set?
contracts/token/ERC827Token.sol
Outdated
function increaseApproval(address _spender, uint _addedValue, bytes _data) public returns (bool) { | ||
require(_spender != address(this)); | ||
|
||
super.approve(_spender, _addedValue); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems I made a comment on this line on a previous commit and github is now showing it as on an outdated diff. Copying it here just in case it goes missing:
Should be super.increaseApproval, not approve
I'm a bit worried that the tests did not catch this one. Maybe make sure that the increase approval test works on a spender that already has a certain approval set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my bad! Yes, it is weird that the tests are still passing, im going to add a check before increase/decrease approval with data are executed.
LGTM! @facuspagnuolo what have we agreed about naming of interfaces vs impls? Is |
@spalladino yes! I followed that same approach |
Add ERC827 Token
Hi all, if I try to use the increaseApproval methods (the one without _data) I receive the following error. Error: Invalid number of arguments to Solidity function. Any Idea? Update: it seems that web3 (0.20.6) is not able to return right method if overloaded with different params number. |
Fix #494