Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Template enhacements #127

Merged
merged 5 commits into from Aug 8, 2019

Conversation

@facuspagnuolo
Copy link
Member

commented Aug 3, 2019

MERGE AFTER #124

This PR provides some minor enhancements:

  • Simplify voting-related helpers
  • Simplify token-manager-related helpers
  • Provide TokenCache reusable contract
  • Automate initialize selectors for apps installations

@facuspagnuolo facuspagnuolo requested review from ajsantander and sohkai Aug 3, 2019

@facuspagnuolo facuspagnuolo self-assigned this Aug 3, 2019

@sohkai

sohkai approved these changes Aug 7, 2019

Copy link
Member

left a comment

Nice trick with the address(0).selector 😉

The one hesitation I have is if the additional "syntax sugar" methods, like _installVotingApp(Kernel, MiniMeToken, uint64[3]), are really worth their cost in contract size.

@@ -152,21 +150,26 @@ contract BaseTemplate is APMNamehash, IsContract {

/* VOTING */

function _installVotingApp(Kernel _dao, MiniMeToken _token, uint64[3] _votingSettings) internal returns (Voting) {

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 7, 2019

Member
Suggested change
function _installVotingApp(Kernel _dao, MiniMeToken _token, uint64[3] _votingSettings) internal returns (Voting) {
function _installVotingApp(Kernel _dao, MiniMeToken _token, uint64[3] memory _votingSettings) internal returns (Voting) {

Just to be a bit more explicit :)

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Aug 7, 2019

Author Member

I'd change all the arrays passed by argument in that case

shared/contracts/BaseTemplate.sol Outdated Show resolved Hide resolved
delete tokenCache[_owner];
return token;
}
}

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 7, 2019

Member

I'm really starting to think we could move these contracts to something like @aragon/templates-helpers or etc. so we can publish them just by themselves.

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Aug 7, 2019

Author Member

Why not publishing this repo then?

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 8, 2019

Member

Hmm yes, ok, maybe it would be better to publish templates-shared since it also contains useful scripts and etc.

@ajsantander
Copy link
Contributor

left a comment

Awesome improvements @facuspagnuolo!!
I left some general minor comments, except one related to the Survey app initialization that may be a bug.

@facuspagnuolo facuspagnuolo changed the base branch from add_payroll_app to master Aug 7, 2019

@facuspagnuolo facuspagnuolo force-pushed the template_enhacements branch from dffcda0 to dc9c8a2 Aug 7, 2019

@facuspagnuolo

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

@sohkai

The one hesitation I have is if the additional "syntax sugar" methods, like _installVotingApp(Kernel, MiniMeToken, uint64[3]), are really worth their cost in contract size.

Remember that this does not increase the bytecode length unless you use those helpers

@facuspagnuolo facuspagnuolo merged commit 297a950 into master Aug 8, 2019

4 checks passed

WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@delete-merged-branch delete-merged-branch bot deleted the template_enhacements branch Aug 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.