mirror
/
openzeppelin-contracts
mirror of https://github.com/OpenZeppelin/openzeppelin-contracts
2
1
Fork 1
Code Issues Releases Wiki Activity

Remove deprecated functions and contracts (#2125)

Browse Source 
* Remove ERC721.burn(owner, tokenId)

* Remove ERC721._checkOnERC721Received from the contract's API

* Fix linter error

* Remove Escrow and PullPayment withdrawWithGas, replace for withdraw

* Add changelog entry

* Add reentrancy notice
pull/2133/head
Nicolás Venturo 5 years ago committed by GitHub
parent c173392e15
commit f1db30955d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 22 additions and 123 deletions
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Show Stats Download Patch File Download Diff File
  1. 3
      CHANGELOG.md
  2. 4
      contracts/mocks/ERC721Mock.sol
  3. 22
      contracts/payment/PullPayment.sol
  4. 4
      contracts/payment/README.adoc
  5. 28
      contracts/payment/escrow/Escrow.sol
  6. 18
      contracts/token/ERC721/ERC721.sol
  7. 12
      test/payment/PullPayment.test.js
  8. 54
      test/token/ERC721/ERC721.test.js

3
CHANGELOG.md
Unescape View File

@ -6,6 +6,9 @@
* `AccessControl`: new contract for managing permissions in a system, replacement for `Ownable` and `Roles`. ([#2112](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2112))
### Breaking changes
* `ERC721`: `burn(owner, tokenId)` was removed, use `burn(owner)` instead. ([#2125](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2125))
* `ERC721`: `_checkOnERC721Received` was removed. ([#2125](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2125))
* `PullPayment`, `Escrow`: `withdrawWithGas` was removed. The old `withdraw` function now forwards all gas. ([#2125](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2125))
* `Roles` was removed, use `AccessControl` as a replacement. ([#2112](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2112))
* `ECDSA`: when receiving an invalid signature, `recover` now reverts instead of returning the zero address. ([#2114](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2114))
* `Pausable`: moved to the `utils` directory. ([#2122](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2122))

4
contracts/mocks/ERC721Mock.sol
Unescape View File

@ -19,10 +19,6 @@ contract ERC721Mock is ERC721 {
_mint(to, tokenId);
}
function burn(address owner, uint256 tokenId) public {
_burn(owner, tokenId);
}
function burn(uint256 tokenId) public {
_burn(tokenId);
}

22
contracts/payment/PullPayment.sol
Unescape View File

@ -28,35 +28,21 @@ contract PullPayment {
}
/**
* @dev Withdraw accumulated payments.
* @dev Withdraw accumulated payments, forwarding all gas to the recipient.
*
* Note that _any_ account can call this function, not just the `payee`.
* This means that contracts unaware of the `PullPayment` protocol can still
* receive funds this way, by having a separate account call
* {withdrawPayments}.
*
* NOTE: This function has been deprecated, use {withdrawPaymentsWithGas}
* instead. Calling contracts with fixed gas limits is an anti-pattern and
* may break contract interactions in network upgrades (hardforks).
* https://diligence.consensys.net/blog/2019/09/stop-using-soliditys-transfer-now/[Learn more.]
*
* @param payee Whose payments will be withdrawn.
*/
function withdrawPayments(address payable payee) public virtual {
_escrow.withdraw(payee);
}
/**
* @dev Same as {withdrawPayments}, but forwarding all gas to the recipient.
*
* WARNING: Forwarding all gas opens the door to reentrancy vulnerabilities.
* Make sure you trust the recipient, or are either following the
* checks-effects-interactions pattern or using {ReentrancyGuard}.
*
* _Available since v2.4.0._
* @param payee Whose payments will be withdrawn.
*/
function withdrawPaymentsWithGas(address payable payee) external virtual {
_escrow.withdrawWithGas(payee);
function withdrawPayments(address payable payee) public virtual {
_escrow.withdraw(payee);
}
/**

4
contracts/payment/README.adoc
Unescape View File

@ -1,6 +1,8 @@
= Payment
NOTE: This page is incomplete. We're working to improve it for the next release. Stay tuned!
Utilities related to sending and receiving payments. Examples are {PullPayment}, which implements the best security practices when sending funds to third parties, and {PaymentSplitter} to receive incoming payments among a number of beneficiaries.
TIP: When transferring funds to and from untrusted third parties, there is always a security risk of reentrancy. If you would like to learn more about this and ways to protect against it, check out our blog post https://blog.openzeppelin.com/reentrancy-after-istanbul/[Reentrancy After Istanbul].
== Utilities

28
contracts/payment/escrow/Escrow.sol
Unescape View File

@ -42,36 +42,16 @@ contract Escrow is Ownable {
}
/**
* @dev Withdraw accumulated balance for a payee, forwarding 2300 gas (a
* Solidity `transfer`).
*
* NOTE: This function has been deprecated, use {withdrawWithGas} instead.
* Calling contracts with fixed-gas limits is an anti-pattern and may break
* contract interactions in network upgrades (hardforks).
* https://diligence.consensys.net/blog/2019/09/stop-using-soliditys-transfer-now/[Learn more.]
*
* @param payee The address whose funds will be withdrawn and transferred to.
*/
function withdraw(address payable payee) public virtual onlyOwner {
uint256 payment = _deposits[payee];
_deposits[payee] = 0;
payee.transfer(payment);
emit Withdrawn(payee, payment);
}
/**
* @dev Same as {withdraw}, but forwarding all gas to the recipient.
* @dev Withdraw accumulated balance for a payee, forwarding all gas to the
* recipient.
*
* WARNING: Forwarding all gas opens the door to reentrancy vulnerabilities.
* Make sure you trust the recipient, or are either following the
* checks-effects-interactions pattern or using {ReentrancyGuard}.
*
* _Available since v2.4.0._
* @param payee The address whose funds will be withdrawn and transferred to.
*/
function withdrawWithGas(address payable payee) public virtual onlyOwner {
function withdraw(address payable payee) public virtual onlyOwner {
uint256 payment = _deposits[payee];
_deposits[payee] = 0;

18
contracts/token/ERC721/ERC721.sol
Unescape View File

@ -269,12 +269,10 @@ contract ERC721 is Context, ERC165, IERC721 {
/**
* @dev Internal function to burn a specific token.
* Reverts if the token does not exist.
* Deprecated, use {_burn} instead.
* @param owner owner of the token to burn
* @param tokenId uint256 ID of the token being burned
*/
function _burn(address owner, uint256 tokenId) internal virtual {
require(ownerOf(tokenId) == owner, "ERC721: burn of token that is not own");
function _burn(uint256 tokenId) internal virtual {
address owner = ownerOf(tokenId);
_beforeTokenTransfer(owner, address(0), tokenId);
@ -287,15 +285,6 @@ contract ERC721 is Context, ERC165, IERC721 {
emit Transfer(owner, address(0), tokenId);
}
/**
* @dev Internal function to burn a specific token.
* Reverts if the token does not exist.
* @param tokenId uint256 ID of the token being burned
*/
function _burn(uint256 tokenId) internal virtual {
_burn(ownerOf(tokenId), tokenId);
}
/**
* @dev Internal function to transfer ownership of a given token ID to another address.
* As opposed to {transferFrom}, this imposes no restrictions on msg.sender.
@ -324,7 +313,6 @@ contract ERC721 is Context, ERC165, IERC721 {
* @dev Internal function to invoke {IERC721Receiver-onERC721Received} on a target address.
* The call is not executed if the target address is not a contract.
*
* This is an internal detail of the `ERC721` contract and its use is deprecated.
* @param from address representing the previous owner of the given token ID
* @param to target address that will receive the tokens
* @param tokenId uint256 ID of the token to be transferred
@ -332,7 +320,7 @@ contract ERC721 is Context, ERC165, IERC721 {
* @return bool whether the call correctly returned the expected magic value
*/
function _checkOnERC721Received(address from, address to, uint256 tokenId, bytes memory _data)
internal virtual returns (bool)
private returns (bool)
{
if (!to.isContract()) {
return true;

12
test/payment/PullPayment.test.js
Unescape View File

@ -46,16 +46,4 @@ describe('PullPayment', function () {
expect(await balanceTracker.delta()).to.be.bignumber.equal(amount);
expect(await this.contract.payments(payee1)).to.be.bignumber.equal('0');
});
it('can withdraw payment forwarding all gas', async function () {
const balanceTracker = await balance.tracker(payee1);
await this.contract.callTransfer(payee1, amount, { from: payer });
expect(await this.contract.payments(payee1)).to.be.bignumber.equal(amount);
await this.contract.withdrawPaymentsWithGas(payee1);
expect(await balanceTracker.delta()).to.be.bignumber.equal(amount);
expect(await this.contract.payments(payee1)).to.be.bignumber.equal('0');
});
});

54
test/token/ERC721/ERC721.test.js
Unescape View File

@ -9,7 +9,7 @@ const { shouldBehaveLikeERC721 } = require('./ERC721.behavior');
const ERC721Mock = contract.fromArtifact('ERC721Mock');
describe('ERC721', function () {
const [ owner, other ] = accounts;
const [ owner ] = accounts;
beforeEach(async function () {
this.token = await ERC721Mock.new();
@ -47,54 +47,10 @@ describe('ERC721', function () {
});
});
describe('_burn(address, uint256)', function () {
describe('_burn', function () {
it('reverts when burning a non-existent token id', async function () {
await expectRevert(
this.token.methods['burn(address,uint256)'](owner, tokenId), 'ERC721: owner query for nonexistent token'
);
});
context('with minted token', function () {
beforeEach(async function () {
await this.token.mint(owner, tokenId);
});
it('reverts when the account is not the owner', async function () {
await expectRevert(
this.token.methods['burn(address,uint256)'](other, tokenId), 'ERC721: burn of token that is not own'
);
});
context('with burnt token', function () {
beforeEach(async function () {
({ logs: this.logs } = await this.token.methods['burn(address,uint256)'](owner, tokenId));
});
it('emits a Transfer event', function () {
expectEvent.inLogs(this.logs, 'Transfer', { from: owner, to: ZERO_ADDRESS, tokenId });
});
it('deletes the token', async function () {
expect(await this.token.balanceOf(owner)).to.be.bignumber.equal('0');
await expectRevert(
this.token.ownerOf(tokenId), 'ERC721: owner query for nonexistent token'
);
});
it('reverts when burning a token id that has been deleted', async function () {
await expectRevert(
this.token.methods['burn(address,uint256)'](owner, tokenId),
'ERC721: owner query for nonexistent token'
);
});
});
});
});
describe('_burn(uint256)', function () {
it('reverts when burning a non-existent token id', async function () {
await expectRevert(
this.token.methods['burn(uint256)'](tokenId), 'ERC721: owner query for nonexistent token'
this.token.burn(tokenId), 'ERC721: owner query for nonexistent token'
);
});
@ -105,7 +61,7 @@ describe('ERC721', function () {
context('with burnt token', function () {
beforeEach(async function () {
({ logs: this.logs } = await this.token.methods['burn(uint256)'](tokenId));
({ logs: this.logs } = await this.token.burn(tokenId));
});
it('emits a Transfer event', function () {
@ -121,7 +77,7 @@ describe('ERC721', function () {
it('reverts when burning a token id that has been deleted', async function () {
await expectRevert(
this.token.methods['burn(uint256)'](tokenId), 'ERC721: owner query for nonexistent token'
this.token.burn(tokenId), 'ERC721: owner query for nonexistent token'
);
});
});

Write Preview
Loading…
Cancel
Save