Merge pull request #319 from status-im/fix/docs

fix: improve contract documentation and removed unneeded allowance check due to already verifying if transfer was successful
This commit is contained in:
Iuri Matias 2019-06-19 09:04:33 -04:00 committed by GitHub
commit 8c5aa9cb19
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 80 additions and 66 deletions

View File

@ -1,10 +1,18 @@
/* solium-disable security/no-block-members */
pragma solidity ^0.5.8;
pragma solidity >=0.5.0 <0.6.0;
import "./License.sol";
/**
* Arbitrable
* @dev Utils for management of disputes
*/
contract Arbitrable {
enum ArbitrationResult {UNSOLVED, BUYER, SELLER}
License public arbitratorLicenses;
mapping(uint => ArbitrationCase) public arbitrationCases;
struct ArbitrationCase {
@ -20,22 +28,29 @@ contract Arbitrable {
event ArbitrationRequired(uint escrowId, uint date);
event ArbitrationResolved(uint escrowId, ArbitrationResult result, address arbitrator, uint date);
enum ArbitrationResult {UNSOLVED, BUYER, SELLER}
License public arbitratorLicenses;
/**
* @param _arbitratorLicenses Address of the Arbitrator Licenses contract
*/
constructor(address _arbitratorLicenses) public {
arbitratorLicenses = License(_arbitratorLicenses);
}
// Abstract functions
/**
* @param _escrowId Id of the escrow with an open dispute
* @param _releaseFunds Release funds to the buyer
* @param _arbitrator Address of the arbitrator solving the dispute
* @dev Abstract contract used to perform actions after a dispute has been settled
*/
function solveDispute(uint _escrowId, bool _releaseFunds, address _arbitrator) internal;
function getArbitrator(uint _escrowId) public view returns(address);
/**
* @notice arbitration exists
* @notice Get arbitrator of an escrow
* @return address Arbitrator address
*/
function getArbitrator(uint _escrowId) public view returns(address);
/**
* @notice Determine if a dispute exists/existed for an escrow
* @param _escrowId Escrow to verify
* @return bool result
*/
@ -43,9 +58,8 @@ contract Arbitrable {
return arbitrationCases[_escrowId].open || arbitrationCases[_escrowId].result != ArbitrationResult.UNSOLVED;
}
/**
* @notice cancel arbitration
* @notice Cancel arbitration
* @param _escrowId Escrow to cancel
*/
function cancelArbitration(uint _escrowId) public {
@ -58,10 +72,9 @@ contract Arbitrable {
emit ArbitrationCanceled(_escrowId, block.timestamp);
}
function openDispute(uint _escrowId, address _openBy, string memory motive) internal {
require(!arbitrationCases[_escrowId].open, "Arbitration already open");
require(arbitrationCases[_escrowId].result == ArbitrationResult.UNSOLVED, "Arbitration already solved");
require(arbitrationCases[_escrowId].result == ArbitrationResult.UNSOLVED && !arbitrationCases[_escrowId].open,
"Arbitration already solved or has been opened before");
arbitrationCases[_escrowId] = ArbitrationCase({
open: true,
@ -74,7 +87,6 @@ contract Arbitrable {
emit ArbitrationRequired(_escrowId, block.timestamp);
}
/**
* @notice Set arbitration result in favour of the buyer or seller and transfer funds accordingly
* @param _escrowId Id of the escrow
@ -87,7 +99,6 @@ contract Arbitrable {
require(arbitratorLicenses.isLicenseOwner(msg.sender), "Only arbitrators can invoke this function");
require(getArbitrator(_escrowId) == msg.sender, "Invalid escrow arbitrator");
arbitrationCases[_escrowId].open = false;
arbitrationCases[_escrowId].result = _result;
arbitrationCases[_escrowId].arbitrator = msg.sender;
@ -105,5 +116,4 @@ contract Arbitrable {
solveDispute(_escrowId, false, msg.sender);
}
}
}

View File

@ -1,21 +1,29 @@
pragma solidity ^0.5.8;
pragma solidity >=0.5.0 <0.6.0;
import "../token/ERC20Token.sol";
import "../common/Ownable.sol";
/**
* @title Fee utilities
* @dev Fee registry, payment and withdraw utilities.
*/
contract Fees is Ownable {
address public feeDestination;
uint public feeAmount;
ERC20Token public feeToken;
uint public feeBalance;
mapping(uint => bool) public feePaid;
event FeeDestinationChanged(address);
event FeeAmountChanged(uint amount);
event FeesWithdrawn(uint amount);
mapping(uint => bool) public feePaid;
/**
* @param _feeToken Address of token used to pay for fees (SNT)
* @param _feeDestination Address to send the fees once withdraw is called
* @param _feeAmount Fee amount in token-wei
* @dev TODO: determine if the contract will hold the fees or if it will send them automatically to the fee destination address
*/
constructor(address _feeToken, address _feeDestination, uint _feeAmount) public {
feeToken = ERC20Token(_feeToken);
feeDestination = _feeDestination;
@ -26,6 +34,7 @@ contract Fees is Ownable {
* @notice Set Fee Destination Address
* @param _addr New address
* @dev Can only be called by the owner of the contract
* TODO: if the contract will be changed to remove ownership, remove this function
*/
function setFeeDestinationAddress(address _addr) public onlyOwner {
feeDestination = _addr;
@ -36,6 +45,7 @@ contract Fees is Ownable {
* @notice Set Fee Amount
* @param _amount New Amount
* @dev Can only be called by the owner of the contract
* TODO: if the contract will be changed to remove ownership, remove this function
*/
function setFeeAmount(uint _amount) public onlyOwner {
feeAmount = _amount;
@ -43,7 +53,7 @@ contract Fees is Ownable {
}
/**
* @notice Withdraw Fees
* @notice Withdraw fees by sending them to the fee destination address
*/
function withdrawFees() public {
uint fees = feeBalance;
@ -52,9 +62,11 @@ contract Fees is Ownable {
emit FeesWithdrawn(fees);
}
/**
* @notice Pay fees for a transaction or element id
* @param _from Address from where the fees are being extracted
* @param _id Escrow id or element identifier to mark as paid
* @dev This will only transfer funds if the fee has not been paid
*/
function payFee(address _from, uint _id) internal {
if(feePaid[_id]) return;
@ -62,7 +74,6 @@ contract Fees is Ownable {
feePaid[_id] = true;
feeBalance += feeAmount;
require(feeToken.allowance(_from, address(this)) >= feeAmount, "Allowance not set for this contract for specified fee");
require(feeToken.transferFrom(_from, address(this), feeAmount), "Unsuccessful token transfer");
}

View File

@ -1,7 +1,6 @@
/* solium-disable security/no-block-members */
/* solium-disable security/no-inline-assembly */
pragma solidity ^0.5.8;
pragma solidity >=0.5.0 <0.6.0;
import "../common/Ownable.sol";
import "../token/ERC20Token.sol";
@ -13,8 +12,6 @@ import "../token/ApproveAndCallFallBack.sol";
*/
contract License is Ownable, ApproveAndCallFallBack {
uint256 public price;
string private constant LICENSE_ALREADY_BOUGHT = "License already bought";
string private constant UNSUCCESSFUL_TOKEN_TRANSFER = "Unsuccessful token transfer";
ERC20Token token;
@ -23,22 +20,24 @@ contract License is Ownable, ApproveAndCallFallBack {
uint creationTime;
}
mapping(address => LicenseDetails) public licenseDetails;
address[] public licenseOwners;
mapping(address => uint) public idxLicenseOwners;
mapping(address => LicenseDetails) public licenseDetails;
event Bought(address buyer, uint256 price);
event PriceChanged(uint256 _price);
constructor(address payable _tokenAddress, uint256 _price) public {
/**
* @param _tokenAddress Address of token used to pay for licenses (SNT)
* @param _price Price of the licenses
*/
constructor(address _tokenAddress, uint256 _price) public {
price = _price;
token = ERC20Token(_tokenAddress);
}
/**
* @dev Check if the address already owns a license
* @notice Check if the address already owns a license
* @param _address The address to check
* @return bool
*/
@ -47,8 +46,8 @@ contract License is Ownable, ApproveAndCallFallBack {
}
/**
* @dev Buy a license
* @notice Requires value to be equal to the price of the license.
* @notice Buy a license
* @dev Requires value to be equal to the price of the license.
* The msg.sender must not already own a license.
*/
function buy() public {
@ -56,14 +55,13 @@ contract License is Ownable, ApproveAndCallFallBack {
}
/**
* @dev Buy a license
* @notice Requires value to be equal to the price of the license.
* @notice Buy a license
* @dev Requires value to be equal to the price of the license.
* The _owner must not already own a license.
*/
function buyFrom(address _owner) private {
require(licenseDetails[_owner].creationTime == 0, LICENSE_ALREADY_BOUGHT);
require(token.allowance(_owner, address(this)) >= price, "Allowance not set for this contract to expected price");
require(token.transferFrom(_owner, address(this), price), UNSUCCESSFUL_TOKEN_TRANSFER);
require(licenseDetails[_owner].creationTime == 0, "License already bought");
require(token.transferFrom(_owner, address(this), price), "Unsuccessful token transfer");
licenseDetails[_owner] = LicenseDetails({
price: price,
@ -77,9 +75,9 @@ contract License is Ownable, ApproveAndCallFallBack {
}
/**
* @dev Set the price
* @notice Set the license price
* @param _price The new price of the license
* @notice Only the owner of the contract can perform this action
* @dev Only the owner of the contract can perform this action
*/
function setPrice(uint256 _price) public onlyOwner {
price = _price;
@ -98,6 +96,8 @@ contract License is Ownable, ApproveAndCallFallBack {
* @notice Withdraw not reserved tokens
* @param _token Address of ERC20 withdrawing excess, or address(0) if want ETH.
* @param _beneficiary Address to send the funds.
* @dev TODO: determine if we will send the fee to some address, if there's a slashing mechanism, or if
* seller can forfeit their license, and remove this function accordinglu
**/
function withdrawExcessBalance(address _token, address payable _beneficiary) external onlyOwner {
require(_beneficiary != address(0), "Cannot burn token");
@ -110,13 +110,12 @@ contract License is Ownable, ApproveAndCallFallBack {
}
}
/**
* @notice Support for "approveAndCall". Callable only by `token()`.
* @param _from Who approved.
* @param _amount Amount being approved, need to be equal `price()`.
* @param _token Token being approved, need to be equal `token()`.
* @param _data Abi encoded data with selector of `register(bytes32,address,bytes32,bytes32)`.
* @param _data Abi encoded data with selector of `buy(and)`.
*/
function receiveApproval(address _from, uint256 _amount, address _token, bytes memory _data) public {
require(_amount == price, "Wrong value");
@ -124,23 +123,17 @@ contract License is Ownable, ApproveAndCallFallBack {
require(_token == address(msg.sender), "Wrong call");
require(_data.length == 4, "Wrong data length");
bytes4 sig = abiDecodeRegister(_data);
require(
sig == bytes4(0xa6f2ae3a), //bytes4(keccak256("buy()"))
"Wrong method selector"
);
require(abiDecodeRegister(_data) == bytes4(0xa6f2ae3a), "Wrong method selector"); //bytes4(keccak256("buy()"))
buyFrom(_from);
}
/**
* @dev Decodes abi encoded data with selector for "buy()".
* @param _data Abi encoded data.
* @return Decoded registry call.
*/
function abiDecodeRegister(bytes memory _data) public pure returns(bytes4 sig) {
function abiDecodeRegister(bytes memory _data) internal pure returns(bytes4 sig) {
assembly {
sig := mload(add(_data, add(0x20, 0)))
}

View File

@ -54,7 +54,7 @@ contract("License", function () {
await SNT.methods.approve(License.options.address, 5).send();
await License.methods.buy().send({from: accounts[0]});
} catch(error) {
assert.strictEqual(error.message, "VM Exception while processing transaction: revert Allowance not set for this contract to expected price");
assert.strictEqual(error.message, "VM Exception while processing transaction: revert Unsuccessful token transfer");
}
});