DTH Pool implementation and tests #154
5 participants
Thanks for squashing them @jbaylina. One more hint. You did not really need to re-open a Pull Request. You could have done it in the same branch, and then you could use gut push --force on your branch for your repository.
This would have resulted in both your branch and the Pull Request being updated!
Anyhow thanks again and we will review the PR as soon as possible.
Looks good after my initial look. will have a deeper look later. Thanks you very much for putting this together!
LefterisJP
commented on an outdated diff
| @@ -1,6 +1,6 @@ | ||
| [{ | ||
| "name": "creation_seconds", | ||
| - "default": 35, | ||
| + "default": 60, |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
@jbaylina Rebasing may be easier for you if you remove the tests from here (since you will probably have a lot of conflicts there), and then make a new PR for the tests only.
LefterisJP
commented on an outdated diff
| @@ -48,3 +50,7 @@ paper/*.aux | ||
| paper/*.log | ||
| paper/*.out | ||
| paper/*.pdf | ||
| + | ||
| +# tmp directory | ||
| +tmp/ |
|
Please remove this, since I suppose it's user specific. There is no reason to add user specific stuff to .gitignore.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
LefterisJP
commented on an outdated diff
| +The DAO is free software: you can redistribute it and/or modify | ||
| +it under the terms of the GNU lesser General Public License as published by | ||
| +the Free Software Foundation, either version 3 of the License, or | ||
| +(at your option) any later version. | ||
| + | ||
| +The DAO is distributed in the hope that it will be useful, | ||
| +but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| +GNU lesser General Public License for more details. | ||
| + | ||
| +You should have received a copy of the GNU lesser General Public License | ||
| +along with the DAO. If not, see <http://www.gnu.org/licenses/>. | ||
| +*/ | ||
| + | ||
| +contract DAOProxy { | ||
| + function proposals(uint _proposalID) returns( |
|
Just leaving this here as a note. This DAO Proxy contract has the Until this bug is fixed, such a hack is necessary.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
@LefterisJP I just merged and resolved the conflicts. The main problem was the deployment of USNRewardPayOut.sol contract.
It would be good in the future to make a different deploy scripts for each contract.
The tests are working for me. The four contracts deploys perfectly.
creation_seconds adjusted from 38 -> 42
@jbaylina Thanks Jordi, I will have a deeper look and run the tests myself too.
colm
and 1 other
commented on an outdated diff
| + for (i = 0; i < pendingProposals.length;) { | ||
| + bool finalized = executeVote(pendingProposals[i]); | ||
| + if (finalized) { | ||
| + pendingProposals[i] = pendingProposals[pendingProposals.length - 1]; | ||
| + pendingProposals.length--; | ||
| + } else { | ||
| + i++; | ||
| + } | ||
| + } | ||
| + return true; | ||
| + } | ||
| + | ||
| + function fixTokens() returns (bool _success) { | ||
| + uint ownedTokens = dao.balanceOf(this); | ||
| + if (ownedTokens < totalPoolTokens) { | ||
| + throw; |
|
could I break @colm: If you send tokens directly to DTHPool then ownedTokens > totalPoolTokens.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
colm
and 1 other
commented on an outdated diff
| + } | ||
| + | ||
| + | ||
| + dao.vote(_proposalID, proposalStatus.suportProposal); | ||
| + proposalStatus.executed = true; | ||
| + VoteExecuted(_proposalID); | ||
| + | ||
| + return true; | ||
| + } | ||
| + | ||
| + function executeAllVotes() returns (bool _success) { | ||
| + uint i; | ||
| + for (i = 0; i < pendingProposals.length;) { | ||
| + bool finalized = executeVote(pendingProposals[i]); | ||
| + if (finalized) { | ||
| + pendingProposals[i] = pendingProposals[pendingProposals.length - 1]; |
|
Whats going on here? This is a very cheap way to mainatin a list of proposals pending to be executed. To remove an item from the list you just move the last item and overwrite it in the place of the item you just want to delete. Then you just decrement the array size. That's much cheaper that shifting all the elements from the list.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
There may be a problem with the undelegation call if the contract is very busy.
To get your tokens back with undelegateDAOTokens, you need to wait until there is a window in which the delegated tokens are not locked in a vote.
If are many proposals being voted on, and if maxTimeBlocked is relatively long, these windows where the tokens are free to transfer can be rare. If these lockup periods overlap, such a window may never occur at all.
(This can also be a kind of DDOS "attack" by the delegate: she can add .01 eth to the pool, and then vote on everything in sight, and in that way keep your tokens hostage, until the delegators pay a ransom)
I wrote a similar contract here that resolves that problem by having undelegate exclude your tokens from the voting pool, so that they are garanteed to be free to transfer as soon as the current lockup ends. https://github.com/Backfeed/dao-delegation/blob/master/VoteDelegator.sol
@jellegerbrandy:
1. The delegate can only vote in "non split" proposals. That should not be many.
2. The contract limits the window before the voting period ends that the contract can vote to the DAO. In my head this is about one hour, but it can be reduced if there are many proposals.
Regarding to your contract:
The main problem that I see with the approach of one account per user is this DoS attack. If lots of accounts delegate the vote, then the vote to the DAO function can get out of gas. If an attacker just split his tokens in many accounts and delegate all of them, it would be easy to prevent the contract to vote.
@jbaylina whether there will be many proposals to vote on or not we don't know yet - if https://dao.consider.it/ is an indication, there could be quite a lot. But you are right, 24 per day seems improbable :-).
You make a good point about the gas limits and the one account per user. I do not know of an elegant solution to it (except perhaps limiting it to some maximum length that is expected to keep the function within gas limits)
I removed all the tests so it can be merged without any conflict. I will add the tests later when the test sweet is more stable...
I still have problems with the solidity bug. I just commented in the code and prepare it for the fix. In the while I would prefer to keep it this way to be easy to deploy in older versions of mist and geth.
I want to note you that this work around is quite save because DAO1.0 will never change the way they return data. And the string is a pointer so it can be replaced by a uint256.
LefterisJP
commented on an outdated diff
| @@ -18,7 +18,16 @@ along with the DAO. If not, see <http://www.gnu.org/licenses/>. | ||
| import "./libs/oraclize.sol"; | ||
| import "./Token.sol"; | ||
| -contract DAOProxy { | ||
| +///////////////////// | ||
| +// There is a solidity bug in the return parameters that it's not solved | ||
| +// when the bug is solved, the import from DAO is more clean. | ||
| +// In the while, a workarround proxy is defined |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
LefterisJP
commented on an outdated diff
| @@ -18,7 +18,16 @@ along with the DAO. If not, see <http://www.gnu.org/licenses/>. | ||
| import "./libs/oraclize.sol"; | ||
| import "./Token.sol"; | ||
| -contract DAOProxy { | ||
| +///////////////////// | ||
| +// There is a solidity bug in the return parameters that it's not solved | ||
| +// when the bug is solved, the import from DAO is more clean. | ||
| +// In the while, a workarround proxy is defined | ||
| + | ||
| +// Uncoment this line when error xfixed |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
LefterisJP
commented on an outdated diff
| @@ -18,7 +18,16 @@ along with the DAO. If not, see <http://www.gnu.org/licenses/>. | ||
| import "./libs/oraclize.sol"; | ||
| import "./Token.sol"; | ||
| -contract DAOProxy { | ||
| +///////////////////// | ||
| +// There is a solidity bug in the return parameters that it's not solved | ||
| +// when the bug is solved, the import from DAO is more clean. | ||
| +// In the while, a workarround proxy is defined | ||
| + | ||
| +// Uncoment this line when error xfixed | ||
| +// import "./DAO.sol"; | ||
| + | ||
| +// Work arround proxy remove when fixed |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
LefterisJP
commented on an outdated diff
| @@ -42,7 +51,8 @@ contract DAOProxy { | ||
| function balanceOf(address _owner) constant returns (uint256 balance); | ||
| } | ||
| - | ||
| +// End of workArround proxy |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
LefterisJP
commented on an outdated diff
| @@ -222,7 +235,7 @@ contract DTHPool is DTHPoolInterface, Token, usingOraclize { | ||
| var (,,,votingDeadline, ,,,,newCurator) = dao.proposals(_proposalID); | ||
| - if (votingDeadline < now || newCurator) { | ||
| + if ((votingDeadline < now) || newCurator ) { |
|
no need for this parentheses here, please remove and no need for the extra whitespace either.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
@jbaylina You have to use the latest develop version of solc in order for the fix to work. At the moment there is no release of solc containing the fix so you have to build by source.
@jbaylina
As for the tests, I would actually like them in. I can help you with the rebase, or just tell you when it would be safe to rebase again.
You can rebase now for example ... and I will try to get your PR merged as soon as possible. Just do me a favour and squash the commits of removing and re-adding the tests and/or other files. I hate convoluted git history.
Regarding the bug fix as soon as there is a solc version that includes the fix then you should remove the proxy contract. I don't want any unecessary code that's just a workaround to avoid a bug when there is a release that fixes said bug.
It would be nice to have this PR merged, so that whenever we make changes to the testing framework from this point I can also modify your tests and you don't need to rebase anymore. You have done an amazing work and I don't want to make it more difficult for you to keep up with our changes ;)
@CJentzsch: This makes no changes to the actual DAO contracts and belongs to the userland. It is open for quite some time and constant rebasing becomes difficult. Let's merge it, all right?
This pull request implements a DTH Pool contract in the userland space.
The normal prcess of the contract is:
Thank you to @psdev, @colm, @LefterisJP and @hiromant for the idea and the discussion in thedao slack.