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

Update contracts and dependencies to use new Templates #69

Open
wants to merge 4 commits into
base: master
from

Conversation

@0xGabi
Copy link
Member

commented Jul 30, 2019

TODO:

  • Add node script to setup new templates

  • Update tests

  • Publish @aragon/templates-shared

  • Update aragonCLI to deploy Templates with new script

@0xGabi 0xGabi added the enhancement label Jul 30, 2019
@0xGabi 0xGabi requested review from sohkai and facuspagnuolo Jul 30, 2019
@0xGabi 0xGabi added this to In progress in Aragon Mesh Team via automation Jul 30, 2019
// Create DAO and install apps
(Kernel dao, ACL acl) = _createDAO();
Vault agentOrVault = _useAgentAsVault ? _installDefaultAgentApp(dao) : _installVaultApp(dao);
Finance finance = _installFinanceApp(dao, agentOrVault, _financePeriod == 0 ? DEFAULT_FINANCE_PERIOD : _financePeriod);

This comment has been minimized.

Copy link
@sohkai

sohkai Jul 31, 2019

Member

I wouldn’t bother with installing the Vault and Finance apps; generally they’re not useful for people (especially locally where there are no tokens deployed).

@sohkai

This comment has been minimized.

Copy link
Member

commented Jul 31, 2019

I’ll publish the BaseTemplate later tonight!

Soon :)

Copy link
Member

left a comment

Looking really good! Left some comments to improve the code a bit :)

string constant private ERROR_MISSING_TOKEN_CACHE = "COMPANY_MISSING_TOKEN_CACHE";
string constant private ERROR_EMPTY_HOLDERS = "COMPANY_EMPTY_HOLDERS";
string constant private ERROR_BAD_HOLDERS_STAKES_LEN = "COMPANY_BAD_HOLDERS_STAKES_LEN";
string constant private ERROR_BAD_VOTE_SETTINGS = "COMPANY_BAD_VOTE_SETTINGS";

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Jul 31, 2019

Member

We can use a more suitable keyword here for the error messages instead of COMPANY, don't know if this template has a proper name... otherwise, I'd go with simply TEMPLATE

fac = _fac;
}
}
address constant ANY_ENTITY = address(-1);

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Jul 31, 2019

Member

If you're not calling this from outside, please set it to private


// Install placeholder-app-name
//--------------------------------------------------------------//
bytes32 _appId = keccak256(abi.encodePacked(apmNamehash("open"), keccak256("placeholder-app-name")));

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Jul 31, 2019

Member

can we move this to a constant like we are doing for the rest of the app ids?

This comment has been minimized.

Copy link
@0xGabi

0xGabi Aug 1, 2019

Author Member

I don't think so cause it depends of the user's app name. Maybe explain in a comment that is a good practice to save gas?

bytes32 _appId = keccak256(abi.encodePacked(apmNamehash("open"), keccak256("placeholder-app-name")));
CounterApp app = _installApp(_dao, _appId, new bytes(0), false);
app.initialize();
//--------------------------------------------------------------//

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Jul 31, 2019

Member

you can move all the counter app installation to an internal method called _installCounterApp like we're doing with the rest of the apps to be more consistent and for clarity

// Setup placeholder-app-name permissions
//--------------------------------------------------------------//
_createCustomAppPermissions(_acl, _app, _voting);
//--------------------------------------------------------------//

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Jul 31, 2019

Member

I don't think it's necessary to have this line between comments :)

This comment has been minimized.

Copy link
@0xGabi

0xGabi Aug 1, 2019

Author Member

Cool, I thought it might help users not much proficient with solidity yet to guide them what should they edit. But most likely I underestimating users here 😅

contracts/Template.sol Outdated Show resolved Hide resolved
contracts/Template.sol Outdated Show resolved Hide resolved
contracts/Template.sol Outdated Show resolved Hide resolved
contracts/Template.sol Outdated Show resolved Hide resolved
contracts/Template.sol Outdated Show resolved Hide resolved
0xGabi and others added 2 commits Sep 13, 2019
Thanks for reviewing Mathew 🙏

Co-Authored-By: Mathew Cormier <mathew.corm@gmail.com>
acl.grantPermission(root, acl, acl.CREATE_PERMISSIONS_ROLE());
acl.revokePermission(this, acl, acl.CREATE_PERMISSIONS_ROLE());
acl.setPermissionManager(root, acl, acl.CREATE_PERMISSIONS_ROLE());
function _cacheToken(

This comment has been minimized.

Copy link
@sohkai

sohkai Oct 9, 2019

Member

We now have the TokenCache utility available, so we can remove these parts :).

TokenManager tokenManager = _installTokenManagerApp(dao, token, TOKEN_TRANSFERABLE, TOKEN_MAX_PER_ACCOUNT);
Voting voting = _installVotingApp(dao, token, _votingSettings[0], _votingSettings[1], _votingSettings[2]);
// Install app
CounterApp app = _installCounterApp(dao);

This comment has been minimized.

Copy link
@sohkai

sohkai Oct 9, 2019

Member

I would encapsulate all the "custom app" installation and permissions into one or two functions, called after we initialize the "base" apps, to make it super easy for people to extend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Aragon Mesh Team
  
In progress
4 participants
You can’t perform that action at this time.