Remove SignatureBouncer from drafts (#1879)

* Remove SignatureBouncer

* Update changelog entry

* Fix coverage

* Update CHANGELOG.md

CHANGELOG.md

@@ -10,7 +10,8 @@
  * `Address.isContract`: switched from `extcodesize` to `extcodehash` for less gas usage. ([#1802](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1802))
  * `SafeMath`: added custom error messages support for `sub`, `div` and `mod` functions. `ERC20` and `ERC777` updated to throw custom errors on subtraction overflows. ([#1828](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1828))
 
-### Bugfixes
+### Breaking changes in drafts:
+ * `SignatureBouncer` has been removed from the library, both to avoid confusions with the GSN Bouncers and `GSNBouncerSignature` and because the API was not very clear. ([#1879](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/1879))
 
 ## 2.3.0 (2019-05-27)
 

contracts/mocks/SignatureBouncerMock.sol

@@ -1,50 +0,0 @@
-pragma solidity ^0.5.0;
-
-import "../drafts/SignatureBouncer.sol";
-import "./SignerRoleMock.sol";
-
-contract SignatureBouncerMock is SignatureBouncer, SignerRoleMock {
-    function checkValidSignature(address account, bytes memory signature)
-        public view returns (bool)
-    {
-        return _isValidSignature(account, signature);
-    }
-
-    function onlyWithValidSignature(bytes memory signature)
-        public onlyValidSignature(signature) view
-    {
-        // solhint-disable-previous-line no-empty-blocks
-    }
-
-    function checkValidSignatureAndMethod(address account, bytes memory signature)
-        public view returns (bool)
-    {
-        return _isValidSignatureAndMethod(account, signature);
-    }
-
-    function onlyWithValidSignatureAndMethod(bytes memory signature)
-        public onlyValidSignatureAndMethod(signature) view
-    {
-        // solhint-disable-previous-line no-empty-blocks
-    }
-
-    function checkValidSignatureAndData(address account, bytes memory, uint, bytes memory signature)
-        public view returns (bool)
-    {
-        return _isValidSignatureAndData(account, signature);
-    }
-
-    function onlyWithValidSignatureAndData(uint, bytes memory signature)
-        public onlyValidSignatureAndData(signature) view
-    {
-        // solhint-disable-previous-line no-empty-blocks
-    }
-
-    function theWrongMethod(bytes memory) public pure {
-        // solhint-disable-previous-line no-empty-blocks
-    }
-
-    function tooShortMsgData() public onlyValidSignatureAndData("") view {
-        // solhint-disable-previous-line no-empty-blocks
-    }
-}

test/cryptography/ECDSA.test.js

@@ -14,6 +14,18 @@ contract('ECDSA', function ([_, other]) {
     this.ecdsa = await ECDSAMock.new();
   });
 
+  context('recover with invalid signature', function () {
+    it('with short signature', async function () {
+      expect(await this.ecdsa.recover(TEST_MESSAGE, '0x1234')).to.equal(ZERO_ADDRESS);
+    });
+
+    it('with long signature', async function () {
+      // eslint-disable-next-line max-len
+      expect(await this.ecdsa.recover(TEST_MESSAGE, '0x01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'))
+        .to.equal(ZERO_ADDRESS);
+    });
+  });
+
   context('recover with valid signature', function () {
     context('with v0 signature', function () {
       // Signature generated outside ganache with method web3.eth.sign(signer, message)

test/drafts/SignatureBouncer.test.js

@@ -1,223 +0,0 @@
-const { expectRevert } = require('openzeppelin-test-helpers');
-const { getSignFor } = require('../helpers/sign');
-const { shouldBehaveLikePublicRole } = require('../behaviors/access/roles/PublicRole.behavior');
-
-const { expect } = require('chai');
-
-const SignatureBouncerMock = artifacts.require('SignatureBouncerMock');
-
-const UINT_VALUE = 23;
-const BYTES_VALUE = web3.utils.toHex('test');
-const INVALID_SIGNATURE = '0xabcd';
-
-contract('SignatureBouncer', function ([_, signer, otherSigner, other, authorizedUser, ...otherAccounts]) {
-  beforeEach(async function () {
-    this.sigBouncer = await SignatureBouncerMock.new({ from: signer });
-    this.signFor = getSignFor(this.sigBouncer, signer);
-  });
-
-  describe('signer role', function () {
-    beforeEach(async function () {
-      this.contract = this.sigBouncer;
-      await this.contract.addSigner(otherSigner, { from: signer });
-    });
-
-    shouldBehaveLikePublicRole(signer, otherSigner, otherAccounts, 'signer');
-  });
-
-  describe('modifiers', function () {
-    context('plain signature', function () {
-      it('allows valid signature for sender', async function () {
-        await this.sigBouncer.onlyWithValidSignature(await this.signFor(authorizedUser), { from: authorizedUser });
-      });
-
-      it('does not allow invalid signature for sender', async function () {
-        await expectRevert(
-          this.sigBouncer.onlyWithValidSignature(INVALID_SIGNATURE, { from: authorizedUser }),
-          'SignatureBouncer: invalid signature for caller'
-        );
-      });
-
-      it('does not allow valid signature for other sender', async function () {
-        await expectRevert(
-          this.sigBouncer.onlyWithValidSignature(await this.signFor(authorizedUser), { from: other }),
-          'SignatureBouncer: invalid signature for caller'
-        );
-      });
-
-      it('does not allow valid signature for method for sender', async function () {
-        await expectRevert(
-          this.sigBouncer.onlyWithValidSignature(await this.signFor(authorizedUser, 'onlyWithValidSignature'),
-            { from: authorizedUser }), 'SignatureBouncer: invalid signature for caller'
-        );
-      });
-    });
-
-    context('method signature', function () {
-      it('allows valid signature with correct method for sender', async function () {
-        await this.sigBouncer.onlyWithValidSignatureAndMethod(
-          await this.signFor(authorizedUser, 'onlyWithValidSignatureAndMethod'), { from: authorizedUser }
-        );
-      });
-
-      it('does not allow invalid signature with correct method for sender', async function () {
-        await expectRevert(
-          this.sigBouncer.onlyWithValidSignatureAndMethod(INVALID_SIGNATURE, { from: authorizedUser }),
-          'SignatureBouncer: invalid signature for caller and method'
-        );
-      });
-
-      it('does not allow valid signature with correct method for other sender', async function () {
-        await expectRevert(
-          this.sigBouncer.onlyWithValidSignatureAndMethod(
-            await this.signFor(authorizedUser, 'onlyWithValidSignatureAndMethod'), { from: other }
-          ),
-          'SignatureBouncer: invalid signature for caller and method'
-        );
-      });
-
-      it('does not allow valid method signature with incorrect method for sender', async function () {
-        await expectRevert(
-          this.sigBouncer.onlyWithValidSignatureAndMethod(await this.signFor(authorizedUser, 'theWrongMethod'),
-            { from: authorizedUser }), 'SignatureBouncer: invalid signature for caller and method'
-        );
-      });
-
-      it('does not allow valid non-method signature method for sender', async function () {
-        await expectRevert(
-          this.sigBouncer.onlyWithValidSignatureAndMethod(await this.signFor(authorizedUser), { from: authorizedUser }),
-          'SignatureBouncer: invalid signature for caller and method'
-        );
-      });
-    });
-
-    context('method and data signature', function () {
-      it('allows valid signature with correct method and data for sender', async function () {
-        await this.sigBouncer.onlyWithValidSignatureAndData(UINT_VALUE,
-          await this.signFor(authorizedUser, 'onlyWithValidSignatureAndData', [UINT_VALUE]), { from: authorizedUser }
-        );
-      });
-
-      it('does not allow invalid signature with correct method and data for sender', async function () {
-        await expectRevert(
-          this.sigBouncer.onlyWithValidSignatureAndData(UINT_VALUE, INVALID_SIGNATURE, { from: authorizedUser }),
-          'SignatureBouncer: invalid signature for caller and data'
-        );
-      });
-
-      it('does not allow valid signature with correct method and incorrect data for sender', async function () {
-        await expectRevert(
-          this.sigBouncer.onlyWithValidSignatureAndData(UINT_VALUE + 10,
-            await this.signFor(authorizedUser, 'onlyWithValidSignatureAndData', [UINT_VALUE]),
-            { from: authorizedUser }
-          ), 'SignatureBouncer: invalid signature for caller and data'
-        );
-      });
-
-      it('does not allow valid signature with correct method and data for other sender', async function () {
-        await expectRevert(
-          this.sigBouncer.onlyWithValidSignatureAndData(UINT_VALUE,
-            await this.signFor(authorizedUser, 'onlyWithValidSignatureAndData', [UINT_VALUE]),
-            { from: other }
-          ), 'SignatureBouncer: invalid signature for caller and data'
-        );
-      });
-
-      it('does not allow valid non-method signature for sender', async function () {
-        await expectRevert(
-          this.sigBouncer.onlyWithValidSignatureAndData(UINT_VALUE,
-            await this.signFor(authorizedUser), { from: authorizedUser }
-          ), 'SignatureBouncer: invalid signature for caller and data'
-        );
-      });
-
-      it('does not allow msg.data shorter than SIGNATURE_SIZE', async function () {
-        await expectRevert(
-          this.sigBouncer.tooShortMsgData({ from: authorizedUser }), 'SignatureBouncer: data is too short'
-        );
-      });
-    });
-  });
-
-  context('signature validation', function () {
-    context('plain signature', function () {
-      it('validates valid signature for valid user', async function () {
-        expect(await this.sigBouncer.checkValidSignature(authorizedUser, await this.signFor(authorizedUser)))
-          .to.equal(true);
-      });
-
-      it('does not validate invalid signature for valid user', async function () {
-        expect(await this.sigBouncer.checkValidSignature(authorizedUser, INVALID_SIGNATURE)).to.equal(false);
-      });
-
-      it('does not validate valid signature for anyone', async function () {
-        expect(await this.sigBouncer.checkValidSignature(other, await this.signFor(authorizedUser))).to.equal(false);
-      });
-
-      it('does not validate valid signature for method for valid user', async function () {
-        expect(await this.sigBouncer.checkValidSignature(
-          authorizedUser, await this.signFor(authorizedUser, 'checkValidSignature'))
-        ).to.equal(false);
-      });
-    });
-
-    context('method signature', function () {
-      it('validates valid signature with correct method for valid user', async function () {
-        expect(await this.sigBouncer.checkValidSignatureAndMethod(authorizedUser,
-          await this.signFor(authorizedUser, 'checkValidSignatureAndMethod'))
-        ).to.equal(true);
-      });
-
-      it('does not validate invalid signature with correct method for valid user', async function () {
-        expect(await this.sigBouncer.checkValidSignatureAndMethod(authorizedUser, INVALID_SIGNATURE)).to.equal(false);
-      });
-
-      it('does not validate valid signature with correct method for anyone', async function () {
-        expect(await this.sigBouncer.checkValidSignatureAndMethod(other,
-          await this.signFor(authorizedUser, 'checkValidSignatureAndMethod'))
-        ).to.equal(false);
-      });
-
-      it('does not validate valid non-method signature with correct method for valid user', async function () {
-        expect(await this.sigBouncer.checkValidSignatureAndMethod(authorizedUser, await this.signFor(authorizedUser))
-        ).to.equal(false);
-      });
-    });
-
-    context('method and data signature', function () {
-      it('validates valid signature with correct method and data for valid user', async function () {
-        expect(await this.sigBouncer.checkValidSignatureAndData(authorizedUser, BYTES_VALUE, UINT_VALUE,
-          await this.signFor(authorizedUser, 'checkValidSignatureAndData', [authorizedUser, BYTES_VALUE, UINT_VALUE]))
-        ).to.equal(true);
-      });
-
-      it('does not validate invalid signature with correct method and data for valid user', async function () {
-        expect(
-          await this.sigBouncer.checkValidSignatureAndData(authorizedUser, BYTES_VALUE, UINT_VALUE, INVALID_SIGNATURE)
-        ).to.equal(false);
-      });
-
-      it('does not validate valid signature with correct method and incorrect data for valid user',
-        async function () {
-          expect(await this.sigBouncer.checkValidSignatureAndData(authorizedUser, BYTES_VALUE, UINT_VALUE + 10,
-            await this.signFor(authorizedUser, 'checkValidSignatureAndData', [authorizedUser, BYTES_VALUE, UINT_VALUE]))
-          ).to.equal(false);
-        }
-      );
-
-      it('does not validate valid signature with correct method and data for anyone', async function () {
-        expect(await this.sigBouncer.checkValidSignatureAndData(other, BYTES_VALUE, UINT_VALUE,
-          await this.signFor(authorizedUser, 'checkValidSignatureAndData', [authorizedUser, BYTES_VALUE, UINT_VALUE]))
-        ).that.equal(false);
-      });
-
-      it('does not validate valid non-method-data signature with correct method and data for valid user',
-        async function () {
-          expect(await this.sigBouncer.checkValidSignatureAndData(authorizedUser, BYTES_VALUE, UINT_VALUE,
-            await this.signFor(authorizedUser, 'checkValidSignatureAndData'))
-          ).to.equal(false);
-        }
-      );
-    });
-  });
-});