Token and vouching mechanics #115

fiiiu · 2018-09-21T20:20:23Z

This PR adds a new package implementing the ZEP token and the Vouching mechanics, closing #101 and #108.

Pending Work:

The contracts are not yet upgradeable, we are tackling this on a new branch on top of this one (Make Vouching contract upgradeable #116).
TPL is not yet integrated, so the token contract is some of a placeholder. Same as above regarding further work (Add TPL support to token contract #103).

We are opening this as is so as to get a first round of review on the vouching mechanics, which are independent of the above.

Notes:

Right now, an attacker can register a new dependency with a fresh name, but with an address already listed in the system. This does not open any obvious vulnerability, but might cause confusion with respect to who the owner of a dependency is. Ultimately, this stems from the fact that the indexing is done with dependency names instead of their addresses.

(Opening on master since that's where we started from.. Cannot automatically merge on next :-/ )

spalladino

Awesome work! I added a few minor comments (mostly cleanup), and a few questions we may want to consider, mainly whether we want to accept vouching from non-owner addresses (for simplicity, in case the dev is not the one holding the tokens).

spalladino · 2018-09-21T20:27:31Z

packages/vouching/.gitattributes

@@ -0,0 +1 @@
+*.sol linguist-language=Solidity


Should we move this to the root of the repo?

spalladino · 2018-09-21T20:28:03Z

packages/vouching/contracts/Migrations.sol

@@ -0,0 +1,23 @@
+pragma solidity ^0.4.23;
+
+contract Migrations {


Let's remove this contract altogether

spalladino · 2018-09-21T20:28:51Z

packages/vouching/contracts/Vouching.sol

+
+
+import "./ZepToken.sol";
+import "openzeppelin-solidity/contracts/math/SafeMath.sol";


Ok for now, but keep in mind that we'll need to use oz-zos and not oz-sol when tackling #116

spalladino · 2018-09-21T20:30:04Z

packages/vouching/contracts/Vouching.sol

+contract Vouching {
+  using SafeMath for uint256;
+
+  struct Dependency {


Not a fan of the name. Dependency makes sense in a context where it is a dependency of something else. I'd prefer PackageEntry or something like that.

Also don't like Dependency, but PackageEntry is somewhat confusing.. I don't have a better proposal though, any other ideas? Entry on its own is too dry?

I find Entry a bit generic.
We're kinda indexing packages by their name in the registry mapping, so how about PackageIndexEntry ?

How about Vouchable? And I'd change dependencyAddress to packageAddress.

spalladino · 2018-09-21T20:30:21Z

packages/vouching/contracts/Vouching.sol

+
+  mapping (string => Dependency) private _registry;
+  uint256 private _minimumStake;
+  ZepToken private _token;


Let's use a generic ERC20 here

Will do, but may I ask why though? It's not like we will use Vouching with other tokens..

In particular, we might need to call other specific ZEP methods when doing the TPL integration..

As discussed offline, we'll move to use a generic ERC20 here to keep things decoupled.

spalladino · 2018-09-21T20:38:31Z

packages/vouching/package.json

+  },
+  "keywords": [],
+  "author": "",
+  "license": "ISC",


Aren't we using MIT so far in the other packages?

spalladino · 2018-09-21T20:38:41Z

packages/vouching/package.json

@@ -0,0 +1,24 @@
+{
+  "name": "zos-vouching",
+  "version": "1.0.0",


Let's follow the same version as the other packages.

spalladino · 2018-09-21T20:40:19Z

packages/vouching/contracts/Vouching.sol

+  event DependencyRemoved(string name);
+
+  constructor(uint256 minimumStake, ZepToken token) public {
+    require(token != address(0));


Even if they are not yet supported by truffle, let's add revert reasons to all requires.

packages/vouching/contracts/Vouching.sol

packages/vouching/test/Vouching.test.js

tinchoabbate · 2018-09-24T16:01:18Z

Already pushed some updates. Now waiting for the opened discussions to be settled to make the final changes.

facuspagnuolo

Awesome work guys! 😄 👏

facuspagnuolo · 2018-09-24T16:51:20Z

packages/vouching/contracts/ZepToken.sol

+  string public constant symbol = "ZEP";
+  uint8 public constant decimals = 18;
+
+  uint256 public constant TOTAL_SUPPLY = 100000000 * (10 ** uint256(decimals));


we can use 1e8 instead of 100000000 to be clearer

facuspagnuolo · 2018-09-24T16:54:43Z

packages/vouching/test/helpers/expectEvent.js

+module.exports = {
+  inLogs,
+  inTransaction,
+};


nice! we can move this ones to lib

packages/vouching/test/helpers/assertRevert.js

packages/vouching/test/ZepToken.test.js

facuspagnuolo · 2018-09-24T16:59:53Z

packages/vouching/contracts/Vouching.sol

+    require(initialStake >= _minimumStake, "Initial stake must be equal or greater than minimum stake");
+    require(owner != address(0), "Owner address cannot be zero");
+    require(dependencyAddress != address(0), "Dependency address cannot be zero");
+    // name should not be already registered


this comment is redundant considering the error message

facuspagnuolo · 2018-09-24T17:05:40Z

packages/vouching/contracts/Vouching.sol

+  event DependencyCreated(string name, address indexed owner, address indexed dependencyAddress, uint256 initialStake);
+  event Vouched(string name, uint256 amount);
+  event Unvouched(string name, uint256 amount);
+  event DependencyRemoved(string name);


I'm not sure if indexing strings is still being not simple, here is some research I did for the previous release (see zeppelinos/zos-lib#199)

packages/vouching/contracts/Vouching.sol

* Removed truffle leftovers * Added revert reasons to requires in contract * Refactored getDependency function * Added new test case for unvouch * Updated package version to 1.4.1 and license to MIT

* Tests now use chai (should and bignumber) * Deleted unused import in contract * Made total supply definition clearer in contract

facuspagnuolo

Looking good! Left some comments, we do need to fix some issues on the vouching contract related to the ERC20 standard before merging, sorry that I didn't see that before 🙏

facuspagnuolo · 2018-09-25T11:05:14Z

packages/vouching/contracts/Vouching.sol

+  ERC20 private _token;
+
+  modifier onlyDependencyOwner(string name) {
+    require(msg.sender == _registry[name].owner, "Not allowed");


We could use a more descriptive error message here, sth like "Sender must be the dependency owner"

facuspagnuolo · 2018-09-25T11:06:30Z

packages/vouching/contracts/Vouching.sol

+
+  function remove(string name) external onlyDependencyOwner(name) {
+    // Owner surrenders _minimumStake to the system
+    _token.transfer(msg.sender, _registry[name].stake.sub(_minimumStake));


same here... remember that all the methods of the ERC20 interface are supposed to return a boolean value in order to tell whether the call was successful or not

facuspagnuolo · 2018-09-25T11:06:46Z

packages/vouching/contracts/Vouching.sol

+    require(dependencyAddress != address(0), "Dependency address cannot be zero");
+    require(_registry[name].dependencyAddress == address(0), "The name has already been registered");
+
+    _token.transferFrom(owner, this, initialStake);


This transfer should be wrapped within a require statement

I tackled all instances of this issue using SafeERC20 methods.

facuspagnuolo · 2018-09-25T11:08:26Z

packages/vouching/contracts/Vouching.sol

+    require(initialStake >= _minimumStake, "Initial stake must be equal or greater than minimum stake");
+    require(owner != address(0), "Owner address cannot be zero");
+    require(dependencyAddress != address(0), "Dependency address cannot be zero");
+    require(_registry[name].dependencyAddress == address(0), "The name has already been registered");


What about "Given dependency name was already registererd"?

facuspagnuolo · 2018-09-25T11:08:44Z

packages/vouching/contracts/Vouching.sol

+  }
+
+  function vouch(string name, uint256 amount) external onlyDependencyOwner(name) {
+    _token.transferFrom(msg.sender, this, amount);


facuspagnuolo · 2018-09-25T11:08:51Z

packages/vouching/contracts/Vouching.sol

+    require(remainingStake >= _minimumStake, "Remaining stake must be equal or greater than minimum stake");
+
+    _registry[name].stake = remainingStake;
+    _token.transfer(msg.sender, amount);


facuspagnuolo · 2018-09-25T11:17:17Z

packages/vouching/package.json

+  "devDependencies": {
+    "chai": "^4.1.2",
+    "chai-bignumber": "^2.0.2",
+    "ganache-cli": "6.1.0",


I think we are not using this one right?

packages/vouching/test/helpers/assertRevert.js

packages/vouching/test/helpers/expectEvent.js

* Make Vouching upgradeable * Updated Vouching tests * Updated package dependencies to include zos-lib and openzeppelin-zos * Fix CI * Replace let with const in Vouching test

tinchoabbate · 2018-09-25T18:57:21Z

I'm coming up with a possible implementation for indexing dependency names. Still testing locally. When it's ready, I'll branch out from vouching and then create a PR to merge again.

EDIT: already opened PR (#133)

fiiiu · 2018-09-25T19:41:24Z

packages/vouching/contracts/Vouching.sol

@@ -55,9 +57,9 @@ contract Vouching is Initializable {
    require(initialStake >= _minimumStake, "Initial stake must be equal or greater than minimum stake");
    require(owner != address(0), "Owner address cannot be zero");
    require(dependencyAddress != address(0), "Dependency address cannot be zero");
-    require(_registry[name].dependencyAddress == address(0), "The name has already been registered");
+    require(_registry[name].dependencyAddress == address(0), "Given dependency name was already registererd");


NIT: reads strangely, I propose "The provided name is already registered".

facuspagnuolo

Left some other comments. I think we can merge it after fixing those and keep working the pending issues against master, otherwise this PR will grow a lot

packages/vouching/contracts/Vouching.sol

facuspagnuolo · 2018-09-26T12:07:23Z

packages/vouching/contracts/ZepToken.sol

+   * @dev Constructor that gives msg.sender all of existing tokens, and sets it as the owner
+   */
+  constructor() public {
+    owner = msg.sender;


you shouldn't do this manually since the Ownable constructor is taking care of it

facuspagnuolo · 2018-09-26T12:08:35Z

packages/vouching/test/helpers/assertRevert.js

+
+module.exports = {
+  assertRevert,
+};


Please remove this one since you're already using the one in zos-lib

tinchoabbate · 2018-09-26T18:08:13Z

Already fixed latest issues, should be ready now.

spalladino · 2018-09-26T20:47:41Z

packages/vouching/contracts/Vouching.sol

+contract Vouching {
+  using SafeMath for uint256;
+
+  struct Dependency {


How about Vouchable? And I'd change dependencyAddress to packageAddress.

spalladino · 2018-09-26T21:33:29Z

@facuspagnuolo do you want to update your review, as it is blocking the merge?

facuspagnuolo

LGTM!

fiiiu requested review from spalladino and facuspagnuolo September 21, 2018 20:20

spalladino reviewed Sep 21, 2018

View reviewed changes

facuspagnuolo reviewed Sep 24, 2018

View reviewed changes

fiiiu and others added 16 commits September 24, 2018 16:34

Vouching interface

27c7053

added ZepToken and npm conf files

d9f2249

added ZepToken dependency in Vouching

cee9932

added git syntax highlighting

23bd13b

first vouching implementation

d632ea2

Add gitignore file

cef43ef

Add initial test suite and contract fixes

9f76db1

Add SafeMath to Vouching contract

84dc1e1

added ZepToken tests, simplified testing scripts, added TravisCI

325c529

Update Vouching contract and tests

850c575

added vouching README and modified other READMEs accordingly

e321dbb

small test fixes

116133e

removed mint and burn capabilities from token in favor of fixed supply

b41bffa

Update Vouching contract, tests and package

1d6650b

* Removed truffle leftovers * Added revert reasons to requires in contract * Refactored getDependency function * Added new test case for unvouch * Updated package version to 1.4.1 and license to MIT

Vouching: code cleanup an token now is ERC20

ba980e8

ZepToken and related tests update

ecfc2e6

* Tests now use chai (should and bignumber) * Deleted unused import in contract * Made total supply definition clearer in contract

spalladino force-pushed the vouching branch from 3e1abf5 to ecfc2e6 Compare September 24, 2018 19:36

Fix CI

2cf5c5a

facuspagnuolo suggested changes Sep 25, 2018

View reviewed changes

tinchoabbate mentioned this pull request Sep 25, 2018

Make Vouching upgradeable #118

Merged

tinchoabbate added 5 commits September 25, 2018 15:35

Make Vouching upgradeable (#118)

dee9e74

* Make Vouching upgradeable * Updated Vouching tests * Updated package dependencies to include zos-lib and openzeppelin-zos * Fix CI * Replace let with const in Vouching test

Remove ganache-cli dependency

a2faead

Add SafeERC20 to Vouching

0d2e59c

Use assertRevert from zos-lib in Vouching test

4679b42

Improve Vouching require messages

08f314d

tinchoabbate mentioned this pull request Sep 25, 2018

Consider indexing name argument in all Vouching events #132

Closed

fiiiu commented Sep 25, 2018

View reviewed changes

facuspagnuolo reviewed Sep 26, 2018

View reviewed changes

tinchoabbate added 4 commits September 26, 2018 12:39

Remove unnecessary test helper

fb77688

Remove unnecessary owner assignment in ZepToken

8051d00

Move transfers to last part of Vouching functions

9f74137

Fix typo in require message

95ffe36

facuspagnuolo assigned spalladino Sep 26, 2018

spalladino approved these changes Sep 26, 2018

View reviewed changes

facuspagnuolo approved these changes Sep 27, 2018

View reviewed changes

spalladino merged commit c068387 into master Sep 27, 2018

spalladino deleted the vouching branch October 25, 2018 09:29

		@@ -0,0 +1,23 @@
		pragma solidity ^0.4.23;

		contract Migrations {



		import "./ZepToken.sol";
		import "openzeppelin-solidity/contracts/math/SafeMath.sol";

Token and vouching mechanics #115

Token and vouching mechanics #115

Conversation

fiiiu commented Sep 21, 2018 • edited Loading

spalladino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinchoabbate commented Sep 24, 2018

facuspagnuolo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facuspagnuolo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinchoabbate commented Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

facuspagnuolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinchoabbate commented Sep 26, 2018

Choose a reason for hiding this comment

spalladino commented Sep 26, 2018

facuspagnuolo left a comment

Choose a reason for hiding this comment

fiiiu commented Sep 21, 2018 •

edited

Loading

facuspagnuolo left a comment •

edited

Loading

facuspagnuolo left a comment •

edited

Loading

tinchoabbate commented Sep 25, 2018 •

edited

Loading