diff --git a/contracts/Bucket.sol b/contracts/Bucket.sol index 02a4618..55fbd53 100644 --- a/contracts/Bucket.sol +++ b/contracts/Bucket.sol @@ -1,11 +1,11 @@ pragma solidity ^0.6.1; pragma experimental ABIEncoderV2; -import "@openzeppelin/contracts/cryptography/ECDSA.sol"; +import "@openzeppelin/contracts-ethereum-package/contracts/cryptography/ECDSA.sol"; +import "@openzeppelin/contracts-ethereum-package/contracts/access/Ownable.sol"; -abstract contract Bucket { +abstract contract Bucket is OwnableUpgradeSafe { bool initialized; - address payable public owner; address public tokenAddress; uint256 public expirationTime; uint256 public startTime; @@ -34,16 +34,16 @@ abstract contract Bucket { mapping(address => Redeemable) public redeemables; - modifier onlyOwner() { - require(msg.sender == owner, "owner required"); - _; - } - constructor(bytes memory _eip712DomainName, address _tokenAddress, uint256 _startTime, uint256 _expirationTime, uint256 _maxTxDelayInBlocks) public { - initialize(_eip712DomainName, _tokenAddress, _startTime, _expirationTime, _maxTxDelayInBlocks, msg.sender); + initialize(_eip712DomainName, _tokenAddress, _startTime, _expirationTime, _maxTxDelayInBlocks); } - function initialize(bytes memory _eip712DomainName, address _tokenAddress, uint256 _startTime, uint256 _expirationTime, uint256 _maxTxDelayInBlocks, address _owner) public { + // called by OwnableUpgradeSafe; using tx.origin otherwise msg.sender would be the factory + function _msgSender() internal view override(ContextUpgradeSafe) returns (address payable) { + return tx.origin; + } + + function initialize(bytes memory _eip712DomainName, address _tokenAddress, uint256 _startTime, uint256 _expirationTime, uint256 _maxTxDelayInBlocks) public { require(initialized == false, "already initialized"); require(_maxTxDelayInBlocks > 0 && _maxTxDelayInBlocks < 256, "the valid range is 1 to 255"); require(_expirationTime > block.timestamp, "expiration can't be in the past"); @@ -52,7 +52,8 @@ abstract contract Bucket { startTime = _startTime; expirationTime = _expirationTime; maxTxDelayInBlocks = _maxTxDelayInBlocks; - owner = payable(_owner); + + __Ownable_init(); DOMAIN_SEPARATOR = keccak256(abi.encode( EIP712DOMAIN_TYPEHASH, @@ -101,7 +102,7 @@ abstract contract Bucket { function kill() external onlyOwner { require(block.timestamp >= expirationTime, "not expired yet"); transferRedeemablesToOwner(); - selfdestruct(owner); + selfdestruct(payable(owner())); } function getChainID() internal pure returns (uint256) { diff --git a/contracts/ERC20Bucket.sol b/contracts/ERC20Bucket.sol index 848f9a1..1464458 100644 --- a/contracts/ERC20Bucket.sol +++ b/contracts/ERC20Bucket.sol @@ -2,7 +2,7 @@ pragma solidity ^0.6.1; pragma experimental ABIEncoderV2; import "./Bucket.sol"; -import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import "@openzeppelin/contracts-ethereum-package/contracts/token/ERC20/IERC20.sol"; contract ERC20Bucket is Bucket { uint256 public redeemableSupply; @@ -48,7 +48,7 @@ contract ERC20Bucket is Bucket { } function transferRedeemablesToOwner() internal override { - bool success = IERC20(tokenAddress).transfer(owner, this.totalSupply()); + bool success = IERC20(tokenAddress).transfer(owner(), this.totalSupply()); assert(success); } diff --git a/contracts/ERC20BucketFactory.sol b/contracts/ERC20BucketFactory.sol index c353b3c..44d908d 100644 --- a/contracts/ERC20BucketFactory.sol +++ b/contracts/ERC20BucketFactory.sol @@ -13,7 +13,7 @@ contract ERC20BucketFactory { } function create(address _tokenAddress, uint256 _startTime, uint256 _expirationTime, uint256 _maxTxDelayInBlocks) public returns (address) { - address p = address(new Proxy(abi.encodeWithSelector(0xe0c69ab8, "KeycardERC20Bucket", _tokenAddress, _startTime, _expirationTime, _maxTxDelayInBlocks, msg.sender), address(ERC20BucketImplementation))); + address p = address(new Proxy(abi.encodeWithSelector(0x185d1646, "KeycardERC20Bucket", _tokenAddress, _startTime, _expirationTime, _maxTxDelayInBlocks), address(ERC20BucketImplementation))); emit BucketCreated(msg.sender, p); return p; } diff --git a/contracts/NFTBucket.sol b/contracts/NFTBucket.sol index 7969d07..3e13bcf 100644 --- a/contracts/NFTBucket.sol +++ b/contracts/NFTBucket.sol @@ -20,6 +20,7 @@ contract NFTBucket is Bucket, IERC165, IERC721Receiver { } function transferRedeemablesToOwner() internal override { + address owner = owner(); IERC721(tokenAddress).setApprovalForAll(owner, true); assert(IERC721(tokenAddress).isApprovedForAll(address(this), owner)); } @@ -34,7 +35,7 @@ contract NFTBucket is Bucket, IERC165, IERC721Receiver { function onERC721Received(address _operator, address _from, uint256 _tokenID, bytes calldata _data) external override(IERC721Receiver) returns(bytes4) { require(msg.sender == tokenAddress, "only the NFT contract can call this"); - require((_operator == owner) || (_from == owner), "only the owner can create redeemables"); + require((_operator == owner()) || (_from == owner()), "only the owner can create redeemables"); require(_data.length == 52, "invalid data field"); bytes memory d = _data; diff --git a/contracts/NFTBucketFactory.sol b/contracts/NFTBucketFactory.sol index 15d7e0b..11fe816 100644 --- a/contracts/NFTBucketFactory.sol +++ b/contracts/NFTBucketFactory.sol @@ -13,7 +13,7 @@ contract NFTBucketFactory { } function create(address _tokenAddress, uint256 _startTime, uint256 _expirationTime, uint256 _maxTxDelayInBlocks) public returns (address) { - address p = address(new Proxy(abi.encodeWithSelector(0xe0c69ab8, "KeycardNFTBucket", _tokenAddress, _startTime, _expirationTime, _maxTxDelayInBlocks, msg.sender), address(NFTBucketImplementation))); + address p = address(new Proxy(abi.encodeWithSelector(0x185d1646, "KeycardNFTBucket", _tokenAddress, _startTime, _expirationTime, _maxTxDelayInBlocks), address(NFTBucketImplementation))); emit BucketCreated(msg.sender, p); return p; } diff --git a/contracts/erc20/IERC20Detailed.sol b/contracts/erc20/IERC20Detailed.sol index 540050d..6bc47df 100644 --- a/contracts/erc20/IERC20Detailed.sol +++ b/contracts/erc20/IERC20Detailed.sol @@ -1,6 +1,6 @@ pragma solidity >=0.5.0 <0.7.0; -import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import "@openzeppelin/contracts-ethereum-package/contracts/token/ERC20/IERC20.sol"; abstract contract IERC20Detailed is IERC20 { function name() virtual public view returns (string memory); diff --git a/contracts/erc20/StandardToken.sol b/contracts/erc20/StandardToken.sol index 768069e..0dd1294 100644 --- a/contracts/erc20/StandardToken.sol +++ b/contracts/erc20/StandardToken.sol @@ -1,6 +1,6 @@ pragma solidity ^0.6.1; -import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import "@openzeppelin/contracts-ethereum-package/contracts/token/ERC20/IERC20.sol"; contract StandardToken is IERC20 { diff --git a/package.json b/package.json index 79e5ef7..bb7fb6e 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "dependencies": { - "@openzeppelin/contracts": "^3.2.0", + "@openzeppelin/contracts-ethereum-package": "^3.0.0", "eth-gas-reporter": "^0.2.17" } } diff --git a/test/erc20_spec.js b/test/erc20_spec.js index 7bdd1bf..5040d2c 100644 --- a/test/erc20_spec.js +++ b/test/erc20_spec.js @@ -1,6 +1,10 @@ const TestToken = artifacts.require('TestToken'); const ERC20Bucket = artifacts.require('ERC20Bucket'); const ERC20BucketFactory = artifacts.require('ERC20BucketFactory'); +const { + bucketShouldBeOwnable, + factoryShouldCreateAnOwnableBucket, +} = require("./helpers"); const TOTAL_SUPPLY = 10000; const GIFT_AMOUNT = 10; @@ -102,6 +106,9 @@ contract("ERC20Bucket", function () { tokenInstance = new web3.eth.Contract(TestToken.abi, deployedTestToken.address); }); + bucketShouldBeOwnable("erc20", () => [ERC20Bucket, shop, tokenInstance, [START_TIME, EXPIRATION_TIME, MAX_TX_DELAY_BLOCKS]]); + factoryShouldCreateAnOwnableBucket("erc20", () => [ERC20BucketFactory, ERC20Bucket, shop, tokenInstance, [START_TIME, EXPIRATION_TIME, MAX_TX_DELAY_BLOCKS]]); + it("deploy factory", async () => { const contract = new web3.eth.Contract(ERC20BucketFactory.abi); const deploy = contract.deploy({ data: ERC20BucketFactory.bytecode }); @@ -130,7 +137,7 @@ contract("ERC20Bucket", function () { }); it("deploy bucket via factory", async () => { - const create = factoryInstance.methods.create(tokenInstance._address, START_TIME, EXPIRATION_TIME, MAX_TX_DELAY_BLOCKS); + const create = factoryInstance.methods.create(tokenInstance.options.address, START_TIME, EXPIRATION_TIME, MAX_TX_DELAY_BLOCKS); const gas = await create.estimateGas(); const receipt = await create.send({ from: shop, diff --git a/test/helpers.js b/test/helpers.js new file mode 100644 index 0000000..1476b98 --- /dev/null +++ b/test/helpers.js @@ -0,0 +1,47 @@ +module.exports.bucketShouldBeOwnable = (bucketType, argsFunc) => { + it(`deploy an ownable ${bucketType} bucket`, async () => { + [bucketSpecs, deployer, tokenInstance, args] = argsFunc(); + const contract = new web3.eth.Contract(bucketSpecs.abi); + const deploy = contract.deploy({ + data: bucketSpecs.bytecode, + arguments: [tokenInstance.options.address, ...args], + }); + const gas = await deploy.estimateGas(); + const deployed = await deploy.send({ + from: deployer, + gas, + }); + + const bucket = new web3.eth.Contract(bucketSpecs.abi, deployed.options.address); + const owner = await bucket.methods.owner().call(); + assert.equal(owner, deployer); + }); +} + +module.exports.factoryShouldCreateAnOwnableBucket = (bucketType, argsFunc) => { + it(`factory creates an ownable ${bucketType} bucket`, async () => { + [factorySpecs, bucketSpecs, deployer, tokenInstance, args] = argsFunc(); + const factoryContract = new web3.eth.Contract(factorySpecs.abi); + const deploy = factoryContract.deploy({ + data: factorySpecs.bytecode, + arguments: [], + }); + const deployGas = await deploy.estimateGas(); + const deployed = await deploy.send({ + from: deployer, + gas: deployGas, + }); + + const factory = new web3.eth.Contract(factorySpecs.abi, deployed.options.address); + const create = factory.methods.create(tokenInstance.options.address, ...args); + const createGas = await create.estimateGas(); + const rec = await create.send({ + from: deployer, + gas: createGas, + }); + + const bucket = new web3.eth.Contract(bucketSpecs.abi, rec.events.BucketCreated.returnValues.bucket); + const owner = await bucket.methods.owner().call(); + assert.equal(owner, deployer); + }); +} diff --git a/test/nft_contract_spec.js b/test/nft_contract_spec.js index d12ff3e..315e8fd 100644 --- a/test/nft_contract_spec.js +++ b/test/nft_contract_spec.js @@ -1,6 +1,10 @@ const TestNFT = artifacts.require('TestNFT'); const NFTBucket = artifacts.require('NFTBucket'); const NFTBucketFactory = artifacts.require('NFTBucketFactory'); +const { + bucketShouldBeOwnable, + factoryShouldCreateAnOwnableBucket, +} = require("./helpers"); const TOTAL_SUPPLY = 10000; const GIFT_AMOUNT = 10; @@ -109,6 +113,9 @@ contract("NFTBucket", function () { tokenInstance = new web3.eth.Contract(TestNFT.abi, deployedTestToken.address); }); + bucketShouldBeOwnable("nft", () => [NFTBucket, shop, tokenInstance, [START_TIME, EXPIRATION_TIME, MAX_TX_DELAY_BLOCKS]]); + factoryShouldCreateAnOwnableBucket("nft", () => [NFTBucketFactory, NFTBucket, shop, tokenInstance, [START_TIME, EXPIRATION_TIME, MAX_TX_DELAY_BLOCKS]]); + it("deploy factory", async () => { const contract = new web3.eth.Contract(NFTBucketFactory.abi); const deploy = contract.deploy({ data: NFTBucketFactory.bytecode }); @@ -137,9 +144,9 @@ contract("NFTBucket", function () { }); it("deploy bucket via factory", async () => { - const create = factoryInstance.methods.create(tokenInstance._address, START_TIME, EXPIRATION_TIME, MAX_TX_DELAY_BLOCKS); + const create = factoryInstance.methods.create(tokenInstance.options.address, START_TIME, EXPIRATION_TIME, MAX_TX_DELAY_BLOCKS); const gas = await create.estimateGas(); - const receipt = await create.send({ + const rec = await create.send({ from: shop, gas: gas, }); diff --git a/yarn.lock b/yarn.lock index 4bf7802..967d210 100644 --- a/yarn.lock +++ b/yarn.lock @@ -99,10 +99,10 @@ "@ethersproject/constants" "^5.0.4" "@ethersproject/logger" "^5.0.5" -"@openzeppelin/contracts@^3.2.0": - version "3.2.0" - resolved "https://registry.yarnpkg.com/@openzeppelin/contracts/-/contracts-3.2.0.tgz#3e6b3a7662d8ed64271ade96ef42655db983fd9d" - integrity sha512-bUOmkSoPkjnUyMiKo6RYnb0VHBk5D9KKDAgNLzF41aqAM3TeE0yGdFF5dVRcV60pZdJLlyFT/jjXIZCWyyEzAQ== +"@openzeppelin/contracts-ethereum-package@^3.0.0": + version "3.0.0" + resolved "https://registry.yarnpkg.com/@openzeppelin/contracts-ethereum-package/-/contracts-ethereum-package-3.0.0.tgz#d5db971a177c3b37733db2ee4ebdb79c67575d64" + integrity sha512-Xg33RtX7FGbSK/YnroLhcGNAvH30/C84NRW8KvbSdXXYiLA8YqM1bOA9sAeLjmQxXqYUn/YL4AUVTgDnG51NOw== "@solidity-parser/parser@^0.5.2": version "0.5.2"