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

ERC 865 #741

Closed
wants to merge 1 commit into from
Closed

ERC 865 #741

wants to merge 1 commit into from

Conversation

lgalabru
Copy link

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

@martriay
Copy link
Contributor

martriay commented Feb 14, 2018

Hello Ludovic :)

Considering the approve/transferFrom pattern risks, what do you think of removing the approvePreSigned function from it?

@ptrwtts
Copy link

ptrwtts commented Feb 15, 2018

There seems to be two potential paths:

  1. Have a presigned version of all ERC20 transfer-related functions
  2. Exclude the presigned approve(), so developers aren't tempted to use a bad pattern

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?

@spalladino
Copy link
Contributor

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?

@martriay
Copy link
Contributor

martriay commented May 4, 2018

The code looks good to me. @lgalabru @ptrwtts would you mind updating the base branch and adding some tests?

@kabl
Copy link

kabl commented Sep 4, 2018

The problem with the current implementation is in my opinion that we have a second implementation of transfer(...), transferFrom(...) and all the other functions. Lot of code duplication which would need to be covered with tests. Which would need to be updated in ERC865Token whenever a modification in BasicToken is.

Why not modify BasicToken.sol as follow:

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 ERC865Token.sol

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 transferPreSigned and transferFromPreSigned could be good enough. To have a PreSigned function for every smart contract function will bloat up the implementation and also the contract code which needs to be deployed.

balances[msg.sender] = balances[msg.sender].add(_fee);
signatures[_signature] = true;

Approval(from, _spender, _subtractedValue);

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);

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?

@wagyu298
Copy link

Hi, I am very interested in ERC865.
I read this pull request and I commented some problems above.
Best,

pure
returns (bytes32)
{
/* "48664c16": transferPreSignedHashing(address,address,address,uint256,uint256,uint256) */
Copy link
Contributor

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.

@jsdavis28
Copy link

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:

  1. The user selects a payment method (wallet address) and an option for automated payments (e.g. monthly minimum payment).
  2. At the time of payment processing, the app generates a message containing the required params.
  3. The app prompts the user to approve the payment.
  4. The user "approves" the payment by signing the transaction.
  5. The app sends the signed message to a delegate to "transferFrom" on behalf of the token holder by calling transferPreSigned.

I've followed the same basic pattern here for a different use case and I've also included tests.

Thoughts?

@tbocek tbocek mentioned this pull request Jan 23, 2019
@nventuro
Copy link
Contributor

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.

@nventuro nventuro closed this Feb 28, 2019
if (v != 27 && v != 28) {
return (address(0));
} else {
return ecrecover(hash, v, r, s);

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()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.