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

Optimizes app installations #125

merged 1 commit into from Aug 1, 2019


Copy link

commented Aug 1, 2019

馃弫 **Ready for Review 馃弫
Fix #105

@ajsantander ajsantander requested a review from facuspagnuolo Aug 1, 2019


This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

@facuspagnuolo note how the optimization couldn't be applied to the TokenManager app. This is because the TokenManager's initialize function will fail if it is not the current controller of the token.

This could be overcome by precomputing the address of the yet-to-exist TokenManager instance, since it should be a function of the current contract's address and its nonce. However, since the nonce is not know by a contract, the implementation may not be trivial.

Pseudo code:

return TokenManager(_installNonDefaultApp(_dao, TOKEN_MANAGER_APP_ID, abi.encodeWithSelector(0xe37ff29f, _token, _transferable, _maxAccountTokens)));
@luisivan luisivan referenced this pull request Aug 1, 2019
17 of 19 tasks complete

sohkai approved these changes Aug 1, 2019

Copy link

left a comment

Leaving the Token Manager out of the optimization sounds OK 馃憣.

shared/contracts/BaseTemplate.sol Show resolved Hide resolved
@sohkai sohkai referenced this pull request Aug 1, 2019

@facuspagnuolo facuspagnuolo merged commit 7b6e5eb into master Aug 1, 2019

4 checks passed

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

@delete-merged-branch delete-merged-branch bot deleted the optimized_init branch Aug 1, 2019

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