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.

Sign up for GitHub

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

Add no-return-data ERC20 support to SafeERC20. #1655

Merged
merged 6 commits into from
Feb 28, 2019
Merged

Add no-return-data ERC20 support to SafeERC20. #1655

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
139b69a
Add no-return-data ERC20 support to SafeERC20.
nventuro Feb 27, 2019
92e75e5
Add changelog entry.
nventuro Feb 27, 2019
9192661
Replace abi.encodeWithSignature for encodeWithSelector.
nventuro Feb 27, 2019
d03b819
Remove SafeERC20 test code duplication.
nventuro Feb 27, 2019
7e6919d
Replace assembly for abi.decode.
nventuro Feb 28, 2019
3460346
Fix linter errors.
nventuro Feb 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions contracts/token/ERC20/SafeERC20.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,29 +16,29 @@ library SafeERC20 {
using SafeMath for uint256;

function safeTransfer(IERC20 token, address to, uint256 value) internal {
callAndAssertSuccess(token, abi.encodeWithSignature("transfer(address,uint256)", to, value));
callAndAssertSuccess(token, abi.encodeWithSelector(token.transfer.selector, to, value));
}

function safeTransferFrom(IERC20 token, address from, address to, uint256 value) internal {
callAndAssertSuccess(token, abi.encodeWithSignature("transferFrom(address,address,uint256)", from, to, value));
callAndAssertSuccess(token, abi.encodeWithSelector(token.transferFrom.selector, from, to, value));
}

function safeApprove(IERC20 token, address spender, uint256 value) internal {
// safeApprove should only be called when setting an initial allowance,
// or when resetting it to zero. To increase and decrease it, use
// 'safeIncreaseAllowance' and 'safeDecreaseAllowance'
require((value == 0) || (token.allowance(address(this), spender) == 0));
callAndAssertSuccess(token, abi.encodeWithSignature("approve(address,uint256)", spender, value));
callAndAssertSuccess(token, abi.encodeWithSelector(token.approve.selector, spender, value));
}

function safeIncreaseAllowance(IERC20 token, address spender, uint256 value) internal {
uint256 newAllowance = token.allowance(address(this), spender).add(value);
callAndAssertSuccess(token, abi.encodeWithSignature("approve(address,uint256)", spender, newAllowance));
callAndAssertSuccess(token, abi.encodeWithSelector(token.approve.selector, spender, newAllowance));
}

function safeDecreaseAllowance(IERC20 token, address spender, uint256 value) internal {
uint256 newAllowance = token.allowance(address(this), spender).sub(value);
callAndAssertSuccess(token, abi.encodeWithSignature("approve(address,uint256)", spender, newAllowance));
callAndAssertSuccess(token, abi.encodeWithSelector(token.approve.selector, spender, newAllowance));
}

/**
Expand Down
149 changes: 49 additions & 100 deletions test/token/ERC20/SafeERC20.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const ERC20ReturnTrueMock = artifacts.require('ERC20ReturnTrueMock');
const ERC20NoReturnMock = artifacts.require('ERC20NoReturnMock');
const SafeERC20Wrapper = artifacts.require('SafeERC20Wrapper');

contract('SafeERC20', function () {
contract.only('SafeERC20', function () {
nventuro marked this conversation as resolved.
Show resolved Hide resolved
describe('with token that returns false on all calls', function () {
beforeEach(async function () {
this.wrapper = await SafeERC20Wrapper.new((await ERC20ReturnFalseMock.new()).address);
Expand Down Expand Up @@ -37,126 +37,75 @@ contract('SafeERC20', function () {
this.wrapper = await SafeERC20Wrapper.new((await ERC20ReturnTrueMock.new()).address);
});

it('doesn\'t revert on transfer', async function () {
await this.wrapper.transfer();
});
shouldOnlyRevertOnErrors();
});

it('doesn\'t revert on transferFrom', async function () {
await this.wrapper.transferFrom();
describe('with token that returns no boolean values', function () {
beforeEach(async function () {
this.wrapper = await SafeERC20Wrapper.new((await ERC20NoReturnMock.new()).address);
});

describe('approvals', function () {
context('with zero allowance', function () {
beforeEach(async function () {
await this.wrapper.setAllowance(0);
});
shouldOnlyRevertOnErrors();
});
});

it('doesn\'t revert when approving a non-zero allowance', async function () {
await this.wrapper.approve(100);
});

it('doesn\'t revert when approving a zero allowance', async function () {
await this.wrapper.approve(0);
});
function shouldOnlyRevertOnErrors() {
it('doesn\'t revert on transfer', async function () {
await this.wrapper.transfer();
});

it('doesn\'t revert when increasing the allowance', async function () {
await this.wrapper.increaseAllowance(10);
});
it('doesn\'t revert on transferFrom', async function () {
await this.wrapper.transferFrom();
});

it('reverts when decreasing the allowance', async function () {
await shouldFail.reverting(this.wrapper.decreaseAllowance(10));
});
describe('approvals', function () {
context('with zero allowance', function () {
beforeEach(async function () {
await this.wrapper.setAllowance(0);
});

context('with non-zero allowance', function () {
beforeEach(async function () {
await this.wrapper.setAllowance(100);
});

it('reverts when approving a non-zero allowance', async function () {
await shouldFail.reverting(this.wrapper.approve(20));
});

it('doesn\'t revert when approving a zero allowance', async function () {
await this.wrapper.approve(0);
});

it('doesn\'t revert when increasing the allowance', async function () {
await this.wrapper.increaseAllowance(10);
});

it('doesn\'t revert when decreasing the allowance to a positive value', async function () {
await this.wrapper.decreaseAllowance(50);
});

it('reverts when decreasing the allowance to a negative value', async function () {
await shouldFail.reverting(this.wrapper.decreaseAllowance(200));
});
it('doesn\'t revert when approving a non-zero allowance', async function () {
await this.wrapper.approve(100);
});
});
});

describe('with token that returns no boolean values', function () {
beforeEach(async function () {
this.wrapper = await SafeERC20Wrapper.new((await ERC20NoReturnMock.new()).address);
});
it('doesn\'t revert when approving a zero allowance', async function () {
await this.wrapper.approve(0);
});

it('doesn\'t revert on transfer', async function () {
await this.wrapper.transfer();
});
it('doesn\'t revert when increasing the allowance', async function () {
await this.wrapper.increaseAllowance(10);
});

it('doesn\'t revert on transferFrom', async function () {
await this.wrapper.transferFrom();
it('reverts when decreasing the allowance', async function () {
await shouldFail.reverting(this.wrapper.decreaseAllowance(10));
});
});

describe('approvals', function () {
context('with zero allowance', function () {
beforeEach(async function () {
await this.wrapper.setAllowance(0);
});

it('doesn\'t revert when approving a non-zero allowance', async function () {
await this.wrapper.approve(100);
});

it('doesn\'t revert when approving a zero allowance', async function () {
await this.wrapper.approve(0);
});

it('doesn\'t revert when increasing the allowance', async function () {
await this.wrapper.increaseAllowance(10);
});

it('reverts when decreasing the allowance', async function () {
await shouldFail.reverting(this.wrapper.decreaseAllowance(10));
});
context('with non-zero allowance', function () {
beforeEach(async function () {
await this.wrapper.setAllowance(100);
});

context('with non-zero allowance', function () {
beforeEach(async function () {
await this.wrapper.setAllowance(100);
});

it('reverts when approving a non-zero allowance', async function () {
await shouldFail.reverting(this.wrapper.approve(20));
});
it('reverts when approving a non-zero allowance', async function () {
await shouldFail.reverting(this.wrapper.approve(20));
});

it('doesn\'t revert when approving a zero allowance', async function () {
await this.wrapper.approve(0);
});
it('doesn\'t revert when approving a zero allowance', async function () {
await this.wrapper.approve(0);
});

it('doesn\'t revert when increasing the allowance', async function () {
await this.wrapper.increaseAllowance(10);
});
it('doesn\'t revert when increasing the allowance', async function () {
await this.wrapper.increaseAllowance(10);
});

it('doesn\'t revert when decreasing the allowance to a positive value', async function () {
await this.wrapper.decreaseAllowance(50);
});
it('doesn\'t revert when decreasing the allowance to a positive value', async function () {
await this.wrapper.decreaseAllowance(50);
});

it('reverts when decreasing the allowance to a negative value', async function () {
await shouldFail.reverting(this.wrapper.decreaseAllowance(200));
});
it('reverts when decreasing the allowance to a negative value', async function () {
await shouldFail.reverting(this.wrapper.decreaseAllowance(200));
});
});
});
});
}