-
Notifications
You must be signed in to change notification settings - Fork 71
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
Fix past councils spending amounts #4554
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
"total spent" column should include the total of councilor reward + WG budget updates + funding proposals + channel payout rewards |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once the CI error is fixed, I believe the PR is good
proposalsApproved: data?.proposalsApproved?.totalCount ?? 0, | ||
proposalsRejected: (data?.proposalsRejected?.totalCount || 0) + (data?.proposalsSlashed?.totalCount || 0), | ||
totalSpent: data && getTotalSpent(data.budgetSpendingEvents), | ||
// totalSpent: data && getTotalSpent(data.budgetSpendingEvents), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove unnecessary line
Tested on https://dao-h1gyak8tv-joystream.vercel.app/#/council/past-councils @chrlschwb I couldn't find the numbers that correspond to the total spent... this is term 19 |
// totalSpent: getTotalSpent(spendingEvents), | ||
totalSpent: councilFields.councilMembers | ||
.reduce((a, b) => a.add(new BN(b.accumulatedReward)), BN_ZERO) | ||
.add(workingGroupBudgets.reduce((a, b) => a.add(new BN(b.budgetChangeAmount)), BN_ZERO)) | ||
.add(getSpentOnProposals(fundingRequestsApproved)) | ||
.add(workingGroupRewardPaidEvents.reduce((a, b) => a.add(new BN(b.amount)), BN_ZERO)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this code will probably have to be updated in the furture I think it's worth makin it a bit clearer so please remove the export
from getTotalSpent
and update it's definition to something like:
const getTotalSpent = (
councilMembers: ...,
workingGroupBudgets: ...,
workingGroupRewardPaidEvents: ...,
fundingRequestsApproved: ...,
) => {
const totalAccumulatedReward = councilMembers
.reduce((a, b) => a.add(new BN(b.accumulatedReward)), BN_ZERO)
const totalBudgetChange = workingGroupBudgets
.reduce((a, b) => a.add(new BN(b.budgetChangeAmount)), BN_ZERO)
const totalRewardPaid = workingGroupRewardPaidEvents
.reduce((a, b) => a.add(new BN(b.amount)), BN_ZERO)
const spentOnProposal = getSpentOnProposals(fundingRequestsApproved)
return totalAccumulatedReward
.add(totalBudgetChange)
.add(totalRewardPaid)
.add(spentOnProposal)
}
Then:
// totalSpent: getTotalSpent(spendingEvents), | |
totalSpent: councilFields.councilMembers | |
.reduce((a, b) => a.add(new BN(b.accumulatedReward)), BN_ZERO) | |
.add(workingGroupBudgets.reduce((a, b) => a.add(new BN(b.budgetChangeAmount)), BN_ZERO)) | |
.add(getSpentOnProposals(fundingRequestsApproved)) | |
.add(workingGroupRewardPaidEvents.reduce((a, b) => a.add(new BN(b.amount)), BN_ZERO)), | |
totalSpent: getTotalSpent(councilFields.councilMembers, workingGroupBudgets, workingGroupRewardPaidEvents, spentOnProposal), |
Also @chrlschwb should channel payout be included here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe channel payout should be included
@ivanturlakov thanks for the test. can you please check with other council term reports? |
Tested on https://dao-h1gyak8tv-joystream.vercel.app/#/council/past-councils @chrlschwb Seems that the amount of validator rewards is not the cause I tried playing with numbers yesterday but it didn't help me find the cause
|
@ivanturlakov I think council reports could contain some errors since they are amalgamation of WG reports and WG reports can contain slight errors due to unpaid rewards at the time of writing them, etc. However, this will require a more detailed analysis. |
@thesan @chrlschwb I have added channelPaymentMadeEvent for calculating |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Some more in depth QA is probably needed but since these are currently wrong in produciton anyway I'll merge this PR right now.
@chrlschwb same result as prev test |
fundingRequestsApproved: proposals( | ||
where: { | ||
inBlock_gt: $fromBlock | ||
inBlock_lt: $toBlock | ||
proposal: { details_json: { isTypeOf_eq: "FundingRequestProposalDetails" } } | ||
statusSetAtBlock_gt: $fromBlock | ||
statusSetAtBlock_lt: $toBlock | ||
details_json: { isTypeOf_eq: "FundingRequestProposalDetails" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The query should only return actually executed funding requests, and maybe just those approved between $fromBlock
and $toBlock
regardless from when they executed.
Fix the #4121 (wrong values in Councils and past councils)