Projects v2 contracts #1002

topocount · 2019-06-19T05:11:10Z

Remaining Support Actions

The initial contract work is done, but supporting files still need some work and the contract likely needs some polish in order to conform to conventions we've set forth in Rewards and Address Book contracts:

Get converage working again Testing spans two repo's due to solidity pragma incompatibilities. we need to get Truffle executing in both repos within the context of coverage
Obtain 100% Coverage Right now a few cases where require functions fail are missing coverage
Update contracts and tests to conform to conventions set forth in Address Book and Rewards

- remove default token from initialize call (defaults to address(0)) - remove references to github

- add metaTX fulfillment call - remove unsupported getters - remove support for adding multiple Issuers

2019-06-21: initial submission, corrected linter issues, corrected testing issues

- convert _removeBounty to work with Standard Bounties V2 - add drainBounty and changeDeadline interfaces

ottodevs

Just some questions to value if any of these should be needed to address into the audit cleanup

ottodevs · 2019-07-08T23:37:14Z

apps/address-book/contracts/test/TestImports.sol

@@ -8,6 +8,7 @@ import "@tps/test-helpers/contracts/factory/EVMScriptRegistryFactory.sol";
 import "@tps/test-helpers/contracts/lib/misc/Migrations.sol";
 import "@aragon/apps-shared-minime/contracts/MiniMeToken.sol";
 import "@tps/test-helpers/contracts/lib/bounties/StandardBounties.sol";
+import "@tps/test-helpers/contracts/lib/bounties/BountiesEvents.sol";


I think we should not actually need to import this if we don't intend to use BountiesEvents in AddressBook tests, do you have an special reason for it?

Good catch. I've reorganized Spoof.sol and TestImports.sol imports to better reflect their purposes in the address book app

ottodevs · 2019-07-08T23:38:10Z

apps/allocations/test/Spoof.sol

@@ -5,6 +5,7 @@ import "@tps/test-helpers/contracts/factory/DAOFactory.sol";
 import "@tps/test-helpers/contracts/acl/ACL.sol";
 import "@aragon/apps-shared-minime/contracts/MiniMeToken.sol";
 import "@tps/test-helpers/contracts/lib/bounties/StandardBounties.sol";
+import "@tps/test-helpers/contracts/lib/bounties/BountiesEvents.sol";


I think we should not actually need to import this if we don't intend to use BountiesEvents in Allocations tests, do you have an special reason for it?

We need to import it here or Truffle throws this error (as outlined by Jorge's comments in Spoof.sol): Error: Could not find artifacts for ./lib/bounties/BountiesEvents from any sources

ottodevs · 2019-07-08T23:38:37Z

apps/dot-voting/test/Spoof.sol

@@ -1,6 +1,7 @@
 pragma solidity ^0.4.24;

 import "@tps/test-helpers/contracts/lib/bounties/StandardBounties.sol";
+import "@tps/test-helpers/contracts/lib/bounties/BountiesEvents.sol";


apps/planning-suite-kit/contracts/PlanningKit.sol

apps/projects/contracts/Projects.sol

ottodevs · 2019-07-08T23:58:55Z

apps/projects/contracts/Projects.sol

@@ -293,7 +344,7 @@ contract Projects is IsContract, AragonApp {
 ///////////////////////
    /**
     * @notice Add repository to the Projects app
-     * @param _repoId Github id of the repo to add
+     * @param _repoId id of the repo to be aadded


Really small miss click typo 😇

ottodevs · 2019-07-09T00:01:18Z

apps/projects/contracts/Projects.sol

@@ -370,7 +426,8 @@ contract Projects is IsContract, AragonApp {
        bool _approved
    ) external auth(REVIEW_APPLICATION_ROLE)
    {
-        GithubIssue storage issue = repos[_repoId].issues[_issueNumber];
+        Issue storage issue = repos[_repoId].issues[_issueNumber];
+        require(issue.assignee != 0xffffffffffffffffffffffffffffffffffffffff, "ISSUE_OPEN");


Very opinionated comment, but the address seems ugly to me, maybe address(-1) could work?

ottodevs · 2019-07-09T00:04:00Z

apps/projects/contracts/Projects.sol

            emit SubmissionAccepted(_submissionNumber, _repoId, _issueNumber);
        } else {
-            submission.status = SubmissionStatus.Rejected;
+            bounties.performAction(


I will probably try to clean this duped block, it seems that it could be outside of the condition, I will check carefully anyway

I moved the bounties.performAction call outside of the if/else construct and just put it at the end

ottodevs · 2019-07-09T00:13:07Z

apps/projects/test/projects.test.js

@@ -136,7 +168,7 @@ contract('Projects App', accounts => {
    )

    // Deploy test Bounties contract
-    bounties = await StandardBounties.new(web3.toBigNumber(owner1))
+    bounties = { address: '0x72D1Ae1D6C8f3dd444b3D95bAd554Be483082e40'.toLowerCase() }


Just curious, what is the motivation of toLowerCase here?

the test for changing bounty settings fails:

1) Contract: Projects App post-initialization settings management can change Bounty Settings: StandardBounties Contract address incorrect + expected - actual -0x72d1ae1d6c8f3dd444b3d95bad554be483082e40 +0x72D1Ae1D6C8f3dd444b3D95bAd554Be483082e40

Ok I think I understand, to prevent different hashes for the same address with different casing, yeah, thinking about it I agree is a good decision, it seems less cumbersome than dealing with checksummed addresses 👍

ottodevs · 2019-07-11T18:28:15Z

apps/shared/test-helpers/artifacts.js

@@ -7,5 +7,6 @@ module.exports = {
  Kernel: artifacts.require('./kernel/Kernel'),
  MiniMeToken: artifacts.require('./lib/minime/MiniMeToken'),
  StandardBounties: artifacts.require('./lib/bounties/StandardBounties'),
+  BountiesEvents: artifacts.require('./lib/bounties/BountiesEvents'),


Here is the small change that enforces to import BountiesEvents on each Spoof.sol for every app 🎉

I am sorry you experienced errors locally, it seems this one is so confusing

I blame myself because I created this "artifact-requirer" helper, but it seems is not useful anymore, my recommendation would be to move StandardBounties related code to a projects subfolder or to the integrations one, as I think it is convenient for us to get rid of test-helpers folder, I tried to follow this approach with Rewards when moving the mining functions there

Is not a big deal, but seems it is related to #588 and it actually clears lot of intermediate dead code, I think the change worths be done

I already did the changes so I will commit those, if you don't mind @topocount I just tried to clarify about the previous related comments so we can save some time in the future with these pesky things

…untyContractValid

- convert repoIndex to a mapping - add requires to token approval calls

- add isInitialized to all functions without auth - convert all functions with small enough parameter sizes to "external" from "public" - reorder functions to be match visibility order style - add "_description" to updateBounty to make it more forwarder-friendly

topocount and others added 19 commits June 17, 2019 17:11

add new StandardBounties.sol to repo

38f9416

new bounties integration connected successfully

0ca6377

Projects compiling with description field

53339d3

add new issueAndContribute function

561187f

initial addBounties V2 implementation

f257063

update tests to work with sepearate Standard Bounties Repo

bbcebbf

remove activateBounty

a4dd8d9

Cleanup addressing #753, #806

f541ef8

- remove default token from initialize call (defaults to address(0)) - remove references to github

Add V2 interfaces and refactor applicable funcs

a068759

- add metaTX fulfillment call - remove unsupported getters - remove support for adding multiple Issuers

[#926] killBounty support

3d3b490

2019-06-21: initial submission, corrected linter issues, corrected testing issues

create addBountiesNoAssignment

a2dfe3c

Merge branch 'mainnet-audit' into projectsV2-contracts

9d2c456

Merge remote-tracking branch 'e18r/kill' into projectsV2-contracts

54e8b53

- convert _removeBounty to work with Standard Bounties V2 - add drainBounty and changeDeadline interfaces

update test script to use explicit truffle binary

5b0ea62

add comments and adjust params to conform to interface

3363829

tests passing for killing bounties and other pre-existing operations

fb30248

add test cases for new "require" statements

520592d

add performAction integration

c891e97

Projects contract V2 completely implemented

459a826

topocount marked this pull request as ready for review July 8, 2019 14:24

topocount assigned topocount and ottodevs and unassigned topocount Jul 8, 2019

topocount added 2 commits July 8, 2019 16:53

coverage works with new Standard Bounties Contract

61495eb

100 percent coverage for V2 Standard Bounties

6f897d8

ottodevs reviewed Jul 11, 2019

View reviewed changes

utilize extcodecopy to hash external contract

e44aba7

topocount force-pushed the projectsV2-contracts branch from 03a7e84 to b490ac8 Compare July 12, 2019 21:20

topocount force-pushed the projectsV2-contracts branch 2 times, most recently from faa15c8 to e44aba7 Compare July 12, 2019 23:47

sequentially test 4 segments of an updated bounties contract in _isBo…

c79afa7

…untyContractValid

topocount force-pushed the projectsV2-contracts branch from 21cbb80 to c79afa7 Compare July 14, 2019 05:00

topocount added 2 commits July 14, 2019 21:27

Resolve remaining medium-level audit findings

39379d2

- convert repoIndex to a mapping - add requires to token approval calls

Resolve low-risk items

ae351c5

- add isInitialized to all functions without auth - convert all functions with small enough parameter sizes to "external" from "public" - reorder functions to be match visibility order style - add "_description" to updateBounty to make it more forwarder-friendly

topocount closed this Jul 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Projects v2 contracts #1002

Projects v2 contracts #1002

topocount commented Jun 19, 2019 •

edited

ottodevs left a comment

ottodevs Jul 8, 2019

topocount Jul 11, 2019

ottodevs Jul 8, 2019

topocount Jul 11, 2019

ottodevs Jul 8, 2019

ottodevs Jul 8, 2019

ottodevs Jul 9, 2019

ottodevs Jul 9, 2019

topocount Jul 11, 2019

ottodevs Jul 9, 2019

topocount Jul 11, 2019

ottodevs Jul 11, 2019

ottodevs Jul 11, 2019

Projects v2 contracts #1002

Projects v2 contracts #1002

Conversation

topocount commented Jun 19, 2019 • edited

Remaining Support Actions

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

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

topocount commented Jun 19, 2019 •

edited