Allows to pass more parameters to templates #119

eternauta1337 · 2019-07-25T13:57:59Z

🏁 Ready for Review 🏁
Fix #111

Tasks (In reputation, membership, company and company-board templates):

facuspagnuolo

Looking good! Left one small comment 👏

templates/company-board/contracts/CompanyBoardTemplate.sol

…ion as a parameters

…tRequired as parameters

…nAcceptanceQuorum as parameters

facuspagnuolo

Looking good! I realized a simple optimization we can do for the company-board template that will improve gas costs. LMKYT, maybe it will also make sense to replicate it for the rest of the templates

templates/company-board/contracts/CompanyBoardTemplate.sol

templates/company/contracts/CompanyTemplate.sol

…d tests (Will not compile until rebased on #119)

…uolo

…uolo

facuspagnuolo

Looking good, left another comment on how we can optimize the cache for the company-board template... WDYT about replicating the voting settings thing for the rest of the templates?

facuspagnuolo · 2019-07-26T15:10:34Z

templates/company-board/contracts/CompanyBoardTemplate.sol

+        (TokenManager boardTokenManager, Voting boardVoting) = _setupBoard(_boardMembers, _boardVoteSettings[0], _boardVoteSettings[1], _boardVoteSettings[2]);
+        (TokenManager shareTokenManager, Voting shareVoting) = _setupShare(_shareHolders, _shareStakes, _shareVoteSettings[0], _shareVoteSettings[1], _shareVoteSettings[2]);
+        _setupPermissions(boardVoting, boardTokenManager, shareVoting, shareTokenManager, agent, finance);
+        _registerDAO(_id);


I'm sure we can optimize the caching here a little bit. Probably you can pop up the cache at the beginning and pass the dao/acl by parameter to the rest of the functions instead of loading the cache on each sub function

Yeah, I had tried this before but it "caused stack too deep" errors. I uploaded that version in case you want to see. I failed at the linked line: https://github.com/aragon/dao-templates/blob/parameterize_templates_simpler_cache/templates/company-board/contracts/CompanyBoardTemplate.sol#L65

Ofc that indexing variables at that level won't help, you should pass the array to the internal function and check it there

Yes, you're right! Still, I just tried changing that, and the stack too deep error persists. I've pushed it to that temporary branch nonetheless, so that we may use this code as reference for a future gas optimization.

eternauta1337 · 2019-07-26T15:52:33Z

Looking good, left another comment on how we can optimize the cache for the company-board template... WDYT about replicating the voting settings thing for the rest of the templates?

TBH, I think that using an array for all voting settings is less explicit in function interfaces that dedicated parameters per setting. I wouldn't use it unless it's necessary. Now, if we put consistency before all other things, yes, we'd have to do it, but it would make the other templates just a bit less clear.

* Implemented agent/vault selection in ReputationTemplate contract and tests * Refactored how ReputationTemplate tests handle agent/vault * Implemented agent/vault selection in MembershipTemplate contract and tests * Implemented agent/vault selection in CompanyTemplate contract and tests * Implemented agent/vault selection in CompanyBoardTemplate contract and tests (Will not compile until rebased on #119) * Updated READMEs for useAgentAsVault option * Minor change on reputation template tests * Fixed some linting errors in contracts

sohkai

A couple notes on small things, great job @ajsantander :).

sohkai · 2019-07-29T08:08:29Z

templates/company-board/contracts/CompanyBoardTemplate.sol


    bool constant private BOARD_TRANSFERABLE = false;
-    string constant private BOARD_TOKEN_NAME = "Board Token";
+    string constant private BOARD_TOKEN_NAME = "Board Token";	


Note that there's some extra whitespace lingering in most of the contract files.

(LMK if your editor doesn't have an easy way to remove them, I use this vim command to fix it automatically)

Oops! Thanks for catching that. I believe that @facuspagnuolo already fixed this in #123

I was planning on installing a linter to avoid this sort of stuff, but didn't find the time last week. Anyways, I installed that plugin for the mean time. Love it :D

sohkai · 2019-07-29T08:14:42Z

templates/company-board/test/company-board.js

+      const totalGas = prepareGas + setupGas;
+      assert.isAtMost(prepareGas, 5.0e6, 'prepare script should cost almost 5.0e6 gas')
+      assert.isAtMost(setupGas, 5.4e6, 'setup script should cost almost 5.4e6 gas')
+      assert.isAtMost(totalGas, 10.4e6, 'prepare + setup scripts should cost almost 10.4e6 gas');


Note on the trailing ; too, generally we don't use them anymore :)

👍
I'll also be setting up my linter for JS this week to comply with the team's JS coding style.

sohkai · 2019-07-29T08:19:29Z

templates/company/contracts/CompanyTemplate.sol

+        address[] _holders, 
+        uint256[] _stakes, 
+        string _tokenName, 
+        string _tokenSymbol,


Super small thing, but the parameter order may be a little bit more obvious if we had the arguments in the same order they would be used in, i.e. _tokenName, _tokenSymbol, _id, _holders`, ...

That way a frontend could conceivably do something like creationArgs = [].concat(tokenArgs, instanceArgs) and it'd just work (not that we would likely be this loose in our own frontend).

eternauta1337 added 4 commits July 25, 2019 07:23

ReputationTemplate accepts token name and symbol as parameters

c930c85

MembershipTemplate accepts token name and symbol as parameters

d4568b0

CompanyTemplate accepts token name and symbol as parameters

4f53b63

CompanyBoardTemplate accepts token name and symbol as parameters

2fe1f9f

eternauta1337 requested a review from facuspagnuolo July 25, 2019 13:58

facuspagnuolo reviewed Jul 25, 2019

View reviewed changes

templates/company-board/contracts/CompanyBoardTemplate.sol Outdated Show resolved Hide resolved

Removed parameterization of Board token's name and symbol

00bbef8

luisivan mentioned this pull request Jul 25, 2019

Create new templates contracts aragonone/product#138

Closed

19 tasks

eternauta1337 added 4 commits July 25, 2019 08:40

ReputationTemplate now accepts voteDuration as a parameter

7d88e5c

MembershipTemplate now accepts voteDuration as a parameter

6609fcd

CompanyTemplate now accepts voteDuration as a parameter

81b4611

CompanyBoardTemplate now accepts boardVoteDuration and shareVoteDurat…

bf975d9

…ion as a parameters

eternauta1337 changed the title ~~Allows to pass more parameters to templates~~ (WIP) Allows to pass more parameters to templates Jul 25, 2019

eternauta1337 added 17 commits July 25, 2019 10:41

ReputationTemplate now accepts supportRequired as a parameter

cdff867

MembershipTemplate now accepts supportRequired as a parameter

6e0ad2f

CompanyTemplate now accepts supportRequired as a parameter

95e63a1

CompanyBoardTemplate now accepts boardSupportRequired and shareSuppor…

c3098a7

…tRequired as parameters

Update to comply with linting rules in CompanyBoardTemplate

9f47438

ReputationTemplate now accepts minAcceptanceQuorum as a parameter

76d703d

MembershipTemplate now accepts minAcceptanceQuorum as a parameter

8ae4fc0

CompanyTemplate now accepts minAcceptanceQuorum as a parameter

814757c

CompanyBoardTemplate now accepts boardMinAcceptanceQuorum and shareMi…

5679000

…nAcceptanceQuorum as parameters

ReputationTemplate now accepts financePeriod as a parameter

63ae3d0

MembershipTemplate now accepts financePeriod as a parameter

4226c3b

CompanyTemplate now accepts financePeriod as a parameter

4767191

CompanyBoardTemplate now accepts financePeriod as a parameter

76b6aa0

ReputationTemplate defaults financePeriod to 30 days if 0 is passed

3e78f06

MembershipTemplate defaults financePeriod to 30 days if 0 is passed

552edca

CompanyTemplate defaults financePeriod to 30 days if 0 is passed

2727f0e

CompanyBoardTemplate defaults financePeriod to 30 days if 0 is passed

594cf23

Updated readme files with new available parameters

efc0c04

eternauta1337 changed the title ~~(WIP) Allows to pass more parameters to templates~~ Allows to pass more parameters to templates Jul 26, 2019

facuspagnuolo approved these changes Jul 26, 2019

View reviewed changes

templates/company-board/contracts/CompanyBoardTemplate.sol Outdated Show resolved Hide resolved

templates/company/contracts/CompanyTemplate.sol Outdated Show resolved Hide resolved

facuspagnuolo assigned eternauta1337 Jul 26, 2019

facuspagnuolo added template: company template: company-board template: membership template: reputation enhancement New feature or request labels Jul 26, 2019

eternauta1337 added a commit that referenced this pull request Jul 26, 2019

Implemented agent/vault selection in CompanyBoardTemplate contract an…

6b0c0e0

…d tests (Will not compile until rebased on #119)

eternauta1337 added 3 commits July 26, 2019 08:45

Gas optimization in CompanyBoardTemplate as recommended by @facuspagn…

4ed802b

…uolo

Updated READMEs

329a537

Made all newTokenAndInstance() functions external

2c5c547

eternauta1337 mentioned this pull request Jul 26, 2019

Toggles between Agent or Vault app #121

Merged

4 tasks

facuspagnuolo reviewed Jul 26, 2019

View reviewed changes

eternauta1337 merged commit 495af37 into master Jul 26, 2019

delete-merged-branch bot deleted the parameterize_templates branch July 26, 2019 16:59

sohkai reviewed Jul 29, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allows to pass more parameters to templates #119

Allows to pass more parameters to templates #119

eternauta1337 commented Jul 25, 2019 •

edited

Loading

facuspagnuolo left a comment

facuspagnuolo left a comment

facuspagnuolo left a comment

facuspagnuolo Jul 26, 2019

eternauta1337 Jul 26, 2019

facuspagnuolo Jul 26, 2019

eternauta1337 Jul 26, 2019

eternauta1337 commented Jul 26, 2019

sohkai left a comment

sohkai Jul 29, 2019

eternauta1337 Jul 29, 2019

sohkai Jul 29, 2019

eternauta1337 Jul 29, 2019

sohkai Jul 29, 2019

Allows to pass more parameters to templates #119

Allows to pass more parameters to templates #119

Conversation

eternauta1337 commented Jul 25, 2019 • edited Loading

Tasks (In reputation, membership, company and company-board templates):

facuspagnuolo left a comment

Choose a reason for hiding this comment

facuspagnuolo left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eternauta1337 commented Jul 26, 2019

sohkai 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

eternauta1337 commented Jul 25, 2019 •

edited

Loading