Fix Service Provider rewards math #887
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Break out calculating rewards for a single service provider. This means in a test when we only care about a single sp, we can find that record and get all of the rewards from it without having to do any math. So the math can not differ from prod.
Previously, half the values were representing DC, and the other half Bones. You cannot take percentages of one unit from another unit when they are scaled differently. Early on, we convert dc -> bones, then calculate percentages based off those numbers. dc_perc has been renamed to data_perc because it is the percentage of rewards that is allocated because of data transfer. Rounding now uses the floor function provided by rust_decimal.
we no longer need to know the rewards_per_share, because we are not doinga share type algorithm for computing rewards. We get percentages of a whole, which has the same effect as multipling shares by a fraction of reward units.
michaeldjeffrey
requested review from
macpie,
andymck,
bbalser and
jeffgrunewald
| @@ -2275,7 +2275,7 @@ mod test { | |||
| // force the service provider to have spend more DC than total rewardable | |||
| ServiceProviderDCSessions::from([(sp1, total_rewards_value_in_dc * dec!(2.0))]), | |||
| ServiceProviderPromotions::default(), | |||
| total_rewards_value_in_dc, | |||
There was a problem hiding this comment.
this variable doesn't seem to be used anymore but we're still calculating it above?
Comment on lines
+59
to
+60
| total_sp_allocation: Decimal, // Bones | ||
| mobile_bone_price: Decimal, // Price in Bones |
There was a problem hiding this comment.
i didn't think fmt would let you do a line-ending comment like this but great news if it does
bbalser
approved these changes
Nov 1, 2024
jeffgrunewald
approved these changes
Nov 1, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TLDR;
Mixing unit is bad. mmmkay
Previously, for calculating which percentage of rewards a Service provider should receive, the calculation was suing this number as it's base.
let used_allocation = total_sp_allocation.max(all_transfer);total_sp_allocationwas the maximum number of Bones that could be rewarded in an epoch.all_transferwas all the DC transferred in the epoch. Because they were 2 different units, and Bones is at a scale far surpassing DC, the number would always betotal_sp_allocation.This is not a problem, until the amount of DC transferred when converted to Bones exceeds the amount allocated.
That leads to a situation where we would assume there is more rewards available than exist. Which in this case lead to matching rewards when there were non unallocated.
Now, we convert DC to bones as soon as possible. Since we're always working in the same units, the percentages should hold true.