Build on aragonOS #62
Loading…
x
Reference in New Issue
Block a user
No description provided.
Delete Branch "aragonos"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This PR builds on #55 (includes the contribution contract) and refactors the smart contracts to use aragonOS.
Sadly it got a bit of a big refactoring 🤷♂️ that also follows the typical directory structure for aragonOS based contracts. Those are called "apps" and organized in independent folders (which could also be independent npm packages)
Advantages
Upgradeablity
This allows use to develop and deploy each contract independently and use the aragon upgrading system to upgrade the DAO/our kredits instance that uses the app.
We don't have to develop our own upgrading proxy and keep our own registry - we can focus on our core contracts/features and less on base infrastructure.
Permission system
Aragon brings a powerful permission system that allows to evaluate the ACL on runtime and thus to more loosly couply our contracts.
For example to only allow contributors to vote: The proposal contract does no longer need to know about contributors it simply defines a permission and role. The permission is then configured in the ACL and can later be easly updated without the requirement to change the contract.
Contracts / Apps
apps/contributionmanages contributions. currently allows to add and claim contributions.
missing: veto of contributions
apps/contributorsmanages all contributor profiles
apps/proposalsallows to add and vote on proposals that issue contribtions.
apps/tokenthe actual ERC20 kredits token.
issued by claiming contributions
Usage of the kredits-contracts wrapper
The usage of the wrapper does not change. It slightly got updated to use the aragon DAO system instead of our own registry.
About AragonOS
It is important to mention that using aragon tools does NOT mean we run within the aragon app/network! So far we only use the tools provided by aragonOS. These are mainly the permission system and the upgradeability of contracts.
We can allow our contracts/apps to be used with other aragon "apps" or even within the aragon UI, but that is not planned right now.
Links:
closes #57
closed #52
That looks pretty awesome to me!
I think we don't need that
capitalizehelper anymore and we should be consistent with theaddmethods on the contracts. We sometimes haveaddContributor,addContributionand sometimes justadd...@ -47,1 +57,4 @@);});});return RSVP.all(addressPromises).then(() => { return this });Can be removed
Do we still need it for backwards compatibility?
@ -0,0 +18,4 @@const daoFactoryAddress = arapp.environments[environment].daoFactory || process.env.DAO_FACTORYmodule.exports = async function(callback) {Are you a javascript pro, now?
What about:
Haha, make a function to update the
addresseFile.not sure, maybe kredits-web is still using it.
even though this PR changes a lot the API of the wrapper is still the same and I tried to minimize changes there.
@ -0,0 +18,4 @@const daoFactoryAddress = arapp.environments[environment].daoFactory || process.env.DAO_FACTORYmodule.exports = async function(callback) {a pro in being desperate :)
those scripts are executed in some truffle context and debugging sometimes is a pain because truffle swallows the errors and traces... so I sometimes threw everything on it :D
yep, did that now finally. and added a `helpers' directory...
do you have a good name for that file/function that injects that data based on the networkId?
I've deployed to rinkeby and the local devchain deployments work for me.
So would be awesome if you could test it and see if you can run it locally on your machine (see readme file)
To deploy to a testnet or mainnet it now actually also supports frame to handle keys, but there is an open PR to make this work with truffle 4 and web3 0.x which we are using.
it's been quite a lot of hacking and trial and error to get this running and I need to clean up some code, but feedback is welcome.
I feel like this is now finally a working smart contract setup to iterate on... let's see if it really works.
@ -0,0 +16,4 @@// ensure alphabetic orderenum Apps { Contribution, Contributor, Proposal, Token }bytes32[4] public appIds;❤️
@ -0,0 +18,4 @@const daoFactoryAddress = arapp.environments[environment].daoFactory || process.env.DAO_FACTORYmodule.exports = async function(callback) {Much better with a normal function!
I was thinking about
updateAddressesbut as it also updates IDs I thinkfileInjectworks fine.const networkId = await getNetworkId(web3);const networkId = await getNetworkId(web3);Have you tried the new suggestion feature in these comments? It's pretty useful! (And you'll be credited in the commit.)
@ -10,3 +5,1 @@const Registry = artifacts.require('./Registry.sol');Registry.deployed().then(async (registry) => {const initKredits = require('./helpers/init_kredits.js');const networkId = await getNetworkId(web3);const networkId = await getNetworkId(web3);👍
@ -0,0 +10,4 @@if (err) {reject(err);} else {resolve(network);Do the address files need the
networkIdas a string? Otherwise I would do theparseInthere.@ -0,0 +10,4 @@if (err) {reject(err);} else {resolve(network);yeah... that keeps bothering me also. but I don't know... maybe we sometimes use strings maybe integers.
I think I should go through the whole code once and make sure we use strings everywhere.
but also I am nut sure what web3 or ethers.js return as network id - if that is consistent.
@bumi luckily we didn't implement the diff ratio factor. You would be doomed: +70,784 −2,006
@fsmanuel yeah, I hoped nobody (and especially you) notices it :D
and we still should implement that... hopefully after this one is merged.. lol :D
I am wondering how I can prevent that those local address details get commited.
Those "14945560" network IDs are devchain IDs.
so far I don't think it is worth to write code for that and maybe simply try to ignore commiting those changes, but if somebody has an idea...?
ok, now we should soon merge it because I am adding stuff that really should not be here :/ :D
Maybe a git pre commit hook would work. I have never worked with git hooks...
Yes, I think that could be a good solution. Git hooks are pretty easy to work with actually.
hmm. the issue is locally you need those addresses in the file but in the repo we only need the commonly used ones.
so what would the hook do?
then it probably does not change that often...
now frame and potentially also local nodes are supported for deployment and for the helper scripts .
using infura the helper scripts now also have a readonly mode to testnet/mainnet deploys.
e.g.
truffle exec scripts/list-proposals.js --network=rinkebywill list all proposals from the rinkeby testnet.@ -0,0 +1,48 @@const promptly = require('promptly');const initKredits = require('./helpers/init_kredits.js');❤️
@ -62,3 +79,2 @@// TODO: rename to contributorreturn this.contractFor('contributors');return this.contractFor('Contributor');}You don't need the
()seeget Contributors()@ -62,3 +79,2 @@// TODO: rename to contributorreturn this.contractFor('contributors');return this.contractFor('Contributor');}ah ja... thx.
@ -0,0 +1,48 @@const promptly = require('promptly');const initKredits = require('./helpers/init_kredits.js');is it actually
init_kredits.jsor initKredits.js` as file name?I've updated ethers.js here now... as ethers.js is also a kredits-web dependency we have to update it there.
I should really wait until we merge this PR....
@ -0,0 +1,48 @@const promptly = require('promptly');const initKredits = require('./helpers/init_kredits.js');I think JS folks use
require('./helpers/init-kredits');You don't need the.jsShip it!
Went through the instructions and bootstrap etc. worked like a charm!
I think we should merge this asap, and then finish everything with new, smaller PRs from master.
@skddc if the last changes solve the problem of the changing network id then merge it. \o/