Skip to content
This repository has been archived by the owner on Feb 6, 2019. It is now read-only.

Initial/draft implementation of token rules. #161

Open
wants to merge 2 commits into
base: feature/token-holder
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
35 changes: 35 additions & 0 deletions contracts/SharedStructs.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
pragma solidity ^0.4.24;


// Copyright 2018 OpenST Ltd.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.


library SharedStructs {

/* Structs */

struct TokenRule {
address ruleAddress;
}

struct TokenRuleConstraint {
Copy link
Contributor

@jasonklein jasonklein Aug 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the idea behind a constraint to give the flexibility of having global constraints that apply to one or more (or every?) token rule but that is not reflected in any individual token rule?

  • if yes:
    • I'd argue this is an enhancement that may not be apposite for a first complete iteration.
    • but if the decision is to retain the concept all the same, I suggest renaming to "TokenRulesConstraint".
  • if no—each constraint will be specific to a token rule—then I suggest reflecting that in the token rule, and not in the token rules

address constraintAddress;
}

struct TokenRuleTransfer {
address to;
uint256 amount;
}
}
55 changes: 55 additions & 0 deletions contracts/SimplePaymentRule.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
pragma solidity ^0.4.24;


// Copyright 2018 OpenST Ltd.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import "./TokenRules.sol";
import "./SharedStructs.sol";

contract SimpleTransferRule {

/* Variables */

TokenRules tokenRules;


/* Functions */

constructor (
TokenRules _tokenRules
)
public
{
tokenRules = _tokenRules;
}

function execute (
address _from,
address _to,
uint256 _amount
)
public
returns (bool)
{
SharedStructs.TokenRuleTransfer[] memory transfers = new
SharedStructs.TokenRuleTransfer[](1);
transfers[0].to = _to;
transfers[0].amount = _amount;

tokenRules.executeTransfers(_from, transfers);

return true;
}
}
264 changes: 264 additions & 0 deletions contracts/TokenRules.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,264 @@
pragma solidity ^0.4.24;
pragma experimental ABIEncoderV2;
Copy link
Contributor

@jasonklein jasonklein Aug 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, considering that experimental features may likely still be experimental by the time we intend to have these concepts on Mainnet, please make the necessary changes to not need to rely on those features.


// Copyright 2018 OpenST Ltd.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

import "./BrandedToken.sol";
import "./SharedStructs.sol";
import "./SafeMath.sol";


contract TokenRules {

/* Usings */

using SafeMath for uint256;


/* Events */

event RuleAdded(address _sender, address _ruleAddress);

event RuleRemoved(address _sender, address _ruleAddress);

event ConstraintAdded(address _sender, address _constraintAddress);

event ConstraintRemoved(address _sender, address _constraintAddress);


/* Variables */

mapping (address => SharedStructs.TokenRule) public rules;

SharedStructs.TokenRuleConstraint[] public constraints;

address public organization;
BrandedToken public brandedToken;


/* Modifiers */

modifier onlyOrganization {
require(
organization == msg.sender,
"Only organization is allowed to call."
);
_;
}

modifier onlyRule {
require (
rules[msg.sender].ruleAddress != address(0),
"Only registered rule is allowed to call");
_;
}


/* Functions */

constructor (
address _organization,
BrandedToken _brandedToken
)
public
{
organization = _organization;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these inputs be validated?

brandedToken = _brandedToken;
}

/**
* @param _rule The address of rule to add.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_ruleAddress?

*
* \pre Rule to add is not null.
* \pre Rule to add does not exist.
*
* @return true in case of success, otherwise false.
*/
function addRule (
address _ruleAddress
)
external
onlyOrganization
returns (bool)
{
require(_ruleAddress != address(0), "Rule to add is null.");
require (
rules[_ruleAddress].ruleAddress == address(0),
"Rule to add exists."
);

rules[_ruleAddress].ruleAddress = _ruleAddress;

emit RuleAdded(msg.sender, _ruleAddress);

return true;
}

function removeRule (
address _ruleAddress
)
external
onlyOrganization
returns (bool)
{
require (
rules[_ruleAddress].ruleAddress != address(0),
"Rule to remove does not exist."
);

delete rules[_ruleAddress];

emit RuleRemoved(msg.sender, _ruleAddress);

return true;
}


function executeTransfers (
address _from,
SharedStructs.TokenRuleTransfer[] _transfers
)
external
view
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that, because this calls non-view functions (transferFrom, clearAllowance), it is not a view function.

onlyRule
returns (bool)
{
if (0 == _transfers.length) {
return true;
}

require (
checkConstraints(_from, _transfers),
"Constraints do not fullfilled."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please strike "do".

);

for (uint256 i = 0; i < _transfers.length; ++i) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe best practice advises avoiding usage of loops in this manner. Should we be concerned about gas here?

brandedToken.transferFrom(
_from,
_transfers[i].to,
_transfers[i].amount
);
}

brandedToken.clearAllowance(_from);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be duplicative with TokenHolder: https://github.com/OpenSTFoundation/openst-contracts/pull/160/files#diff-65cbb9bc9658dc2fe0c2f217fa670020R339

As the approach in TokenHolder is consistent with EIP20, I'd vote for that one; but, ultimately, my primary concern is eliminating the duplication, so please come to a view with @abhayks1 and @gulshanvasnani.


return true;
}

function addConstraint (
address _constraintAddress
)
external
onlyOrganization
returns (bool)
{
require(_constraintAddress != address(0), "Constraint to add is null.");

uint256 index = findConstraintIndex(_constraintAddress);

require (
index == constraints.length,
"Constraint to add already exists."
);

constraints.push(
SharedStructs.TokenRuleConstraint(
{constraintAddress: _constraintAddress}
)
);

emit ConstraintAdded(msg.sender, _constraintAddress);

return true;
}

function removeConstraint (
address _constraintAddress
)
external
onlyOrganization
returns (bool)
{
require (
_constraintAddress != address(0),
"Constraint to remvoe is null."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"remove"

);

uint256 index = findConstraintIndex(_constraintAddress);

require (
index != constraints.length,
"Constraint to remove does not exist."
);

removeConstraintByIndex(index);

return true;
}

function checkConstraints (
address _from,
SharedStructs.TokenRuleTransfer[] _transfers
)
public
view
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to mark this as pure? Or no because it is clearly a work in progress?

returns (bool)
{
return true;
}

/**
* @dev Finds index of constraint.
*
* @param _constraint Constraint to find in constraints array.
*
* @return index_ Returns index of the constraint if exists,
* otherwise returns constraints.length.
*/
function findConstraintIndex(address _constraint)
private
view
returns (uint256 index_)
{
index_ = 0;
while (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as earlier about loops through dynamic-length arrays.

index_ < constraints.length &&
constraints[index_].constraintAddress != _constraint
)
{
++index_;
}
}

// solium-disable-next-line security/no-assign-params
function removeConstraintByIndex(uint256 _index)
private
returns (bool)
{
require (
_index < 0 || _index >= constraints.length,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the condition _index < 0 needed given that _index is uint256?

"Index is out of range."
);

while ( _index < constraints.length - 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And still the same question about loops :-)

constraints[_index] = constraints[_index + 1];
_index++;
}

--constraints.length;

return true;
}
}
11 changes: 11 additions & 0 deletions contracts/openst-protocol/EIP20Token.sol
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,17 @@ contract EIP20Token is EIP20Interface {
return true;
}

function clearAllowance(address _approver)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If resetting the allowance to 0 is kept in TokenHolder, then this change should be struck.

public
returns (bool)
{
allowed[_approver][msg.sender] = 0;

emit Approval(_approver, msg.sender, 0);

return true;
}

/**
* @notice Internal function claimEIP20.
*
Expand Down