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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add payroll app to all templates #124

Merged
merged 30 commits into from Aug 7, 2019

Conversation

@ajsantander
Copy link
Contributor

commented Jul 29, 2019

馃毀 WIP 馃毀

Tasks

  • Add Payroll app functionality in base template
  • Add Payroll app in company template (review before applying to all others)
  • Company board template
  • Membership template
  • Reputation template

@ajsantander ajsantander requested a review from facuspagnuolo Jul 29, 2019

@luisivan luisivan referenced this pull request Jul 29, 2019
17 of 19 tasks complete
shared/helpers/abi.js Outdated Show resolved Hide resolved
shared/helpers/abi.js Outdated Show resolved Hide resolved
shared/scripts/test-ganache.sh Outdated Show resolved Hide resolved
@facuspagnuolo

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Left a few comments, would you mind updating this branch with master?

@facuspagnuolo
Copy link
Member

left a comment

Looking good, we can work out some improvements for the tests later

@sohkai
Copy link
Member

left a comment

Just looked at the company template's changes, since the other template changes are quite similar.

A couple suggestions :)

shared/contracts/BaseTemplate.sol Show resolved Hide resolved
shared/contracts/BaseTemplate.sol Show resolved Hide resolved
shared/helpers/abi.js Outdated Show resolved Hide resolved
@@ -43,6 +43,7 @@
"@aragon/apps-agent": "^1.0.0",
"@aragon/apps-vault": "^4.1.0",
"@aragon/apps-voting": "^2.1.0",
"@aragon/apps-payroll": "^1.0.0-rc.1",

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 1, 2019

Member

Don't think we need this dependency here 馃槃

This comment has been minimized.

Copy link
@ajsantander

ajsantander Aug 1, 2019

Author Contributor

There's something weird going on, if that dependency isn't there, Trust doesn't compile. Or at least that was the case with the CI.

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 7, 2019

Member

Hmm, I see, the contract resolution algorithm is a bit tricky with how npm works (and it's why we can't hoist contracts when using lerna).

Not sure if truffle@5 has made any inroads to fixing this, but for now I've moved the unused apps into the dev dependencies.

shared/helpers/assertRevert.js Outdated Show resolved Hide resolved
templates/company/test/company.js Outdated Show resolved Hide resolved
templates/company/test/company.js Outdated Show resolved Hide resolved
templates/company/contracts/CompanyTemplate.sol Outdated Show resolved Hide resolved
address denominationToken = _toAddress(_payrollSettings[0]);
IFeed priceFeed = IFeed(_toAddress(_payrollSettings[1]));
uint64 rateExpiryTime = _payrollSettings[2].toUint64();
address employeeManager = _toAddress(_payrollSettings[3]);

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 1, 2019

Member

One small thing is perhaps doing this coercion at the start of newInstance() instead, so the user doesn't waste gas creating an entire org before their input fails here.

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Aug 3, 2019

Member

see my summary comment bellow

@facuspagnuolo

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

I've been wasting a lot of time dealing with stack too deep issues... we do have to start thinking on either using ABI encoder V2 or having less optimized templates (e.g. reading settings from storage, using more txs, or sth like that).

Note that the code looks completely awkward due to how we need to load from storage, pass by argument, and the rest of ticks and tricks that we needed to do to avoid compiling issues, it definitely won't look good for inexperienced users as an example of how to build a template :/

@facuspagnuolo facuspagnuolo self-assigned this Aug 3, 2019

@facuspagnuolo facuspagnuolo requested a review from sohkai Aug 3, 2019

@facuspagnuolo facuspagnuolo changed the title (WIP) Add payroll app to all templates Add payroll app to all templates Aug 3, 2019

@facuspagnuolo facuspagnuolo referenced this pull request Aug 3, 2019
@ajsantander

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

I've been wasting a lot of time dealing with stack too deep issues... we do have to start thinking on either using ABI encoder V2 or having less optimized templates (e.g. reading settings from storage, using more txs, or sth like that).

Note that the code looks completely awkward due to how we need to load from storage, pass by argument, and the rest of ticks and tricks that we needed to do to avoid compiling issues, it definitely won't look good for inexperienced users as an example of how to build a template :/

@facuspagnuolo
ABI encoder V2 sounds nice, but we would need to either enable experimental features on contracts (not even sure if 0.4.24 has the feature or how stable it is), or update all contracts to a newer version of Solidity like 0.5.x.

IDEA: What about implementing a very simple system in which any template can be initialized with 2 transactions, no matter how basic or complex it is.

tx 1) The user deploys a "configuration contract" that simply holds state in a bunch of public variables that can be read later by a template being initialized.

tx 2) The template's initialize function, instead of getting its settings from parameters in a function call, retrieves them from the configuration contract by reading state. The template could even selfdestruct the configuration contract, getting a bit of gas refunded.

The configuration contract could also contain booleans like "installPayrollApp", so that much of the modular functionality of installing a particular combination of apps could be abstracted to the BaseTemplate, making templates even simpler to read and understand.

const ONE_DAY = 60 * 60 * 24
const ONE_WEEK = ONE_DAY * 7
const THIRTY_DAYS = ONE_DAY * 30
const TWO_MONTHS = ONE_DAY * 31

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 7, 2019

Member

This is a bit misleading; perhaps MORE_THAN_ONE_MONTH would be clearer?

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Aug 7, 2019

Member

Ohhh, it definitely should be ONE_MONTH


function _setupPermissions(Voting _shareVoting, TokenManager _shareTokenManager, Vault _agentOrVault, Finance _finance, bool _useAgentAsVault) internal returns (Voting) {
Kernel dao = _fetchDaoCache();
ACL _acl = ACL(dao.acl());

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 7, 2019

Member

Can we name this as just acl?

This comment has been minimized.

Copy link
@facuspagnuolo
{
Kernel dao = _popDaoCache(msg.sender);
_setupApps(dao, _boardMembers, _shareHolders, _shareStakes, _boardVotingSettings, _shareVotingSettings, _financePeriod, _useAgentAsVault);
function setupShare(string _id, address[] _holders, uint256[] _stakes, uint64[3] _votingSettings, uint64 _financePeriod, bool _useAgentAsVault) external {

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 7, 2019

Member

If I understand it correctly now, the flow is:

  1. prepareInstance()
  2. setupBoard()
  3. setupShare()

Is it not possible to combine prepareInstance() with setupBoard()?

This comment has been minimized.

Copy link
@facuspagnuolo

facuspagnuolo Aug 7, 2019

Member

It may be possible since they cost 5e6 and 1.3e6 respectively, which means they should fit in a single tx

@@ -43,6 +43,7 @@
"@aragon/apps-agent": "^1.0.0",
"@aragon/apps-vault": "^4.1.0",
"@aragon/apps-voting": "^2.1.0",
"@aragon/apps-payroll": "^1.0.0-rc.1",

This comment has been minimized.

Copy link
@sohkai

sohkai Aug 7, 2019

Member

Hmm, I see, the contract resolution algorithm is a bit tricky with how npm works (and it's why we can't hoist contracts when using lerna).

Not sure if truffle@5 has made any inroads to fixing this, but for now I've moved the unused apps into the dev dependencies.

@sohkai

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

using ABI encoder V2

Would ABI encoder V2 help with the stack issues? There's probably not too much we can do on that regard since we only get 16 slots?

IDEA: What about implementing a very simple system in which any template can be initialized with 2 transactions, no matter how basic or complex it is.

I'm not sure if this would be cheaper than just passing along arguments; the main benefit I see with deploying a configuration contract (or writing to a global, shared one), would be ease of recovering if you only sent 1 / 3 transactions during onboarding or etc.

sohkai added some commits Aug 7, 2019

@sohkai

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

@facuspagnuolo I've pushed a few commits that are mostly cosmetic / small fixes. Looking into changing the initialization structure of the company with board template (#124 (comment)) to see if I can come up with something more intuitive :).

@ajsantander

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

I made a quick experiment in remix to see if a configuration contract would help with the stack too deep problem, and it looks like the technique produces the errors even sooner than when using plain old function parameters. If you want to have a look and play with it yourselves: https://remix.ethereum.org/#version=soljson-v0.5.1+commit.c8a2cb62.js&optimize=false&gist=17461f2c1dbd9edb598acba1a46ea412

@sohkai

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

That is definitely not a good sign for the configuration contract... it's probably because doing a call involves quite a lot of stack variables.

@facuspagnuolo facuspagnuolo merged commit ce29559 into master Aug 7, 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 add_payroll_app branch Aug 7, 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鈥檛 perform that action at this time.