Reputation improvements

[area]

Closes #1124

Per-token reputation rate

In order to maintain some of the behaviour around payments (i.e. reputation being emitted on finalization, not claim), after a discussion with Arren, we came to the conclusion that we would limit the number of tokens that, if paid out from a colony, would result in reputation being awarded to the recipient. We landed on ten, which seems like plenty. In principle, this could be increased in the future, but only with the spectre of gas limits hanging over our heads.

The relevant tokens are stored in a linked list, and tokens must be inserted in order of ascending address in order to ensure there are not duplicate entries - indeed, when any editing of the list is taking place, the previous token must be provided.

The root permission is required to do this, for fairly obvious reasons.

An event is emitted when a token rate is set.

Per-domain reputation rate

Sadly, the same spectre of gas limits hangs over us with respect to per-domain reputation rate, given how it is intended to work (after a discussion with Arren, as this was not clear in the original issue). With multiple domains in a hierarchy, (e.g. A -> B -> C -> D), then the per-domain rate for D is equal to a factor set for D, times the factor (if any) set for C, and so on up the tree. It seems as if that the creation limit (because we note children / parents on skill creation) dwarfs this factor, but something to keep in mind.

Note that strictly, I've implemented this as a per skill reputation scaling, but only provide a mechanism for the skills of domains to be set. This gives us some latitude for the future to add per-skill scaling, but ultimately was driven by the requirement to walk up the skill tree, where it isn't possible to walk up (on-chain) the domain tree.

Again, the root permission is required to set the per-domain reputation rate. This is because of a scenario where the scale factor in a domain is set to 0, any non-root permission being allowed to set it to non-zero allows unexpected earning of reputation in root.

For both per-domain scaling, and per-token scaling, the scale factor is applied before being emitted to the reputation update log. No changes will be required once #1132 is merged as a result, and the reputation miners also need do nothing to accommodate these changes.

An event is emitted when a domain reputation rate is set.

Updates to payout scalar

Encompassing two bullet points in the requirements, permissions around payoutScalar have been changed - ultimately, those who were able to adjust it before can now adjust it between -1 and 0 (previously -1 and 1). This removes the ability to give bonus reputation using the payoutScalar. Root permission is now required to set the scalar above 0 (and can be set to an arbitrarily high value), which can be used in extraordinary circumstances to edit the reputation earned by a payment.

Per-colony decay rate

Fairly self-explanatory. I've limited the precision to the same amount of precision we have currently for the default value. Colonies can return to following our default decay rate even if they've set their own previously.

This feature will require a little additional work to sit alongside #1132, in that the function call that sets the decay rate will need to be bridged, but otherwise decay-rate calculations will occur as previously. Updates to the decay rate require one reputation cycle to complete, which is tracked by storing the address of the currently active cycle and not accessing the storage with a simple getter, but via getColonyReputationDecayRate.

The changes to the reputation miner code is such that it should be able to be deployed before the contract changes to allow a seamless transition.

An event is emitted when a colony's decay rate is set.

Other changes

I've made a few other changes on my way through this work:

• require(false, "...") replaced with revert(...)
• Change the solhint script, as IColony.sol is now too large to be piped in to solhint.
• I've changed the bn2bytes32 function to work as I expected, at least, with negative values.

Notes

I'm not 100% that I've covered all overflow guards for the scaling, so a special eye there would be appreciated.

[kronosapiens]

High level comments:

• Colony-specific decay rates I'm broadly happy with
• Ditto changes to payout scalar
• I think that the introduction of a linked list and capping the number of tokens earning reputation simply to support the feature of "give rep on finalization" represents an injudicious complexity/feature tradeoff
• I think that the introduction of domain scaling before the introduction of nested domains is over-engineering, and that feature should wait until we have actual colonies using nested domains

[kronosapiens]

@jakzilla @arrenv @area @rdig

Elaborating on my point at the end of the call, the current implementation of setTokenReputationRate is 29 lines. Of those, 28 lines are not part of the core functionality, but rather used to implement a special data structure whose only purpose is to preserve support for a (in my opinion) minor feature: allowing reputation to be given out when a payment is finalized (as opposed to when a payment is claimed). Further, this data structure increases the amount of storage required to implement the token-specific reputation feature by 3x, as well as capping the number of tokens which can earn reputation in a colony to 10. If we remove this functionality, setTokenReputationRate can be reduced to one line, the storage requirement would be reduced by 2/3, and there would be no cap on the number of tokens which earn reputation.

The initial decision to give reputation when a payment is finalized (as opposed to when it is claimed) was (I believe) made in response to the argument that no one would "claim" a reputation penalty. While this is probably true, our claim functions have been made public, meaning that anyone can claim a payment on another's behalf. This means that if a payment would result in a negative penalty, and the recipient has no incentive to claim that penalty, that someone else is able to "claim" the penalty on their behalf.

One could argue that this second "claim" function would result in more transactions, and more headache for users. However, given that we have now added support for multicall, it would be straightforward to include any reputation penalty "claims" as part of a single transaction, meaning that these implementation decisions would be ultimately invisible to the user.

I am proposing that we drop the more-complex implementation of this functionality, given that we can achieve the desired product goal in an alternative way, which would result in less overall complexity across the contracts and dapp. The addition of a multicall as part of finalization would require the dapp to make a few more queries on finalization, but I believe this would still be a net simplification over the current implementation.

[arrenv]

To make it clear, this is what we lose from the simplified version:

• There would be no reputation penalty on claiming (although, offset by public claiming function and multicall)

This is what we gain:

• More then 10 tokens that can earn reputation
• Simpler overall code

If this cover the differences, then I would opt for the simplified version as you have described.

[area]

Need to be able to set scale factor for reputation mining. EDIT: This is now done.