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

Token weighted voting #90

Open
elenadimitrova opened this Issue Oct 3, 2017 · 29 comments

Comments

Projects
None yet
2 participants
@elenadimitrova
Member

elenadimitrova commented Oct 3, 2017

9.3.2 Token weighted voting

Unlike with reputation, we do not have the ability to ‘freeze’ the token distribution when a vote starts. While this is effectively possible with something like the MiniMe token [8], we envision token-weighted votes will still be regular enough within a Colony that we do not wish to burden users with the gas costs of deploying a new contract every time.
When conducting a token weighted vote, steps must be taken to ensure that tokens cannot be
used to vote multiple times. In the case of “The DAO”, once a user had voted their tokens were locked until the vote completed. This introduced peculiar incentives to delay voting until as late as possible to avoid locking tokens unnecessarily. Our locking scheme avoids such skewed incentives. Instead, once a vote enters the reveal phase, any user who has voted on that vote will find themselves unable to see tokens sent to them, or be able to send tokens themselves — their token balance has become locked. To unlock their token balance, users only have to reveal the vote they cast for any polls that have entered the reveal phase — something they can do at any time. Once their tokens are unlocked, any tokens they have notionally received since their tokens became locked are added to their balance.
It is possible to achieve this locking in constant gas by storing all submitted secrets for votes in a sorted linked list indexed by close_time. If the first key in this linked list is earlier than now when a user sends or would receive funds, then they find their tokens locked. Revealing a vote causes the key to be deleted (if the user has no other votes submitted for polls that closed at the same time). This will unlock the tokens so long as the next key in the list is a timestamp in the future. A more detailed description of our implementation can be found on the Colony blog [9].
Insertion into this structure can also be done in constant gas if the client supplies the correct insertion location, which can be checked efficiently on-chain, rather than searching for the correct location to insert new items.

@elenadimitrova elenadimitrova added this to the Colony Network Token Sale MVP milestone Oct 3, 2017

@elenadimitrova elenadimitrova removed this from the Colony Network Token Sale MVP milestone Feb 1, 2018

@elenadimitrova

This comment has been minimized.

Member

elenadimitrova commented Feb 11, 2018

This is already implemented in https://github.com/JoinColony/colonyNetwork/tree/feature/token-weighted-voting branch but that is well out of date with develop as it uses the EterenalStorage-contract and libraries upgrade approach rather than EtherRouter.
It is however well documented functionality presented at devcon2 and the 3-part blog series and so it makes a good first issue for contributors.

@markhainesii

This comment has been minimized.

markhainesii commented Aug 28, 2018

I'd like to study your blogs and return to this issue. I see, for instance, VotingLibrary in the old branch. Is this where I get most/some of the functionality?

@elenadimitrova

This comment has been minimized.

Member

elenadimitrova commented Aug 28, 2018

Yes @markhainesii . You can compare the changes there to master as develop is based on the new upgrade model using delegate proxies a.k.a. EtherRouter whereas master is still on the simpler EthernalStorage pattern master...feature/token-weighted-voting

@markhainesii

This comment has been minimized.

markhainesii commented Aug 29, 2018

@elenadimitrova I have a general idea now and a few cursory points about the old branch to start:

  • How close do you want the files to resemble the current pattern, interface-storage-contract. I realise this is for upgrade and gas efficiency?
  • Are the recent TokenLocking contracts reusable for this issue?
  • I'll update any old syntax like var, constant, etc
@elenadimitrova

This comment has been minimized.

Member

elenadimitrova commented Aug 30, 2018

@markhainesii sounds very much like you're on the right track! On your questions respectively:

  • The proxy/delegate pattern aka EtherRouter has to be followed here as we want both upgradability and gas efficiency in the implementation
  • TokenLocking has been designed for reuse indeed!
  • 👍
@markhainesii

This comment has been minimized.

markhainesii commented Aug 30, 2018

Great, thanks for clarifying. I'm interested in spending some time on this. Do I need to be assigned at this stage?

@elenadimitrova

This comment has been minimized.

Member

elenadimitrova commented Aug 30, 2018

Great. Not for now Mark. This conversation is sufficient for other collaborators to understand you're actively working on it. Any questions or if something blocks you, let us know here or Gitter

@markhainesii

This comment has been minimized.

markhainesii commented Sep 6, 2018

I'm looking at the TokenLocking contract and the function:

function setColonyNetwork(address _colonyNetwork) public auth {
    colonyNetwork = _colonyNetwork;
  }

where is the address for _coloyNetwork coming from? Is it normally passed during migration?

@elenadimitrova

This comment has been minimized.

Member

elenadimitrova commented Sep 6, 2018

@markhainesii

This comment has been minimized.

markhainesii commented Sep 7, 2018

Relating to the proxy pattern: do all new "logic" contracts require address setter functions added to ColonyNetwork.sol? Then setResolver() is passed these addresses? I'm trying to follow the order of things!

@elenadimitrova

This comment has been minimized.

Member

elenadimitrova commented Sep 9, 2018

setResolver is only called once on the respective EtherRouter so the latter knows where to lookup incoming function signatures (that is in the Resolver instance). To get new logic contract functions registered properly we expose their public functions through the corresponding interface contract e.g. IColonyNetwork, IColony etc. Then the script here registers all that in the resolver https://github.com/JoinColony/colonyNetwork/blob/develop/helpers/upgradable-contracts.js#L60
Not sure I answered your question though..

@markhainesii

This comment has been minimized.

markhainesii commented Sep 10, 2018

Ok thanks. I was missing some setup process and didn't notice this script. Is this the same Resolver instance that is stored at the beginning of the storage contracts: address resolver ?

@elenadimitrova

This comment has been minimized.

Member

elenadimitrova commented Sep 10, 2018

Yes the resolver address in the beginning of the storage contracts is there as a declaration to match the address in EtherRouter as storage slots have to match between the two.

@markhainesii

This comment has been minimized.

markhainesii commented Sep 14, 2018

I've come across this unfamiliar nested statement for two abstract contracts. Is this a conversion or a reference to functions from two contracts?
address clnyToken = IColony(IColonyNetwork(colonyNetwork).getMetaColony()).getToken();

@elenadimitrova

This comment has been minimized.

Member

elenadimitrova commented Sep 16, 2018

We are twice casting an address to interface in order to satisfy Solidity type safety and call the functions on an address we know is of certain interface. Now however we have a slightly better and shorter way to achieve the above, which essentially gets the CLNY token address.
We store the Meta colony address on the Colony Network at metaColony this is accessible inside the ColonyNetwork contract instance via the property name itself and outside - via the IColonyNetwork.getMetaColony function. After you have that, to get the CLNY token address you just can just do one cast :address clnyToken = IColony(metaColony).getToken();

@markhainesii

This comment has been minimized.

markhainesii commented Sep 17, 2018

I'm trying to decide on what the mappings should look like for the new token weighted voting storage. For every hashed list and value that used to update the EternalStorage mappings, would there be any problems making a global bytes32 => bytes32 mapping instead? I'm considering all the mixed types.

@elenadimitrova

This comment has been minimized.

Member

elenadimitrova commented Sep 17, 2018

You mean for storing the secret votes? That is indeed a bytes32 voting key mapped to a bytes32 voting secret. Not sure what you mean by "considering all the mixed types" but hopefully I've answered your question.

@markhainesii

This comment has been minimized.

markhainesii commented Sep 17, 2018

Yes, but for example I also mean the other storage entries like:
EternalStorage(_storageContract).setUIntValue(keccak256("Poll", pollId, "startTime"), now);
which I think is uint. Maybe I'm over-complicating.

@elenadimitrova

This comment has been minimized.

Member

elenadimitrova commented Sep 17, 2018

Oh right you're asking in general how you should model the storage. I imagine there will be a few structs like Poll, Vote etc and some mappings to store these e.g mapping(uint256 => Poll) public polls;

@markhainesii

This comment has been minimized.

markhainesii commented Sep 18, 2018

Good, thanks. Apart from making this issue fit the EtherRouter pattern, is there anything else in particular that should change?

@elenadimitrova

This comment has been minimized.

Member

elenadimitrova commented Sep 18, 2018

I can't think of anything but the token locking which needs to reuse the one we've already implemented. Although it has been two years after we wrote it and I'm pretty sure in the course of implementation there maybe changes needed.

@markhainesii

This comment has been minimized.

markhainesii commented Sep 25, 2018

Going on the struct idea in previous comments, this old code will not be compatible with a mapping to struct type:
var pollOptionCount = EternalStorage(_storageContract).getUIntValue(keccak256("Poll", pollId, "OptionsCount"));
So if it's the uint256 value being assigned to pollOptionCount I'm hoping it will be safe to change this to:
uint256 pollOptionCount = option[keccak256(abi.encodePacked("Poll", pollId, "OptionsCount"))].pollOptionCount;

@elenadimitrova

This comment has been minimized.

Member

elenadimitrova commented Sep 26, 2018

Bear in mind that the EternalStorage design was made to ensure mapping keys are unique, hence the long key "Poll", pollId, "OptionsCount"... being hashed. You don't have to do any of that in the EtherRouter and can just map uint256 -> Poll

@markhainesii

This comment has been minimized.

markhainesii commented Sep 26, 2018

ok, that will look a lot nicer as well. So I don't go too far, I have a Gist showing a concept for this. Is it similar to what you are expecting? https://gist.github.com/markhainesii/f06ad9f928d554aead30fc1ada53042b

@elenadimitrova

This comment has been minimized.

Member

elenadimitrova commented Sep 26, 2018

I don't know what storage will end up looking like but this seems like a good enough starting point. I expect there will be further normalization of the storage design needed once you start refactoring.

@markhainesii

This comment has been minimized.

markhainesii commented Oct 3, 2018

Does a linked list have to be at least two polls: one zero poll and one real poll so as to keep the "closed" relationship: prevTimestamp > prevPollId > nextTimestamp > nextPollId ?

@elenadimitrova

This comment has been minimized.

Member

elenadimitrova commented Oct 3, 2018

Yes exactly @markhainesii

@markhainesii

This comment has been minimized.

markhainesii commented Oct 4, 2018

Great. And I know it's not necessary this time, but how should I interpret mapping keys like "Voting", userAddress, prevTimestamp, "secrets", zeroUint, "nextPollId" so I can create new struct variables?

@elenadimitrova

This comment has been minimized.

Member

elenadimitrova commented Oct 5, 2018

I'm not sure I understand completely but basically yes, there will likely need to be a struct defined containing some of these properties. Also as a general comment, it might be best to get a branch in so we can have a look at actual implementation changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment