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

Add ERC827 Token #518

Merged
merged 14 commits into from Jan 17, 2018
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions contracts/mocks/ERC827TokenMock.sol
@@ -0,0 +1,15 @@
pragma solidity ^0.4.13;


import '../token/ERC827.sol';


// mock class using ERC827 Token
contract ERC827TokenMock is ERC827 {

function ERC827TokenMock(address initialAccount, uint256 initialBalance) {
balances[initialAccount] = initialBalance;
totalSupply = initialBalance;
}

}
25 changes: 25 additions & 0 deletions contracts/mocks/MessageHelper.sol
@@ -0,0 +1,25 @@
pragma solidity ^0.4.11;

contract MessageHelper {

event Show(bytes32 b32, uint256 number, string text);

function showMessage(
bytes32 message, uint256 number, string text
) returns (bool) {
Show(message, number, text);
return true;
}

function fail() {
throw;
}

function call(address to, bytes data) returns (bool) {
if (to.call(data))
return true;
else
return false;
}

}
125 changes: 125 additions & 0 deletions contracts/token/ERC827.sol
@@ -0,0 +1,125 @@
pragma solidity ^0.4.13;

import "./StandardToken.sol";

/**
@title ERC827, an extension of ERC20 token standard

Implementation the ERC827, following the ERC20 standard with extra
methods to transfer value and data and execute calls in transfers and
approvals.
Uses OpenZeppelin StandardToken.
*/
contract ERC827 is StandardToken {

/**
@dev Addition to ERC20 token methods. It allows to
approve the transfer of value and execute a call with the sent data.

Beware that changing an allowance with this method brings the risk that
someone may use both the old and the new allowance by unfortunate
transaction ordering. One possible solution to mitigate this race condition
is to first reduce the spender's allowance to 0 and set the desired value
afterwards:
https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729

@param _spender The address that will spend the funds.
@param _value The amount of tokens to be spent.
@param _data ABI-encoded contract call to call `_to` address.

@return true if the call function was executed successfully
*/
function approve(address _spender, uint256 _value, bytes _data) public returns (bool) {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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

require(_spender != address(this));

super.approve(_spender, _value);

require(_spender.call(_data));

return true;
}

/**
@dev Addition to ERC20 token methods. Transfer tokens to a specified
address and execute a call with the sent data on the same transaction

@param _to address The address which you want to transfer to
@param _value uint256 the amout of tokens to be transfered
@param _data ABI-encoded contract call to call `_to` address.

@return true if the call function was executed successfully
*/
function transfer(address _to, uint256 _value, bytes _data) public returns (bool) {
require(_to != address(this));

super.transfer(_to, _value);

require(_to.call(_data));
return true;
}

/**
@dev Addition to ERC20 token methods. Transfer tokens from one address to
another and make a contract call on the same transaction

@param _from The address which you want to send tokens from
@param _to The address which you want to transfer to
@param _value The amout of tokens to be transferred
@param _data ABI-encoded contract call to call `_to` address.

@return true if the call function was executed successfully
*/
function transferFrom(address _from, address _to, uint256 _value, bytes _data) public returns (bool) {
require(_to != address(this));

super.transferFrom(_from, _to, _value);

require(_to.call(_data));
return true;
}

/**
* @dev Addition to StandardToken methods. Increase the amount of tokens that
* an owner allowed to a spender and execute a call with the sent data.
*
* approve should be called when allowed[_spender] == 0. To increment
* allowed value is better to use this function to avoid 2 calls (and wait until
* the first transaction is mined)
* From MonolithDAO Token.sol
* @param _spender The address which will spend the funds.
* @param _addedValue The amount of tokens to increase the allowance by.
* @param _data ABI-encoded contract call to call `_spender` address.
*/
function increaseApproval(address _spender, uint _addedValue, bytes _data) public returns (bool) {
require(_spender != address(this));

super.approve(_spender, _addedValue);
Copy link
Contributor

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

Copy link
Contributor

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?


require(_spender.call(_data));

return true;
}

/**
* @dev Addition to StandardToken methods. Decrease the amount of tokens that
* an owner allowed to a spender and execute a call with the sent data.
*
* approve should be called when allowed[_spender] == 0. To decrement
* allowed value is better to use this function to avoid 2 calls (and wait until
* the first transaction is mined)
* From MonolithDAO Token.sol
* @param _spender The address which will spend the funds.
* @param _subtractedValue The amount of tokens to decrease the allowance by.
* @param _data ABI-encoded contract call to call `_spender` address.
*/
function decreaseApproval(address _spender, uint _subtractedValue, bytes _data) public returns (bool) {
require(_spender != address(this));

super.decreaseApproval(_spender, _subtractedValue);

require(_spender.call(_data));

return true;
}

}
Copy link
Contributor

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. 👏

Copy link
Contributor Author

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 🎉