skmgoldin / tcr Goto Github PK

It would reduce cognitive overhead for users if the exitPeriodLen was just the exitTimeDelay value. I don't think it needs to be overcomplicated. We already have a bunch of complicated parameter names.

now + get("pApplyStageLen") and now + get("pApplyStageLen") + get("pCommitStageLen") + get("pRevealStageLen") + PROCESSBY can overflow.

Recommendation

Use SafeMath for arithmetic, and assert that these calculations succeed in processProposal so proposals can't get the contract in a state where the arithmetic for new proposals will revert every transaction.

As of f89eb89, coverage output looks like this:

--------------------|----------|----------|----------|----------|----------------|
File                |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
--------------------|----------|----------|----------|----------|----------------|
 contracts/         |    97.36 |    66.87 |    98.18 |    97.04 |                |
  PLCRVoting.sol    |     96.2 |    55.17 |    95.45 |    95.24 |102,103,105,370 |
  Parameterizer.sol |    98.82 |    68.18 |      100 |    98.85 |            223 |
  Registry.sol      |    97.03 |    76.56 |      100 |    96.97 |    136,137,139 |
--------------------|----------|----------|----------|----------|----------------|
All files           |    97.36 |    66.87 |    98.18 |    97.04 |                |
--------------------|----------|----------|----------|----------|----------------|

% Stmts should be at least 95% and % Branch should be at least 85%. These targets currently exclude tests for where token transfers fail. They also exclude PLCRVoting.sol, since that will be consumed from EPM in the future, and the tests really belong in the PLCRVoting repository.

Non-covered test cases to be added:

Parameterizer

proposeRarameterization

• should revert if the proposal is a NOOP

challengeReparameterization

• should revert if the proposal does not exist
• should revert if the proposal already has a challenge against it

processProposal

• should revert in the else case
• (these may not be possible to test)
  • should revert if the set dispensationPct is greater than 100
  • should revert if the set pDispensationPct is greater than 100

claimReward

• should revert if the sender has already claimed their tokens

resolveChallenge

• cover case where the challenge failed and the processBy date has passed
  • it should not set the value, and it should transfer tokens to the proposal owner

Registry

apply

• should revert if the deposit amount is less than the minDeposit

withdraw

• should revert if the message sender is not the owner of the listing
• should be able to withdraw tokens and decrease the unstaked deposit

exit

• should revert if the listing is in the application stage

challenge

• should revert if the application is not in the apply stage and is not on the whitelist
• should revert if the application currently has an open challenge against it

determineReward

• should revert if the challenge has already been resolved
• should revert if the poll has not ended yet

As we know, in Partial Lock Commit Reveal voting (PLCR), voters can vote with the same tokens across multiple polls. This surfaces a few issues when a TCR has MINORITY_BLOC_SLASH (MBS) implemented in order to deter vote splitting. Here’s why:

Problem:

If MINORITY_BLOC_SLASH is set to 100% and a voter votes across multiple polls with the same tokens within a PLCR environment, and once the voter ends up in a losing bloc (100% of their tokens are taken due to MINORITY_BLOC_SLASH being set at 100%), there won’t be any token left for that voter to cover the debt for any remaining polls still open.

Solution:

We can solve for this by setting the MINORITY_BLOC_SLASH lower than 100%. Let’s say that the Minority_Bloc_Slash is set to 10%.

Problem:

At a 10% Minority_Bloc_Slash, we still run into issues.
For example, if a voter uses 100 tokens to vote across 11 polls and the Min_Bloc_Slash is set to 10%. If the voter loses in every poll then 10% is taken out of 100 tokens 11 times, we can see the dilemma. In this scenario, the user will end up with a negative token balance: 100 - 110 = -10.

Solution:

We can solve for this however by implementing the MINORITY_BLOC_SLASH to deduct tokens in chronological order. For example, taking the above scenario of a voter using 100 tokens to vote across multiple polls with a MINORITY_BLOC_SLASH set to 10%. See below.

Conclusion:

For TCR’s using PLCR in conjunction with MINORITY_BLOC_SLASH (as a deterrent for vote splitting), the MINORITY_BLOC_SLASH shouldn’t be set to 100%.
If MINORITY_BLOC_SLASH is set to a percentage less than 100%, then in order to prevent a voter who is voting with the same tokens across multiple polls from ending with a negative balance, we should deduct the MINORITY_BLOC_SLASH percentage chronologically (or implement some other logic to address the potential for a voter to end with a negative balance).

npm install runs truffle install, ahh!

e.g. the as function can be confusing for first-timers

let's see what happens when we change this scary number

Does anyone object to Apache-2?

In addition to a version, a TCR should have a name like "AdChain Registry".

100 - get("pDispensationPct") can underflow.

Recommendation

Use SafeMath for arithmetic, and assert that these calculations succeed in processProposal so proposals can't get the contract in a state where the arithmetic for new challenges will revert every transaction.

The variable name is confusing. Something like voterRewardClaimed is clearer.

Hey all,

While parsing and handling TCR data I came across a bit of a need to have some standards or "suggested standards" around TCR data. Below I want to walk through some potential examples, and ideas around standard TCR data formatting.

Motivation

Many apps and systems will need some kind of standard API or data format to fallback on for some data primitives. It would be nice to have some basic suggested standards around this to simplify third-party system architecture.

Example Formats

Plain Text:

Description

The plaintext option is simply the data in question and does not need to be formatted.

Examples:

data => "nick.com"
data => "MyNameIsThis"
data => "Hello world"

IPFS Hash

Description

This option describes an IPFS hash, which should lead to a reducible JSON data schema

Examples:

data => QmTp2hEo8eXRp6wg7jXv1BLCMh5a4F3B7buAUZNZUu772j

JSON Data Format:
QmTp2hEo8eXRp6wg7jXv1BLCMh5a4F3B7buAUZNZUu772j =>

{
    "title": "My application title",
    "name": "Data Name",
    "description": "My application description",
    "shortDescription": "My short application description",
    "content": "My application content"
}

JSON Schemas can follow some existing standards if possible:
Products: http://schema.org/Product
Persons: http://schema.org/Person
Offers: http://schema.org/Offer
Buisineses: http://schema.org/LocalBusiness

Thoughts welcome!

truffle exec scripts/deploy_proxies.js failed silently.

Using network 'development'.

RegistryFactory:   0x23e84570f7b28ad0d5e5545d398009194a8734f3

Deploying proxy contracts...

did a truffle debug and got the following error

Parameterizer.sol:

248:     @param _challengeID     the challenge ID to claim tokens for
249:     */
250:     function claimReward(uint _challengeID) public {
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

debug(development:0xb38b118c...)> 

Transaction halted with a RUNTIME ERROR.

This is likely due to an intentional halting expression, like assert(), require() or revert(). It can also be due to out-of-gas exceptions. Please inspect your transaction parameters and contract code to determine the meaning of this error.

PLCRVoting determines the winner with (100 * poll.votesFor) > (poll.voteQuorum * (poll.votesFor + poll.votesAgainst)). As a result, polls resolve as failing when they expire with no votes. This is probably the right behavior, but it's surprising.

Recommendation

Document this behavior.

The purpose of the _data string in the apply function is for applicants to be able to furnish arbitrarily complex data related to their application. For example, an IPFS hash which resolves to:

{
  "name": "harvard",
  "average_student_debt": "10000 USD",
  "weather_in_locale": "poor"
}

In the TCR of top colleges, the identifier string here is that listed under the "name" property. The _listingHash should be keccak256("harvard"), and token-holders should strongly suspect they are being scammed if the supplied name does not match the supplied _listingHash.

A separate example could be:

{
  "domain": "consensys.net",
  "scamvoid_score": "high"
}

Here, the "domain" is the identifier string.

User interfaces should be able to flag to users when an identifier in a _data object does not match the provided _listingHash. This will require some way of detecting what the identifier property is. Perhaps for the purposes of user interfaces, by convention, an 'identifier' property in the _data object could be required?

not just in the audit findings

I find it unclear from the name what updateStatus() is supposed to do, which makes it difficult to remember when it is necessary to call it.

From the code, it looks like it can either

1. Whitelist an application that has passed the application period without a challenge
2. If there is a challenge that has finished its voting period (whether it's a new application or an existing listing), remove, add, or reject the applicant/listing

Is there some name that covers both aspects? Otherwise, it might make sense to split updateStatus() into two separate functions. Something like finalizeApplication() and finalizeChallenge()?

REGISTRY

_Application: add appEndDate

_Challenge: remove deposit, add challenger, change pollID to challengeID

_NewListingWhitelisted: change to _ApplicationWhitelisted

Add: (#35 & #45)

• _ListingWithdrawn(bytes32 listingHash);
• _TouchAndRemoved(bytes32 listingHash);

Current Implementation:

event _Application(bytes32 listingHash, uint deposit, string data);
event _Challenge(bytes32 listingHash, uint deposit, uint pollID, string data);
event _Deposit(bytes32 listingHash, uint added, uint newTotal);
event _Withdrawal(bytes32 listingHash, uint withdrew, uint newTotal);
event _NewListingWhitelisted(bytes32 listingHash);
event _ApplicationRemoved(bytes32 listingHash);
event _ListingRemoved(bytes32 listingHash);
event _ChallengeFailed(uint challengeID);
event _ChallengeSucceeded(uint challengeID);
event _RewardClaimed(address voter, uint challengeID, uint reward);

Proposed implementation:

* event _Application(bytes32 listingHash, uint deposit, uint appEndDate, string data);
* event _Challenge(bytes32 listingHash, address challenger, uint challengeID, string data);
  event _Deposit(bytes32 listingHash, uint added, uint newTotal);
  event _Withdrawal(bytes32 listingHash, uint withdrew, uint newTotal);
* event _ApplicationWhitelisted(bytes32 listingHash);
  event _ApplicationRemoved(bytes32 listingHash);
  event _ListingRemoved(bytes32 listingHash);
+ event _ListingWithdrawn(bytes32 listingHash);
+ event _TouchAndRemoved(bytes32 listingHash);
  event _ChallengeFailed(uint challengeID);
  event _ChallengeSucceeded(uint challengeID);
  event _RewardClaimed(address voter, uint challengeID, uint reward);

PARAMETERIZER

Add: (#34)

• _RewardClaimed(msg.sender, _challengeID, reward);

Add: (#33)

• _ProposalAccepted(bytes32 propID);
• _ProposalUnprocessed(bytes32 propID);
• _ChallengeSucceeded(uint challengeID);
• _ChallengeFailed(uint challengeID);

Current:

event _ReparameterizationProposal(address proposer, string name, uint value, bytes32 propID);
event _NewChallenge(address challenger, bytes32 propID, uint pollID);

Proposed:

  event _ReparameterizationProposal(address proposer, string name, uint value, bytes32 propID);
  event _NewChallenge(address challenger, bytes32 propID, uint pollID);
+ event _ProposalAccepted(bytes32 propID);
+ event _ProposalUnprocessed(bytes32 propID);
+ event _ChallengeSucceeded(uint challengeID);
+ event _ChallengeFailed(uint challengeID);
+ event _RewardClaimed(address voter, uint challengeID, uint reward);

PLCRVOTING

PollCreated: change args to commitEndDate, revealEndDate

Add: underscores for convention

Experimental:
VoteRevealed: change to (voter, pollID, votesFor, votesAgainst)

Current:

event VoteCommitted(address voter, uint pollID, uint numTokens);
event VoteRevealed(address voter, uint pollID, uint numTokens, uint choice);
event PollCreated(uint voteQuorum, uint commitDuration, uint revealDuration, uint pollID);
event VotingRightsGranted(address voter, uint numTokens);
event VotingRightsWithdrawn(address voter, uint numTokens);

Proposed:

_  event _VoteCommitted(address voter, uint pollID, uint numTokens);
*  event _VoteRevealed(address voter, uint pollID, uint votesFor, uint votesAgainst);
*  event _PollCreated(uint voteQuorum, uint commitEndDate, uint revealEndDate, uint pollID);
_  event _VotingRightsGranted(address voter, uint numTokens);
_  event _VotingRightsWithdrawn(address voter, uint numTokens);

I copied this code from plcr factory. In this implementation, this chunk of code couldn't actually be possible, right? because the voting and parameterizer contracts need to know the token address when they are instantiated. this function creates a token but the voting and parameterizer contracts wont accept it.

@skmgoldin we're planning to put PLCRFactory on ethpm and consume it here right?

correct me if im wrong, but it seems the only place we need the option create a new token is in the plcr factory

Hello,

I was wondering what's the use of masterCopy var that's used in all 3 contracts ? i'm guessing it's for creating copies of contracts safely as mentioned here ethereum/solidity#2296

if so, my noob questions are:

• How does someone create a copy of the contract using this method?
• Where is masterCopy set ? I looked at setup and the constructor but can't seem to find it. I also looked at the tests but couldn't find that out.

I appreciate you taking the time to answer these questions. and I do apologize if they seem like simple questions 🙇 😄

Cheers.

When a user submits a proposal and it goes unchallenged, the user does not get their token back after the processProposal function is invoked.

In the processProposal function in the first condition if(canBeSet(_propID)) the value is being set but the token is never transferred back to the owner.

There should be a token.transfer within this condition.

Tokens are being locked in contract.

https://github.com/skmgoldin/tcr/blob/master/contracts/Parameterizer.sol#L221

If pMinDeposit is changed after a proposal has been created, tokens will not be distributed properly.

If pMinDeposit is increased, challenge.stake and challenge.rewardPool will be based on a larger value than what the proposer deposited. Winning challengers will attempt to withdraw (2 * challenges[_challengeID].stake) - challenges[_challengeID].rewardPool, which will fail if the deposit has been increased enough to make the reward pool larger than the proposer's deposit. Even when it succeeds, the reward pool will be larger than the balance that remains the contract, so some voters won't get rewarded.

If pMinDeposit is decreased, challenge.stake and challenge.rewardPool will be based on a smaller value than what the proposer deposited. Winning proposed will withdraw (2 * challenges[_challengeID].stake) - challenges[_challengeID].rewardPool, which will leave some tokens allocated neither to the winning proposer nor the winning voters. The tokens will remain stuck in the contract.

Recommendation

In challengeReparameterization, don't reference pMinDeposit. Reference proposal.deposit so challenges are always consistent with their proposals.

Sorry if this is not the right place to ask, but I was wondering to what extend have the smart contracts been audited?

By setting deployToken to false you can specify an address for any token whose API is a superset of ERC20. Can this be made even more modular in the migrations?

what happens when _name is not one of the reparameterizable parameters?

  function proposeReparameterization(string _name, uint _value) public returns (bytes32) {
    uint deposit = get("pMinDeposit");
    bytes32 propID = keccak256(_name, _value);

    require(!propExists(propID)); // Forbid duplicate proposals
    require(get(_name) != _value); // Forbid NOOP reparameterizations
    require(token.transferFrom(msg.sender, this, deposit)); // escrow tokens (deposit amt)

    // attach name and value to pollID    
    proposals[propID] = ParamProposal({
      appExpiry: now + get("pApplyStageLen"),
      challengeID: 0,
      deposit: deposit,
      name: _name,
      owner: msg.sender,
      processBy: now + get("pApplyStageLen") + get("pCommitStageLen") +
        get("pRevealStageLen") + PROCESSBY,
      value: _value
    });

    _ReparameterizationProposal(msg.sender, _name, _value, propID);
    return propID;
  }

  function processProposal(bytes32 _propID) public {
    ParamProposal storage prop = proposals[_propID];

    if (canBeSet(_propID)) {
      set(prop.name, prop.value);
    } else if (challengeCanBeResolved(_propID)) {
      resolveChallenge(_propID);
    } else if (now > prop.processBy) {
      require(token.transfer(prop.owner, prop.deposit));
    } else {
      revert();
    }

    delete proposals[_propID];
  }

The intent of this line is to prevent people from making withdrawals that would leave them subject to touch-and-removes. It does that.

A side effect is that while a challenge is active, tokens cannot be withdrawn which would leave the unstaked deposit less tnan minDeposit, which is actually unnecessary. If the listing loses the challenge they will go away, and if they win they will necessarily be left with at least minDeposit tokens unlocked since they get their deposit amount unlocked plus their winnings from the challenge loser. Since multiple challenges cannot be in flight against a listing at the same time, it is okay if during a challenge a user withdraws their entire unstaked deposit.

Presently, Registry.sol imports Parameterizer.sol in its entirety, but only calls the function get(string _name).

You can save gas and storage space by using a ParameterizerInterface.sol in Registry

interface ParameterizerInterface {
        function get(string _name) public view returns (uint value);
}

Hi guys,
I had an issue when tried to debug transaction with the latest truffle 4.1.13

Gathering transaction data...                                                                                                                                                                                                                
                                                                                                                                                                                                                                             
redux-saga error: uncaught at session.saga                                                                                                                                                                                                   
at session.saga                                                                                                                                                                                                                              
 at recordInstance                                                                                                                                                                                                                           
 TypeError: Cannot destructure property `context` of 'undefined' or 'null'.                                                                                                                                                                  
    at Object.addInstance (/Users/theanht1/.nvm/versions/node/v8.7.0/lib/node_modules/truffle/build/webpack:/packages/truffle-debugger/dist/debugger.js:4523:18)                                                                             
    at addInstance.next (<anonymous>)                                                                                                                                                                                                        
    at recordInstance (/Users/theanht1/.nvm/versions/node/v8.7.0/lib/node_modules/truffle/build/webpack:/packages/truffle-debugger/dist/debugger.js:3452:1)                                                                                  
    at recordInstance.next (<anonymous>)                                                                                                                                                                                                     
    at next (/Users/theanht1/.nvm/versions/node/v8.7.0/lib/node_modules/truffle/build/webpack:/~/redux-saga/es/internal/proc.js:311:1)                                                                                                       
    at currCb (/Users/theanht1/.nvm/versions/node/v8.7.0/lib/node_modules/truffle/build/webpack:/~/redux-saga/es/internal/proc.js:388:1)                                                                                                     
    at runSelectEffect (/Users/theanht1/.nvm/versions/node/v8.7.0/lib/node_modules/truffle/build/webpack:/~/redux-saga/es/internal/proc.js:699:1)

I used scripts/deploy_proxies.js to create contracts. I guess this error caused by the factory pattern because I can debug normally with tcr 1.0 (without factory). Have you ever faced it?

Running node 8.11.1, trying to deploy to any test network and receiving the following error.

Running migration: 3_optional_plcr_factory.js
  Linking DLL to PLCRFactory
  Linking AttributeStore to PLCRFactory
  Deploying PLCRFactory...
  ... 0x69052b83a484f82fa7444b580ac23061dc9d0caa29cfce34e35bb9abb0886eda
Error encountered, bailing. Network state unknown. Review successful transactions manually.
Error: The contract code couldn't be stored, please check your gas amount.
    at Object.callback (/Users/drewstone/src/eth/cw/tcr/node_modules/truffle/build/webpack:/~/web3/lib/web3/contract.js:147:1)
    at /Users/drewstone/src/eth/cw/tcr/node_modules/truffle/build/webpack:/~/web3/lib/web3/method.js:142:1
    at /Users/drewstone/src/eth/cw/tcr/node_modules/truffle/build/webpack:/~/web3/lib/web3/requestmanager.js:89:1
    at /Users/drewstone/src/eth/cw/tcr/node_modules/truffle/build/webpack:/~/truffle-migrate/index.js:225:1
    at /Users/drewstone/src/eth/cw/tcr/node_modules/truffle/build/webpack:/~/truffle-provider/wrapper.js:134:1
    at /Users/drewstone/src/eth/cw/tcr/node_modules/web3-provider-engine/index.js:152:9
    at /Users/drewstone/src/eth/cw/tcr/node_modules/async/internal/once.js:12:16
    at replenish (/Users/drewstone/src/eth/cw/tcr/node_modules/async/internal/eachOfLimit.js:61:25)
    at /Users/drewstone/src/eth/cw/tcr/node_modules/async/internal/eachOfLimit.js:71:9
    at eachLimit (/Users/drewstone/src/eth/cw/tcr/node_modules/async/eachLimit.js:43:36)
    at /Users/drewstone/src/eth/cw/tcr/node_modules/async/internal/doLimit.js:9:16
    at end (/Users/drewstone/src/eth/cw/tcr/node_modules/web3-provider-engine/index.js:127:5)
    at /Users/drewstone/src/eth/cw/tcr/node_modules/web3-provider-engine/subproviders/provider.js:20:5
    at XMLHttpRequest.request.onreadystatechange (/Users/drewstone/src/eth/cw/tcr/node_modules/web3/lib/web3/httpprovider.js:118:13)
    at XMLHttpRequestEventTarget.dispatchEvent (/Users/drewstone/src/eth/cw/tcr/node_modules/web3/node_modules/xhr2/lib/xhr2.js:64:18)
    at XMLHttpRequest._setReadyState (/Users/drewstone/src/eth/cw/tcr/node_modules/web3/node_modules/xhr2/lib/xhr2.js:354:12)
    at XMLHttpRequest._onHttpResponseEnd (/Users/drewstone/src/eth/cw/tcr/node_modules/web3/node_modules/xhr2/lib/xhr2.js:509:12)
    at IncomingMessage.<anonymous> (/Users/drewstone/src/eth/cw/tcr/node_modules/web3/node_modules/xhr2/lib/xhr2.js:469:24)
    at emitNone (events.js:111:20)
    at IncomingMessage.emit (events.js:208:7)
    at endReadableNT (_stream_readable.js:1064:12)
    at _combinedTickCallback (internal/process/next_tick.js:138:11)
    at process._tickCallback (internal/process/next_tick.js:180:9)

Not sure if I'm missing something obvious, but config.json doesn't seem to be actually used anywhere other than in tests. However, the README seems to imply that config.json actually does something:

To deploy your own TCR, first open up `conf/config.json`...

On that note: there isn't actually a way to automatically deploy a TCR, is there? When I run truffle migrate, only the factories are deployed. Wouldn't I have to manually call the factory functions to create TCRs?

This is all good; it's just that the README makes it sound like there's a way to just edit config.json and have a TCR automatically deployed, but that doesn't seem to be the case.

VOTE_QUORUM was misnamed in the paper and is misnamed in this implementation. What VOTE_QUORUM describes has nothing to do with the normal dictionary definition of a quorum. As-implemented (and as described in the paper), VOTE_QUORUM is the share of tokens voting in favor of a candidate (either a listing or applicant) for that candidate to prevail in a challenge. So it describes either the majority or minority of support necessary for the candidate to become/remain whitelisted.

Proposed new name: CANDIDATE_SUCCESS_THRESHOLD

This helps distinguish when a listing has been touched and removed instead of just a listing has been removed.

Though there is an ethpm.json file in the repo, the tcr package is not published to the registry and cannot be installed on another project via truffle install tcr

Steps to reproduce:

1. Start 2 polls: with ids 1 and 2.
2. Call PLCRVoting.commitVote(1, <secretHash1>, 7, 0). Actual vote inside the hash doesn't matter, can be 0 or 1.
3. Call PLCRVoting.commitVote(2, <secretHash2>, 8, 1).
4. Call PLCRVoting.commitVote(1, <secretHash3>, 9, 2).
5. Call PLCRVoting.getInsertPointForNumTokens(<yourAddress>, 6).
   Expected result: 0.
   Actual result: infinite loop/out of gas.

Here is a test case demonstrating this issue #2

The fix is here skmgoldin/sol-dll#1

I'm presuming that the name of a registry will be a significant coordination point for token holders and users of a registry. Reparameterizing the name to better reflect the values of the TCR as the values settle and shift over time may be valuable.

using node 10 an error was reported

The problem is with with truffle": "4.1.11

> truffle install

TypeError: Cannot read property '1' of null
at module.exports (/Users/doh/repo/tcr/node_modules/truffle/build/webpack:/~/promisify-node/utils/args.js:9:1)
at processExports (/Users/doh/repo/tcr/node_modules/truffle/build/webpack:/~/promisify-node/index.js:61:1)
at processExports (/Users/doh/repo/tcr/node_modules/truffle/build/webpack:/~/promisify-node/index.js:82:1)
at /Users/doh/repo/tcr/node_modules/truffle/build/webpack:/~/promisify-node/index.js:137:1
at Array.forEach (<anonymous>)
at processExports (/Users/doh/repo/tcr/node_modules/truffle/build/webpack:/~/promisify-node/index.js:132:4)
at module.exports (/Users/doh/repo/tcr/node_modules/truffle/build/webpack:/~/promisify-node/index.js:164:1)
at Object.<anonymous> (/Users/doh/repo/tcr/node_modules/truffle/build/webpack:/~/ethpm/lib/installer.js:2:1)
at __webpack_require__ (/Users/doh/repo/tcr/node_modules/truffle/build/webpack:/webpack/bootstrap b4601922d6f11f8bff0b:19:1)
at Object.<anonymous> (/Users/doh/repo/tcr/node_modules/truffle/build/webpack:/~/ethpm/index.js:2:1)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] install: `truffle install`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the [email protected] install script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/doh/.npm/_logs/2018-09-10T07_37_42_278Z-debug.logtruffle": "4.1.11```

If PLCRVoting, Parameterizer, and Registry work with any EIP20 token, use the EIP20Interface type rather than ConsenSys's specific implementation of it.

Hi,
I'm using:
node v8.11.3
npm v5.6.0
web3 v1.0.0-beta.35

and I can't install the tcr. Here's what happens
$ npm install
[email protected] install /home/sebi/bc/tcr
truffle install
Error: Invalid JSON RPC response: "" at Object.InvalidResponse (/home/sebi/bc/tcr/node_modules/truffle/build/webpack:/~/web3/lib/web3/errors.js:38:1) at XMLHttpRequest.request.onreadystatechange (/home/sebi/bc/tcr/node_modules/truffle/build/webpack:/~/web3/lib/web3/httpprovider.js:125:1) at XMLHttpRequestEventTarget.dispatchEvent (/home/sebi/bc/tcr/node_modules/truffle/build/webpack:/~/xhr2/lib/xhr2.js:64:1) at XMLHttpRequest._setReadyState (/home/sebi/bc/tcr/node_modules/truffle/build/webpack:/~/xhr2/lib/xhr2.js:354:1) at XMLHttpRequest._onHttpRequestError (/home/sebi/bc/tcr/node_modules/truffle/build/webpack:/~/xhr2/lib/xhr2.js:544:1) at ClientRequest.<anonymous> (/home/sebi/bc/tcr/node_modules/truffle/build/webpack:/~/xhr2/lib/xhr2.js:414:1) at emitOne (events.js:116:13) at ClientRequest.emit (events.js:211:7) at TLSSocket.socketErrorListener (_http_client.js:387:9) at emitOne (events.js:116:13)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] install: truffle install
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the [email protected] install script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
npm ERR! A complete log of this run can be found in:
npm ERR! /home/sebi/.npm/_logs/2018-08-13T19_03_53_085Z-debug.log

Any ideas on what the problem might be?

Thanks!

In the Registry.sol contract there is a claimVoterReward function which would be very useful in Parameterizer.sol for determining whether or not the reward has been claimed.

Using web3, it is not possible to retrieve the voterCanClaimReward mapping from the challenges data. So for UI development it cannot be determined if the rewards have been claimed aside from the user's account receiving the token.

There is no problem claiming rewards, there is just no way using web3 to determine whether it's been claimed already.

It can be confusing to see what "reward" is referring to

determineReward CALL: get the number of tokens for the winning party of a challenge (Applicant or Challenger)

• suggestion: getChallengeWinnerReward

voterReward CALL: get the number of tokens for an individual Voter who voted in the majority_bloc of a challenge.

• suggestion: getVoterReward

claimReward SEND TX: transfer voterReward of msg.sender to msg.sender

• suggestion: claimVoterReward

tokenClaims CALL: see whether a Voter has tokens available to claim (i.e. did voter call claimReward yet)

• suggestions: doesVoterHaveRewardToClaim, doesVoterHaveTokenClaims

• Delayed exits
• Removing salts for claiming rewards
• Multi-functions

Throughout the contract system, calls are made to the token contract followed by state changes in the contracts themselves. Token contracts can perform reentrancy attacks to take advantage of this.

Recommendation

Move token contract calls after all state changes in each function. Alternatively, document that TCRs must use trusted token contracts that cannot call other contracts.

Add two events to Parameterizer.sol to track when a proposal challenge has succeeded or failed like Registry.sol

event _ChallengeSucceeded(uint challengeID)
event _ChallengeFailed(uint challengeID)

Add event to Parameterizer.sol to track when a proposal has been accepted similar to Registry.sol NewListingWhitelisted.

event _ProposalAccepted(bytes32 propID) - when proposal is accepted with no challenge
event _ProposalUnprocessed(bytes32 propID) - when proposal processBy date has passed

listingHash.applicationExpiry = block.timestamp + parameterizer.get("applyStageLen") can overflow.

Recommendation

Use SafeMath for arithmetic. If the token holders set applyStageLen too high, all new applications will revert until they pass a proposal to change the setting.

1. determineReward: gets the number of tokens for the winning party of a challenge (Applicant or Challenger, not Voter)

How about challengeWinnerReward? (same implementation as the now deprecated #10)

2. claimReward: only deals with Voter.

How about claimVoterReward? (same implementation as the now deprecated #10)

3. tokenClaims: sounds like it would return true if a voter has claim to some tokens. however the opposite is true. (clearly i was confused in #9)

How about voterClaimedReward?

Currently in the exit function the event triggered is _ListingRemoved. Since _ListingRemoved is also called when the domain is challenged out, creating either an additional event or replacing the current event int the exit would be a nice to have. Something like _ListingWithdrawn(_listingHash).

In development it will help differentiate whether the listing was voted out or withdrawn.

Add _RewardClaimed(msg.sender, _challengeID, reward) to Parameterizer.sol claimVoterReward function similiar to Registry.sol event https://github.com/skmgoldin/tcr/blob/master/contracts/Registry.sol#L281

Whitelisted applicants can exit the registry when they discover new information that threatens their deposit. New applicants cannot do so because exit calls require(isWhitelisted(_listingHash)). Even without an active challenge, a new applicant with new information cannot protect themselves from a challenge the way whitelisted applicants can.

Recommendation

If this is desired behavior, keep it. Just pointing out the discrepancy.

Invoking events without "emit" prefix is deprecated.

Registry.sol:186:13: Warning: Invoking events without "emit" prefix is deprecated.
            _TouchAndRemoved(_listingHash);
            ^----------------------------^
Parameterizer.sol:155:5: Warning: Invoking events without "emit" prefix is deprecated.
    _ReparameterizationProposal(_name, _value, propID, deposit, proposals[propID].appExpiry, msg.sender);

Easily fixed by adding the emit prefix to all events.

Use of the "var" keyword is deprecated.

Registry.sol:214:29: Warning: Use of the "var" keyword is deprecated.
        var (commitEndDate, revealEndDate,) = voting.pollMap(pollID);

Parameterizer.sol:189:25: Warning: Use of the "var" keyword is deprecated.
    var (commitEndDate, revealEndDate,) = voting.pollMap(pollID);

Not sure how to go about this one. Trying to store the result to a Poll struct gives a TypeError

Parameterizer.sol:189:5: TypeError: Too many components (5) in value for variable assignment (1 needed).
    PLCRVoting.Poll memory poll = voting.pollMap(pollID);
    ^--------------------------------------------------^

This declaration shadows an existing declaration.

Registry.sol:176:9: Warning: This declaration shadows an existing declaration.
        uint deposit = parameterizer.get("minDeposit");
        ^----------^
Registry.sol:115:5: The shadowed declaration is here:
    function deposit(bytes32 _listingHash, uint _amount) external {
    ^ (Relevant source part starts here and spans across multiple lines).

Can be fixed by changing uint deposit to uint minDeposit (seems more descriptive too)