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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reputation improvements #1140

Draft
wants to merge 47 commits into
base: develop
Choose a base branch
from
Draft

Reputation improvements #1140

wants to merge 47 commits into from

Conversation

area
Copy link
Member

@area area commented May 26, 2023

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.

Copy link
Member

@kronosapiens kronosapiens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

contracts/colony/ColonyFunding.sol Outdated Show resolved Hide resolved
contracts/colony/Colony.sol Outdated Show resolved Hide resolved
contracts/colony/ColonyPayment.sol Outdated Show resolved Hide resolved
contracts/colony/ColonyStorage.sol Outdated Show resolved Hide resolved
contracts/colony/ColonyStorage.sol Outdated Show resolved Hide resolved
contracts/colonyNetwork/IColonyNetwork.sol Show resolved Hide resolved
test/contracts-network/colony-permissions.js Show resolved Hide resolved
test/contracts-network/colony.js Outdated Show resolved Hide resolved
test/contracts-network/colony.js Show resolved Hide resolved
test/contracts-network/colony.js Outdated Show resolved Hide resolved
@kronosapiens
Copy link
Member

kronosapiens commented Jun 5, 2023

@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
Copy link
Member

arrenv commented Jun 6, 2023

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 area force-pushed the feat/reputation-improvements branch 3 times, most recently from abf0f20 to 5b6a948 Compare June 9, 2023 14:34
@area
Copy link
Member Author

area commented Jun 9, 2023

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

@area area force-pushed the feat/reputation-improvements branch from 5b6a948 to 7671757 Compare June 9, 2023 17:31
@area area force-pushed the feat/reputation-improvements branch from c7a43ba to 8d2fbd4 Compare June 10, 2023 21:05
@kronosapiens kronosapiens force-pushed the feat/reputation-improvements branch 8 times, most recently from 19f6889 to 1272c2f Compare June 16, 2023 15:28
@area area force-pushed the feat/reputation-improvements branch 2 times, most recently from b00abe5 to 61a2cae Compare June 18, 2023 10:55
contracts/colony/ColonyExpenditure.sol Show resolved Hide resolved
contracts/common/ScaleReputation.sol Outdated Show resolved Hide resolved
@area area mentioned this pull request Jun 19, 2023
@area area force-pushed the feat/reputation-improvements branch from bbaaddb to 1e79f5d Compare June 20, 2023 13:07
@area area force-pushed the feat/reputation-improvements branch 2 times, most recently from cc2f77e to 22031ec Compare June 30, 2023 10:17
@area area force-pushed the feat/reputation-improvements branch from 038f1f6 to a96f3e3 Compare August 8, 2023 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improving reputation
3 participants