Add radio-reward-v2 for mobile-reward-share #407
Conversation
There was a problem hiding this comment.
maybe a question for someone at another provider like Hotspotty but would it be helpful to group some of these fields into more sub-messages so a consumer can index into the decoded struct more quickly for just the components they care about? for instance a multipliers field that groups all the various multipliers, a reports field that groups the speedtests and the coverage report/coverage object for those who care to really dive into an audit?
src/service/poc_mobile.proto
Outdated
| boosted_hex_status_eligible = 0; | ||
| boosted_hex_status_location_score_below_threshold = 1; | ||
| boosted_hex_status_radio_threshold_not_met = 2; | ||
| boosted_hex_status_service_provider_ban = 3; |
There was a problem hiding this comment.
these are pretty verbose and redundant with the enum name; can the variants strip out the boosted_hex_status_ portion?
There was a problem hiding this comment.
I ran into issues when not including the prefix on enum values. Most of the other enums in the file follow the same pattern.
| oracle_boosting_assignment urbanized = 4; | ||
| oracle_boosting_assignment footfall = 5; | ||
| oracle_boosting_assignment landtype = 6; |
There was a problem hiding this comment.
should these be three sub-fields within a single message oracle_boosting_assignment?
There was a problem hiding this comment.
We can do that, just not sure what the extra nesting gets us.
There was a problem hiding this comment.
not a huge benefit, but the only benefit of keeping them under a sub-field I can think of is ability to have the field identifiers (those 4, 5, 6) sequential in case we add a new oracle_boosting_assignment field in the future
There was a problem hiding this comment.
I believe assignments should be separated as now, to have comprehensive information about forming an assignment_multiplier
src/service/poc_mobile.proto
Outdated
| uint64 base_coverage_points = 3; | ||
| // sum of boosted_coverage_points for all covered hexes | ||
| uint64 boosted_coverage_points = 4; |
There was a problem hiding this comment.
i could see it being easy to confuse someone that the name of these fields are the same name as the ones in the covered_hex message type since this one is an aggregate of all the same fields in the hotspot's covered hexes list
There was a problem hiding this comment.
would total_base_coverage_points and total_boosted_coverage_points work better?
There was a problem hiding this comment.
I agree, having these named the same is going to be confusing...prefixing with total makes the distinction clear but sum in place of total feels more descriptive to me
| @@ -438,6 +438,90 @@ message radio_reward { | |||
| repeated boosted_hex boosted_hexes = 10; | |||
| } | |||
|
|
|||
| enum boosted_hex_status { | |||
There was a problem hiding this comment.
if you move this enum into the radio_reward_v2, you could drop the boosted_hex_status_ prefix on each element ..make things less verbose.
There was a problem hiding this comment.
From my testing, boosted_hex_status cannot be moved into radio_reward_v2 because it shares the same name as the field. It appears the boosted_hex_status_ prefix can be dropped from the enum values anyways. Seeing as this is going out soon, and renaming fields is not a problem with proto enums as long as we don't change the order, I'm going to leave that for a less pressing time.
src/service/poc_mobile.proto
Outdated
| uint64 base_coverage_points = 3; | ||
| // sum of boosted_coverage_points for all covered hexes | ||
| uint64 boosted_coverage_points = 4; |
There was a problem hiding this comment.
I agree, having these named the same is going to be confusing...prefixing with total makes the distinction clear but sum in place of total feels more descriptive to me
| oracle_boosting_assignment urbanized = 4; | ||
| oracle_boosting_assignment footfall = 5; | ||
| oracle_boosting_assignment landtype = 6; |
There was a problem hiding this comment.
I believe assignments should be separated as now, to have comprehensive information about forming an assignment_multiplier
src/service/poc_mobile.proto
Outdated
| uint64 base_coverage_points = 2; | ||
| // base_coverage_points * (boosted_multiplier - 1) | ||
| uint64 boosted_coverage_points = 3; |
There was a problem hiding this comment.
Does it mean that earned coverage_points for particular hex is base_coverage_points + boosted_coverage_points?
src/service/poc_mobile.proto
Outdated
| // sum of base_coverage_points for all covered_hexes | ||
| uint64 base_coverage_points = 3; | ||
| // sum of boosted_coverage_points for all covered hexes | ||
| uint64 boosted_coverage_points = 4; |
There was a problem hiding this comment.
Is base_coverage_points + boosted_coverage_points = Accumulated coverage points for the radio? (coverage_points in message radio_reward ).
Is the same for base_poc_reward + boosted_poc_reward = radio's poc reward ?
Co-authored-by: andymck <andrewb.mckenzie@gmail.com>
bbalser
force-pushed
the
bbalser/radio-reward-v2
branch
from
8924be9
to
6f67db8
Compare
Proposal to add radio_reward_v2 to mobile_reward_share. The new radio_reward_v2 contains all the information that is used when determining the rewards for a radio. For a while both radio_reward and radio_reward_v2 would be written out for each radio to have backwards compatibility. The proposal is to add a v2 to create a clean break from the existing structure. we looked at trying to fit all the data into the existing structure but it seemed to add confusion.