Skip to content

Conversation

@k06a
Copy link
Contributor

@k06a k06a commented Oct 1, 2018

🚀 Description

Allow ERC20 subclasses to perform transfers with own rules. For example by providing senders signature or something else.

Just extracted common transfer and transferFrom code into internal _transfer method.

  • 📘 I've reviewed the OpenZeppelin Contributor Guidelines
  • ✅ I've added tests where applicable to test my new functionality.
  • 📖 I've made sure that my contracts are well-documented.
  • 🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:fix).

@nventuro nventuro added feature New contracts, functions, or helpers. contracts Smart contract code. labels Oct 1, 2018
@nventuro nventuro requested a review from frangio October 1, 2018 14:55
Copy link
Contributor

@nventuro nventuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I like this! I wonder why we didn't do it before?

Note that this will become part of our public API, and as such will need to wait until the next minor release.

@frangio
Copy link
Contributor

frangio commented Oct 2, 2018

Yeah I think this makes a lot of sense to have.

@k06a I'm curious, have you run into the need to use this?

@k06a
Copy link
Contributor Author

k06a commented Oct 2, 2018

@frangio @nventuro I browsed the latest version of ERC20.sol and decided this API was missed. Some token creators would like to provide token transfers based on holder signatures for example.

A while ago I developed the library for this: https://github.com/bitclave/Feeless. Do you think this kind of functionality could be included in openzeppelin-solidity library?

@k06a
Copy link
Contributor Author

k06a commented Oct 2, 2018

@frangio @nventuro what do you guys think of this line?

require(value <= _balances[from]);

I am sure this overflow check is already exist in this line:

_balances[from] = _balances[from].sub(value);

It seems we can save some gas.

@frangio
Copy link
Contributor

frangio commented Oct 2, 2018

The redundancy in those lines has come up before, but if you feel we need to revisit the discussion please open a different issue.

Feeless is super interesting! I'm interested in providing stuff like that in OpenZeppelin in the future, but I personally don't see it in the roadmap very soon.

Thanks @k06a!

@frangio frangio merged commit 43ebb4f into OpenZeppelin:master Oct 2, 2018
nventuro pushed a commit that referenced this pull request Oct 4, 2018
@ardagokcek
Copy link

ardagokcek commented Oct 13, 2018

@frangio I think line 98 must be after line 99. When someone edits the _transfer function and doesn't uses require or revert, then line 98 will decrease the allowence but line 99 won't make the transfer.

@k06a
Copy link
Contributor Author

k06a commented Oct 13, 2018

@ardagokcek I think changing this lines order will not help in this case. You mean someone overrides _transfer? It should revert in case of failure. I used this order of lines because of order of lines 207-209.

@ardagokcek
Copy link

ardagokcek commented Oct 13, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contracts Smart contract code. feature New contracts, functions, or helpers.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants