Decoupled projects: Contract changes #1912

topocount · 2020-01-23T12:48:34Z

Addresses #1875 and #1897

This PR adds functions to the projects contract for:

creating decoupled projects that can include an IPFS hash
creating and modifiying issues that don't yet contain bounties

This PR also contains

gas optimizations to allow these issues to fit in the contract
the removal of the standard bounties bytecode check to make room for these features, since isContract should be sufficient

Note

merge test(bounties): remove unnecessary wallet dependency #1908 before this PR

Quazia

Looks good, in integration we'll need to check for all instances of IssueUpdated and the parsing of return data from getIssue I'm not seeing anything else that should need changing.

apps/projects/test/Projects.test.js

apps/projects/contracts/Projects.sol

rkzel · 2020-01-28T12:16:44Z

Specification suggests that there should be a fuction to edit project as well as add/remove. Editing will involve only replacing ipfs hash saved in_projectData, so all we need is a simple function like this:

    function updateRepo(bytes32 _repoId, string _repoData) external auth(....?)
     {
        require(isRepoAdded(_repoId), ERROR_REPO_MISSING);
        uint rowToUpdate = repos[_repoId].index;
        repoIndex[repoIndexLength] = _repoId;
        repos[rowToUpdate].repoData = _repoData;
        emit Repo....?
    }

Probably a new right and event to emit need to be added as well - what do you think, @topocount ? Alternatively if it's too complicated for current timeline, maybe we leave it for now and not allow editing projects at all?

- add decentralized project entries - add and update issues directly without bounties - enable multi-fulfillment bounties BREAKING CHANGE: Projects contract updates

- `fulfillBounty` radspec message now shows up in external tx panel - remove extraneous logging from deploy script - remove unneeded functions from Bounties interface

Projects: improve querying behavior

ottodevs

Changed base branch to merge, to properly see the exact changes going on here

Most of the contract and tests look fine for me, and they are passing. I am a bit concerned about opening potential security vectors, as we are not auditing this new code on this review.

Just some minor details to comment about:

It would be preferable to properly document each function that was added or changed
I would just take advantage of this work to keep the contract clean and audit-ready by properly documenting this new code.

I would also bet to improve the tests suite and make it more readable/ less error prone by making use of constants and avoiding magic numbers and text everywhere.

ottodevs · 2020-02-05T19:08:31Z

apps/projects/contracts/Projects.sol

@@ -319,12 +297,16 @@ contract Projects is AragonApp, DepositableStorage {
    /**
     * @notice Get an entry from the registry.
     * @param _repoId The id of the repo in the projects registry
-     * @return index the repo registry index
+     * @return index the repo registry index, number of open bounties, decoupled bool, repo metadata


It is preferable to document @return params into separate lines each one

ottodevs · 2020-02-05T19:10:11Z

apps/projects/contracts/Projects.sol

@@ -357,20 +339,25 @@ contract Projects is AragonApp, DepositableStorage {
 // Repository functions
 ///////////////////////
    /**
-     * @notice Add repository
+     * @notice Add new Project: `_projectData`.
     * @param _repoId The id of the repo in the projects registry


We should document the rest of params as well

ottodevs · 2020-02-05T19:14:51Z

apps/projects/contracts/Projects.sol

+        string _data
+    ) external
+    {
+        revert();


If we just want to display the radspec, why revert? Also this should be a pure function

Revert will cause non 0 exit status and that maybe non gracefully handled by the frontend

This function should never execute. It's just a workaround for getting the radspec showing up in the frontend. BY forcing it to revert, it effectively doesn't exist and is an invalid call on the contract

ottodevs · 2020-02-05T19:16:42Z

apps/projects/contracts/Projects.sol

+        repos[_repoId].repoData = _repoData;
+        if (!isRepoAdded(_repoId)) {
+            repoIndex[repoIndexLength] = _repoId;
+            repos[_repoId].index = repoIndexLength++;


Are we accounting on a potential overflow here?

ottodevs · 2020-02-05T19:17:35Z

apps/projects/contracts/Projects.sol

+    ) internal
+    {
+        // ensure the transvalue passed equals transaction value
+        //checkTransValueEqualsMessageValue(msg.value, _bountySizes,_tokenBounties);


Is this comment relevant anymore or just dead code? We should remove to avoid confusion

ottodevs · 2020-02-05T19:23:15Z

apps/projects/contracts/test/BountiesEvents.sol

+    event ContributionsRefunded(uint _bountyId, address _issuer, uint [] _contributionIds);
+    event BountyDrained(uint _bountyId, address _issuer, uint [] _amounts);
+    event ActionPerformed(uint _bountyId, address _fulfiller, string _data);
+    event BountyFulfilled(uint _bountyId, uint _fulfillmentId, address  [] _fulfillers, string _data, address _submitter);


Some spacing between array brackets and types seem wrong with inconsistently 1 or 2 spaces, maybe we should try to be consistent about it, I vote for no spacing between type and brackets

Suggested change

event BountyFulfilled(uint _bountyId, uint _fulfillmentId, address [] _fulfillers, string _data, address _submitter);

event BountyFulfilled(uint _bountyId, uint _fulfillmentId, address[] _fulfillers, string _data, address _submitter);

Some for the rest of lines on this block

ottodevs · 2020-02-05T19:24:39Z

apps/projects/test/Projects.test.js

@@ -1,4 +1,4 @@
-/* global artifacts, assert, before, beforeEach, contract, context, expect, it, web3 */
+/* global artifacts, assert, before, beforeEach, contract, context, expect, it, web3, xit */


xit should actually not need to be defined here, as tests should only be skipped during deployment

ottodevs · 2020-02-05T19:26:56Z

apps/projects/test/Projects.test.js

+            1,
+            1,
+            true,
+            'QmbUSy8HCn8J4TMDRRdxCbK2uCCtkQyZtY6XYv3y7kLgDl',


I would move most of these arbitrary magic values to constante to have a more manageable/less error prone test suite, but that's only a personal preference

ottodevs · 2020-02-05T19:28:34Z

apps/projects/test/Projects.test.js

+              new web3.BigNumber(56),
+              new web3.BigNumber(15),
+              new web3.BigNumber(0),
+              '0xffffffffffffffffffffffffffffffffffffffff',


We should use ANY_ADDR (line 16) constant in places like here, otherwise, we define it at all?

ottodevs · 2020-02-05T19:30:18Z

apps/projects/test/Projects.test.js

+    context('decoupled project', () => {
+      let repoId
+      beforeEach(async () => {
+        await app.setRepo('1', true, 'abc')


We should use real repo ids as in other test cases to avoid false negatives when repoId length does not find in the contract field

Projects will be using integers for decoupled repoIds going forward. They'll still get hashed by the frontend before passing them in, but that's really just a messier magic number in the context of tests

topocount · 2020-02-05T22:04:31Z

Changed base branch to merge, to properly see the exact changes going on here

We're working out of the https://github.com/AutarkLabs/open-enterprise/tree/decentralized-project-integration banch, and this branch has already been merged in there. I'll make your requested changes in there and redirect https://github.com/AutarkLabs/open-enterprise/tree/issue-querying to that branch as well.

topocount · 2020-02-17T20:02:58Z

This branch has been merged into PR #1941. The review above has been addressed there

topocount requested review from a team as code owners January 23, 2020 12:48

ghost requested review from ottodevs, Quazia and PeterMPhillips and removed request for a team January 23, 2020 12:48

topocount changed the title ~~Decoupled projects~~ Decoupled projects: Contract changes Jan 23, 2020

javieralaves added app: projects contracts Involves changes to solidity files labels Jan 23, 2020

Quazia previously approved these changes Jan 23, 2020

View reviewed changes

apps/projects/test/Projects.test.js Show resolved Hide resolved

apps/projects/contracts/Projects.sol Outdated Show resolved Hide resolved

topocount dismissed Quazia’s stale review via e16856c January 24, 2020 04:24

topocount force-pushed the decoupled-projects branch 5 times, most recently from 14dd761 to 55d1768 Compare January 24, 2020 19:32

stellarmagnet added this to the Offsite Hackathon milestone Jan 28, 2020

topocount force-pushed the decoupled-projects branch from 98c1d54 to 8ecd8c9 Compare January 28, 2020 22:50

topocount added 2 commits January 29, 2020 13:12

feat(projects): add decoupled project functionality

74ce086

- add decentralized project entries - add and update issues directly without bounties - enable multi-fulfillment bounties BREAKING CHANGE: Projects contract updates

fix(projects): enable radspec for external function

b48653b

- `fulfillBounty` radspec message now shows up in external tx panel - remove extraneous logging from deploy script - remove unneeded functions from Bounties interface

topocount force-pushed the decoupled-projects branch from 8ecd8c9 to b48653b Compare January 29, 2020 18:13

Merge pull request #1923 from AutarkLabs/issue-querying

edcf872

Projects: improve querying behavior

ottodevs changed the base branch from dev to issue-querying February 5, 2020 19:04

ottodevs requested changes Feb 5, 2020

View reviewed changes

ottodevs assigned topocount Feb 5, 2020

topocount closed this Feb 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decoupled projects: Contract changes #1912

Decoupled projects: Contract changes #1912

topocount commented Jan 23, 2020 •

edited

Loading

Quazia left a comment

rkzel commented Jan 28, 2020 •

edited

Loading

ottodevs left a comment

ottodevs Feb 5, 2020

ottodevs Feb 5, 2020

ottodevs Feb 5, 2020

ottodevs Feb 5, 2020

topocount Feb 17, 2020 •

edited

Loading

ottodevs Feb 5, 2020

ottodevs Feb 5, 2020

ottodevs Feb 5, 2020

ottodevs Feb 5, 2020

ottodevs Feb 5, 2020

ottodevs Feb 5, 2020

ottodevs Feb 5, 2020

topocount Feb 17, 2020 •

edited

Loading

topocount commented Feb 5, 2020

topocount commented Feb 17, 2020

	event BountyFulfilled(uint _bountyId, uint _fulfillmentId, address [] _fulfillers, string _data, address _submitter);
	event BountyFulfilled(uint _bountyId, uint _fulfillmentId, address[] _fulfillers, string _data, address _submitter);

		@@ -1,4 +1,4 @@
		/* global artifacts, assert, before, beforeEach, contract, context, expect, it, web3 */
		/* global artifacts, assert, before, beforeEach, contract, context, expect, it, web3, xit */

Decoupled projects: Contract changes #1912

Decoupled projects: Contract changes #1912

Conversation

topocount commented Jan 23, 2020 • edited Loading

Quazia left a comment

Choose a reason for hiding this comment

rkzel commented Jan 28, 2020 • edited Loading

ottodevs 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

topocount Feb 17, 2020 • 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

topocount Feb 17, 2020 • edited Loading

Choose a reason for hiding this comment

topocount commented Feb 5, 2020

topocount commented Feb 17, 2020

topocount commented Jan 23, 2020 •

edited

Loading

rkzel commented Jan 28, 2020 •

edited

Loading

topocount Feb 17, 2020 •

edited

Loading

topocount Feb 17, 2020 •

edited

Loading