-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
ERC 865 #741
ERC 865 #741
Conversation
Hello Ludovic :) Considering the |
There seems to be two potential paths:
I lean towards including it, for consistency, but at the end of the day, it's not critical (especially if increaseApproval can achieve the same functionality). What do others think? |
Let's review the comments in ethereum/EIPs#865, see if there are any major objections or issues, and merge. @martriay can you handle this, as you're familiar with the code? |
The problem with the current implementation is in my opinion that we have a second implementation of Why not modify function transfer(address _to, uint256 _value) public returns (bool) {
transfer(msg.sender, _to, _value);
}
//new function.
function transfer(address _from, address _to, uint256 _value) internal returns (bool) {
require(_to != address(0));
require(_value <= balances[_from]);
// SafeMath.sub will throw if there is not enough balance.
balances[_from] = balances[_from].sub(_value);
balances[_to] = balances[_to].add(_value);
Transfer(_from, _to, _value);
return true;
} In function transferPreSigned(
bytes _signature,
address _to,
uint256 _value,
uint256 _fee,
uint256 _nonce
)
public
returns (bool)
{
require(signatures[_signature] == false);
bytes32 hashedTx = transferPreSignedHashing(address(this), _to, _value, _fee, _nonce);
address from = recover(hashedTx, _signature);
require(from != address(0));
require(transfer(from, _to, _value));
require(transfer(from, msg.sender, _fee));
TransferPreSigned(from, _to, msg.sender, _value, _fee);
return true;
} The downside would be a bit more GAS usage. In addition I like the idea of GAS free transactions. However |
balances[msg.sender] = balances[msg.sender].add(_fee); | ||
signatures[_signature] = true; | ||
|
||
Approval(from, _spender, _subtractedValue); |
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 guess Approval(from, _spender, allowed[from][_spender]);
is correct (not _subtractedValue
).
signatures[_signature] = true; | ||
|
||
Transfer(_from, _to, _value); | ||
Transfer(spender, msg.sender, _fee); |
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.
Does not it require to emit TransferFromPreSigned
or TransferPreSigned
event?
Hi, I am very interested in ERC865. |
pure | ||
returns (bytes32) | ||
{ | ||
/* "48664c16": transferPreSignedHashing(address,address,address,uint256,uint256,uint256) */ |
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.
one address to much? should be "15420b71" with transferPreSignedHashing(address,address,uint256,uint256,uint256). On the other hand, as long as the off-chain code uses the same number its fine.
I agree with some of the reservations about duplication noted by @kabl above. I wonder if an attempt to closely mirror the ERC20 functions leads the code off-target a bit in terms of its functional objectives (at least as I understand them). What I mean by that is the approve and transferFrom pattern makes sense within the context of ERC20 tokens, where we have a "pure" decentralized and fully on-chain implementation; but perhaps for this proposal it is better to more closely mirror the legacy processes that transferFrom was devised to emulate with the help of some off-chain functionality (hear me out). For instance, when I authorize my credit card to debit my bank account for the amount of my monthly minimum payment, I obviously don't have to do anything like what's done with approve. Again, that process flow makes sense within the context of ERC20's approve and transferFrom functions, but one of the great potentials of ERC865 is to create a more user-friendly experience for novice and/or mainstream users (or those who simply don't want to hold Ether in their wallet). So, within the context of ERC865, I would (humbly) suggest that a basic version of ERC865 reduce the amount of duplication from ERC20 and focus on the main objective to enable Ether-free token transfers. I'll sketch out below a very high-level overview of how only relying on transferPreSigned can be effectively and securely used with less friction for the user and less duplication of code. Going with the example of a recurring bill payment, we can potentially bring in the best of both worlds here, by automating recurring payments but still requiring the user "approve" each transfer prior to execution. In this way, you can simulate transferFrom without bloating the code base. Let's imagine the following takes place through a browser or mobile interface:
I've followed the same basic pattern here for a different use case and I've also included tests. Thoughts? |
I agree with @jsdavis28, I'd like to see a more general solution to this as opposed to something tightly coupled to ERC20. That said, the EIP is still a draft, and doesn't seem very active. Closing due to staleness. |
if (v != 27 && v != 28) { | ||
return (address(0)); | ||
} else { | ||
return ecrecover(hash, v, r, s); |
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.
Is it intentional, that the sig[] contains the signature in different order than v, r, s, which is a scientific custom, and for example used by ecrecover()?
Hello there,
I wanted to submit an implementation proposal for EIP865 (described here: ethereum/EIPs#865)
If you are in favor of having that token part of the repo, I''ll work on importing and adding more tests to the PR :).
Best,
Ludovic