Allow ERC20 minting from owner signature

[SylTi]

🚀 Description

This allows minting from any address as long as they have a valid signed message from the owner.
We discussed adding something like this with @shrugs in #928

• 📘 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:all:fix).

[SylTi]

Looks like I missed the broken RBACMintableToken tests, I was only running truffle test test/token/ERC20/MintableToken.test.js. I didn't saw the dependency. Sorry, my bad.

[shrugs]

Do you think it'd be better to abstract the minting signature logic using the pattern in #950 ? that way anyone with the minter permission could mint tokens, and if that happens to be a signature bouncer, that's cool too.

also, for usedSignatures, I was able to get the same behavior down to storing a single nonce per-address; you can see how it's used in the ERC721Minter contract from that PR using NonceTracker.

[SylTi]

Hey @shrugs thanks for the review 👍

Concerning the usage of SignatureBouncer/RBAC, I thought about it, but I didn't want to change the way MintableToken worked.
MintableToken is only allowing 1 owner/minter. I wanted to keep that behavior. If we want the same functionality but with multiple minters maybe we should also implement it inside RBACMintableToken instead?

For the nonce, I agree it should be a little more efficient, but I don't know about using NonceTracker it seems relatively complex when I could simply do something like:

require(_nonce == usedNonce[_to].add(1));
...
usedNonce[_to] = _nonce.add(1);

This seems more optimized, what do you think?

[shrugs]

I'm down to keep it as Ownable and the single minter, yeah 👍

I do think the minting signature logic should either be composed or inherited with the MintableToken, instead of hardcoded. I may want to use MintableToken without verifying sigatures or anything.

Actually, this is basically an Airdrop, but checked by signature bouncer (and now I'm remembering that we talked about this in #928), so it should definitely be abstracted from the token itself. I'd much prefer a composable approach like we've structured the crowdsales.

So we can have different access-control layers:

• BouncerAirdrop
• WhitelistedAirdrop

And different emission layers:

• Airdrop (uses balance)
• AllowanceAirdrop
• MintableAirdrop

[SylTi]

Updated. What do you think about this version @shrugs ?

[shrugs]

You can replace most of the code in the Airdropper contract with

contract Aidropper is SignatureBouncer {

function mint(address _to, uint256 _amount, bytes _sig) onlyValidSignatureAndData {
  token.mint(_to,_amount);
}

}

[SylTi]

If you want multiple signers sure. If you want a single one, you end up paying higher deployment cost and a gas premium on every single minting.
mapping (string => mapping (address => bool)) private roles; vs address owner

[shrugs]

aye, but you're only storing one role, the delegate role for whomever is issuing the signatures. I'd expect that it's negligible, but I suppose it'd be worth profiling. I'll do a test

[shrugs]

For deploying, my airdrop using Bouncer costs 1555981 and this contract costs 1011145, a difference of 544k gas.

For minting, there's a 10k gas difference probably due to the extra if condition in Bouncer that supports contract delegates.

So deployment does have non-negligible gas costs if you inherit directly from Bouncer, although its hard to tell exactly where that gas cost is coming from.

[shrugs]

That said, I'm not convinced that duplicating code here, as opposed to following an existing pattern and library designed for this feature is a great idea for OZ itself. It could definitely make sense for a specific project, though, but once you start caring about $2 in deployment costs, you should have the bandwidth to build your own minimal bouncer-style contract.

[shrugs]

this is what ERC20Airdrop looks like, by the way: https://github.com/Shrugs/zeppelin-solidity/blob/feat%2Fairdrops/contracts/airdrop/ERC20/ERC20Airdrop.sol

[SylTi]

Thanks for taking the time!
I completely agree with you on the fact the $2 is almost a negligible cost for a single deployment and might not be worth breaking DRY for OZ.

but if we replace this contract by your example I would argue this is probably not a great fit for the library directly either, maybe as an example of possibilities (like we were discussing about RBACMintabletoken)

Maybe creating a SignatureUtils that allow the decoupling of SignatureBouncer utilities functions from RBAC making them agnostic to the type of the access control would be a just middle, allowing for example of each Airdroper types (single signer or rbac) to be created easily while respecting DRY principles?

Now that being said let's try to put some perspective on this and future possibilities and let me know if this makes any sense to you:

• It put unneeded data onchain that will need to be stored by every node, forever, for every deployment. It's basically a burden that will need to be supported by the network.
• The 2$ cost increase is only considering current price of ether, what happens in the next bull market if we 10x the value of ether at its peak (1400 x 10)? Now that's $60 lost by deployment.
• The goal of OZ and ethereum, in general, is to be widely used if the contract gets deployed 10 000 times, at current price that's $20 000 lost by the ecosystem, if we take the $60 figure from before now that's $1 200 000 lost by the ecosystem, not so small anymore.

Or maybe that just too much speculation? 🤔

[shrugs]

I pushed some updates to the #1024 branch, adding BouncerUtils; you should be able to do something like

 * modifier onlyValidTransaction() {
 *  require(
 *    BouncerUtils.signerOfMessageData(address(this)) == owner,
 *    "invalid signature"
 *  );
 * }
 *
 * function mint(address _to, uint256 _amount, bytes _sig)
 *   onlyValidTransaction()
 *   public
 * {
 *   // trust arguments
 *   MyToken.mint(_to, _amount);
 * }

let me know if that's what you were expecting

[shrugs]

closing in favor of tracking via #1272