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

Expenditures extension, utilities, updates #1045

Merged
merged 59 commits into from
Aug 5, 2022
Merged

Conversation

kronosapiens
Copy link
Member

@kronosapiens kronosapiens commented May 11, 2022

Closes #1042

  • Add StakedExpenditure for creating expenditures via stakes, and reclaiming or slashing stakes

Also:

  • Add a new moveFundsBetweenPots which accept arrays of tokens & amounts
  • Add a refundDomain private function which returns excess funding back to the domain on claiming of payment
  • Add a setExpenditureValues function which can change all expenditure values in one transaction, for owners
  • Add a setExpenditurePayout function to update slot payouts with correct bookkeeping, for arbitrators

The introduction of setExpenditurePayout addresses a bug in the existing implementation, where using setExpenditureState to update payouts would allow the funding pot bookkeeping to fall out of sync. setExpenditurePayout as an arbitration function provides an alternative for arbitrators (with an eye towards motions) to update payouts with correct bookkeeping. This introduction has necessitated an update to VotingReputation to correctly handle both types of expenditure state changes.

@kronosapiens kronosapiens force-pushed the maint/expenditures-2 branch 3 times, most recently from 75b91ff to 27f50b4 Compare May 12, 2022 21:14
@kronosapiens kronosapiens force-pushed the maint/expenditures-2 branch 3 times, most recently from 558f8bc to 7895b5e Compare May 13, 2022 20:12
@kronosapiens kronosapiens force-pushed the maint/expenditures-2 branch from 5d2198e to f520243 Compare May 25, 2022 04:49
@kronosapiens kronosapiens force-pushed the maint/expenditures-2 branch from 1790070 to a10ad27 Compare May 25, 2022 10:47
Copy link
Member

@area area left a comment

Choose a reason for hiding this comment

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

Would it be possible to move the creation of ColonyRewards in to a new PR that could be reviewed (presumably quickly) separately? I find it very hard to keep track of what's changed when large chunks also move around.

contracts/extensions/ExpenditureUtils.sol Outdated Show resolved Hide resolved
contracts/extensions/ExpenditureUtils.sol Outdated Show resolved Hide resolved
contracts/colony/ColonyExpenditure.sol Show resolved Hide resolved
contracts/colony/ColonyStorage.sol Outdated Show resolved Hide resolved
contracts/colony/IColony.sol Outdated Show resolved Hide resolved
contracts/colony/ColonyFunding.sol Show resolved Hide resolved
contracts/extensions/ExpenditureUtils.sol Outdated Show resolved Hide resolved
@kronosapiens kronosapiens force-pushed the maint/expenditures-2 branch 2 times, most recently from d15bad5 to a1354bb Compare May 26, 2022 16:52
@arrenv arrenv mentioned this pull request May 27, 2022
11 tasks
@area area force-pushed the maint/expenditures-2 branch 4 times, most recently from 496a283 to 7672b73 Compare May 27, 2022 14:52
@kronosapiens kronosapiens force-pushed the maint/expenditures-2 branch 7 times, most recently from 51daa24 to 377d00e Compare June 7, 2022 22:33
@kronosapiens kronosapiens force-pushed the maint/expenditures-2 branch from 377d00e to 758a208 Compare June 8, 2022 13:07
Copy link
Member

@area area left a comment

Choose a reason for hiding this comment

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

There's a lot of low-hanging fruit, branch-coverage wise, on setExpenditureValues. Don't just rest on the laurels of my commit you've cherry picked ;). Same for the new moveFundsBetweenPots.

This PR more or less solves two-out-of-three issues that I brought up in #1031... should it solve the third as well (i.e. the claim delay stuff)? Or leave that for another PR?

Cancelling an expenditure currently has no influence on reputation, which it is spec'd as (having the option of) doing.

contracts/colony/ColonyAuthority.sol Show resolved Hide resolved
contracts/colony/ColonyAuthority.sol Outdated Show resolved Hide resolved
contracts/tokenLocking/TokenLocking.sol Outdated Show resolved Hide resolved
contracts/extensions/ExpenditureUtils.sol Outdated Show resolved Hide resolved
contracts/extensions/ExpenditureUtils.sol Outdated Show resolved Hide resolved
contracts/extensions/ExpenditureUtils.sol Outdated Show resolved Hide resolved
@area area force-pushed the maint/expenditures-2 branch from 3d45e9d to b79d9b1 Compare August 4, 2022 14:13
@area area merged commit 7fe3d19 into develop Aug 5, 2022
@area area deleted the maint/expenditures-2 branch August 5, 2022 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expenditures Extension Brief
3 participants