Contracts first version #13

bingen · 2018-06-21T10:10:34Z

First version of contracts and tests.
Fixes #1

As there can only be one application per registry entry, and only one challenge per application/entry, we can use the same id for all of them.

Closes #3. Besides, add CI.

Closes #4. See: aragon/aragon-apps#380

Now Voting app can resolve challenge when vote is over without the need for another call from Curation app.

`manifest.json` and `arapp.json`

Move IStaking to @aragon/apps-staking

Staking app registers locks per user, not globally.

Challenge amount was always being used. Although it's usually going to be the same (min deposit), it was not correct.

izqui

WIP review

izqui · 2018-07-11T17:10:03Z

contracts/Curation.sol

+    mapping(bytes32 => Challenge) challenges;
+    mapping(address => mapping(uint256 => bool)) usedLocks;
+
+    event NewApplication(bytes32 entryId, address applicant);


We should index entryIds

Btw, there already was an issue for this, #12 , I just forgot ;-) I'm going to add it to the description so it's closed automatically.

izqui · 2018-07-11T17:19:10Z

contracts/Curation.sol

+        uint64 date;
+        bool resolved;
+        uint256 amount;
+        uint256 lockId;


We may run into issues with lockIds changing as when you remove a lock in the staking app the ids can change: https://github.com/aragon/aragon-apps/blob/b968e3ec9ab70cf6cfbfa5ebca8a58460669e7d1/future-apps/staking/contracts/Staking.sol#L173-L178

If a user has an unrelated lock with id = 0 and then creates a lock for creating a challenge with id = 1, if the lock 0 gets unlocked before the challenge is resolved, lock 1 will be lock 0 and unlocking will fail as there is no lock 1.

I think this issue may apply to PLCR voting as well. We may need to rethink how locks get stored in the Staking app.

Yes, since lock ids are exposed to the outside world, they should be immutable.

Created aragon/aragon-apps#406 for this

izqui · 2018-07-11T17:24:15Z

contracts/Curation.sol

+
+    function _checkLock(address user, uint256 lockId, uint64 date) internal returns (uint256) {
+        // check lockId was not used before
+        require(!usedLocks[user][lockId]);


Even though we can be fairly sure that the staking app is not malicious and it is not going to re-enter, we can have this check after the staking.getLock(msg.sender, lockId) to make sure the storage is correct after the external call.

Ideally we would do a STATICCALL but it is too cumbersome in this version of solidity.

izqui · 2018-07-11T17:28:54Z

contracts/Curation.sol

+        if (application.amount < minDeposit) {
+            registry.remove(entryId);
+            staking.unlock(application.applicant, application.lockId);
+            staking.unlock(msg.sender, lockId);


Why do we require a having a lock for this case if the challenge is immediately resolved?

Good point!

izqui · 2018-07-11T17:31:20Z

contracts/Curation.sol

+        // check application doesn't have an ongoing challenge
+        require(!challengeExists(entryId));
+        // check locked tokens
+        uint256 amount = _checkLock(msg.sender, lockId, getTimestamp().add(applyStageLen));


This amount is never checked (it is just saved in the Challenge struct) so I assume there is no minimum amount to stake in order to challenge. Couldn't this become a spam issue in which someone can just challenge all applications without having to stake and risk virtually any money?

It's checked inside _checkLock.

Totally missed it, 👍

izqui · 2018-07-11T17:37:04Z

contracts/Curation.sol

+        assembly {
+            mstore(add(executionScript, 0x20), spec)
+            mstore(add(targetBytes, 0x20), target)
+            mstore(add(executionScript, 0x24), mload(add(targetBytes, 0x2C)))


Why not store the target address directly here?

It would be better to just mload(target), shift the required bits and mstore that. Otherwise we are reading from unknown memory and that's a little scary.

I don't get why we are reading from unknown memory, but my assembly understanding is limited yet. Anyway, you are right, this way is too messy, I did it as you say, just shifting the address, way better. Thanks!

izqui · 2018-07-11T17:46:36Z

contracts/Curation.sol

+        }
+    }
+
+    function registerApplication(bytes32 entryId) isInitialized public {


If this function can only be used for unchallenged applications, I would rename to signal that: registerUnchallengedApplication(bytes32 entryId)

izqui · 2018-07-11T17:47:11Z

contracts/Curation.sol

+
+        // insert in Registry app
+        registry.add(application.data);
+        application.registered = true;


Is there a way to remove an entry once registered and recover the deposit?

Good point, not right now. This possibility doesn't appear in any of the documents (white paper, requirements or specs) as far as I remember, but I think it makes total sense. I'll add it, but also pinging @lkngtn so he is aware of this use case.

izqui · 2018-07-11T17:49:40Z

contracts/Curation.sol

+            // Unlock challenger tokens from Staking app
+            reward = challenge.amount.mul(dispensationPct) / PCT_BASE;
+            // Redistribute tokens
+            staking.unlockAndMoveTokens(challenge.challenger, challenge.lockId, application.applicant, reward);


Also an issue with staking, but it would be more descriptive if the function signature referenced the fact that the unlock is partial: staking.partialUnlockAndMove(...)

izqui · 2018-07-11T17:57:24Z

contracts/Curation.sol

+        // amount * (voter / total) * (1 - dispensationPct)
+        uint256 reward = amount.mul(voterWinningStake).mul(PCT_BASE.sub(dispensationPct)) / (totalWinningStake * PCT_BASE);
+        // Redistribute tokens
+        staking.unlockAndMoveTokens(loser, loserLockId, msg.sender, reward);


I think it would be cleaner if the curation app just took all the tokens from the challenger or applicant onresolveChallenge(...). This way we don't need to modify the lock every time, being more efficient.

Also in terms of the user that lost some tokens, I think it is better to remove them from their account ASAP as there is no way they are going to recover them.

Completely agreed. Plus:

It would solve the TODO 3 lines below about that truncating issue.

It would follow the same logic as our PLCR

bingen · 2018-07-16T15:25:06Z

Fixes #12 (per review).

bingen · 2018-07-16T15:28:14Z

Fixes #4
Closes #5

Fixes #12 too.

Closes #6.

Registry app must now be initialized.

Locks now have start date too.

izqui · 2018-08-01T16:46:48Z

contracts/Curation.sol

+     * @param _voting New Voting app
+     */
+    function setVotingApp(IVoting _voting) authP(CHANGE_VOTING_APP_ROLE, arr(voting, _voting)) public {
+        _setVotingApp(_voting);


This is quite dangerous if there are any challenges open, as they don't have a reference to the voting app, but rely on the global voting instance. I would remove the ability to hot swap the voting app after initialization.

Good catch!!

izqui · 2018-08-02T10:52:00Z

contracts/Curation.sol

+            mstore(add(executionScript, 0x40), entryId)
+        }
+
+        uint256 voteId = voting.newVote(executionScript, "");


Note: currently PLCR is not executing scripts https://github.com/aragonlabs/plcr/pull/1/files#r207182982

izqui · 2018-08-02T10:56:08Z

contracts/Curation.sol

+        }
+
+        // check locked tokens
+        uint256 amount = _checkLock(msg.sender, lockId, getTimestamp(), getTimestamp().add(applyStageLen));


The end date should be when the voting ends, but since we don't have any guarantee that resolveChallenge(...) will be called at that point, the lock shouldn't have an end date to avoid someone pulling the tokens deposited for the challenge before it can be resolved.

izqui · 2018-08-02T11:04:07Z

contracts/Curation.sol

+
+            // Remove from Registry app
+            application.registered = false;
+            registry.remove(entryId);


We should have special logic for whether the application was already in the registry or the challenge is done during the applyStageLen period. This isn't causing a crash because the registry doesn't care whether an entry was present in the registry before attempting to delete it: https://github.com/aragonlabs/registry/pull/3/files#r207185327

Yes, actually I think that's the reason I was not doing those checks on Registry app: it was making things easier here, as the operation would become "idempotent", i.e., it doesn't matter what's the state, the result will always be the desired (in this case, having the entry out of the registry).

izqui · 2018-08-02T11:07:42Z

contracts/Curation.sol

+        vote.votersRewardPool = amount - reward;
+
+        // unlock tokens from Staking app
+        staking.unlock(application.applicant, application.lockId);


If the application enters the registry we shouldn't remove its lock

izqui · 2018-08-02T11:13:39Z

contracts/Curation.sol

+
+        // Remove challenge, and application if needed
+        if (vote.result == true) {
+            delete(applications[entryId]);


Removing both application and challenge from storage once resolved is going to make it real hard to get historical data from the app. This would be a major change to how data is stored here, but let's think about it.

izqui · 2018-08-02T11:22:15Z

contracts/Curation.sol

+        // rewardPool * (voter / total)
+        uint256 reward = vote.votersRewardPool.mul(voterWinningStake) / vote.totalWinningStake;
+        // Redistribute tokens
+        staking.moveTokens(address(this), msg.sender, reward);


Should emit an event as PLCR does when tokens are claimed

izqui · 2018-08-02T11:24:05Z

contracts/Curation.sol

+            if (!application.registered) {
+                application.registered = true;
+                // insert in Registry app
+                registry.add(application.data);


Every time an application is registered or removed, the curation app should emit an event. With the current state in which we remove application and challenge data right away, it would be very hard for observers of the contract what is going on

izqui · 2018-08-02T11:25:54Z

contracts/Curation.sol

+    }
+
+    function _setMinDeposit(uint256 _minDeposit) internal {
+        minDeposit = _minDeposit;


Please log when any of the params change.

izqui · 2018-08-02T11:29:17Z

contracts/Curation.sol

+import "./interfaces/IVoting.sol";
+
+
+contract Curation is AragonApp {


We should consider all the cases in which locks are created to the curation app that may result in tokens getting locked forever, for example, locking tokens to create an application or challenge and being frontrun in the action. We need a mechanism to unlock when the locks haven't been used.

Comment on the topic for PLCR: https://github.com/aragonlabs/plcr/pull/1/files#r206916590

Yes, another related example: #16
Right now nobody is winning the Vote, so tokens are being locked.

bingen added 10 commits June 14, 2018 18:34

First version of contracts and tests

a778226

Unify ids

8458619

As there can only be one application per registry entry, and only one challenge per application/entry, we can use the same id for all of them.

Fix coverage

0e4ff98

Increase coverage to 100%

cb9dff7

Closes #3. Besides, add CI.

Use partial unlock before moving tokens

7114fe3

Closes #4. See: aragon/aragon-apps#380

Fix lint

a00e606

Add EVMScripts for resolving challenge

fe07f6d

Now Voting app can resolve challenge when vote is over without the need for another call from Curation app.

Skip interfaces in coverage

0973f1a

Add config files

af54e75

`manifest.json` and `arapp.json`

Move reward condition assignments close to checks

cff565a

bingen added sprint: 0.5.2 component: contract new-app labels Jun 21, 2018

bingen requested review from izqui and sohkai June 21, 2018 10:10

bingen mentioned this pull request Jun 21, 2018

Contracts #2

Closed

3 tasks

bingen added 2 commits June 21, 2018 12:34

Remove mock function from Voting interface

8e96232

Move uint64 cast to getTimestamp

8bf6651

bingen force-pushed the contracts_first_version branch from 0e28030 to 8bf6651 Compare June 21, 2018 10:34

bingen added 4 commits June 21, 2018 15:13

Change parameters order in Staking unlockAndMoveTokens function

e32a4a9

Fix test assert descriptions

b0563d7

Unify interfaces

7ea2204

Move IStaking to @aragon/apps-staking

Fix wrong used locks management

aa887f3

Staking app registers locks per user, not globally.

bingen self-assigned this Jul 9, 2018

Fix bug computing reward

0cc4697

Challenge amount was always being used. Although it's usually going to be the same (min deposit), it was not correct.

izqui reviewed Jul 11, 2018

View reviewed changes

bingen mentioned this pull request Jul 17, 2018

Staking: make lock ids innmutable aragon/aragon-apps#406

Closed

bingen added 2 commits July 17, 2018 23:23

Address PR #13 comments

7d9be49

Fixes #12 too.

Remove external interfaces

6285b4e

bingen force-pushed the contracts_first_version branch from 053ee46 to 6285b4e Compare July 17, 2018 21:24

This was referenced Jul 20, 2018

Fix lint #9

Closed

Use partial unlock before moving tokens #8

Closed

Add EVMScripts for resolving challenge #10

Closed

bingen added 3 commits July 24, 2018 18:55

Add function doc headers

3e0a007

Closes #6.

Adapt to last changes in Registry app

9e179dc

Registry app must now be initialized.

Adapt to last changes in Staking app

db8d13f

Locks now have start date too.

bingen force-pushed the contracts_first_version branch from 7436fbb to db8d13f Compare July 24, 2018 16:55

izqui requested changes Aug 2, 2018

View reviewed changes

bingen added this to the sprint: 1.2 milestone Sep 10, 2018

sohkai removed epic: tcr labels Dec 10, 2018

sohkai removed this from the sprint: 1.2 milestone Jan 2, 2019

		import "./interfaces/IVoting.sol";


		contract Curation is AragonApp {

Contracts first version #13

Are you sure you want to change the base?

Contracts first version #13

Conversation

bingen commented Jun 21, 2018 • edited Loading

izqui 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

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

bingen Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

bingen commented Jul 16, 2018

bingen commented Jul 16, 2018

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

bingen Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

bingen commented Jun 21, 2018 •

edited

Loading

bingen Jul 12, 2018 •

edited

Loading

bingen Aug 3, 2018 •

edited

Loading