feat: IC-1680: Add NPR computation V1 in ic-registry-node-provider-rewards lib #2655
Conversation
pietrodimarco-dfinity
changed the title
pietrodimarco-dfinity
changed the title
# Conflicts: # Cargo.Bazel.Fuzzing.json.lock # Cargo.Bazel.json.lock
# Conflicts: # Cargo.Bazel.Fuzzing.json.lock # Cargo.Bazel.json.lock
| /// failure rate of the subnet from the node's failure rate for each day. | ||
| /// If the node's failure rate is less than the systematic failure rate, the idiosyncratic | ||
| /// failure rate is set to zero. | ||
| fn nodes_idiosyncratic_fr( |
There was a problem hiding this comment.
idiosyncratic has more of a connotation of "weird" or "peculiar", and maybe only serves to tell someone to read the function.
Maybe we could say "node_failure_rate_relative_to_subnet" or something more wordy, especially since it's only used locally. I'm not sure that's a great name either.
There was a problem hiding this comment.
That's a fairly good name...maybe "compute_relative_node_failure_rate" wdyt?
| } | ||
|
|
||
| #[derive(Clone, Hash, Eq, PartialEq, Debug, Default)] | ||
| pub struct DailyNodeMetrics { |
There was a problem hiding this comment.
Because this is called daily_node_metrics, it makes it difficult to also use the variable name "daily_node_metrics" to refer to the vector of DailyNodeMetrics that spans a number of days.
I wonder if there's a clearer name for this... It's not actually multiple metrics, it seems to only be a single measurement, with some supporting data, and a timestamp that actually indicates a time period (i.e. a whole day).
As I've been reviewing, I've been having trouble tracking the logical flow of data as the shapes of each collection is transformed, and I think a large part of the difficulty is the naming of variables and concepts, and perhaps some type ambiguity.
There was a problem hiding this comment.
I see your point and I agree...I called it DailyNodeMetrics to stay consistent with "NodeMetrics" name from NodeMetricsHistoryResponse. One solution could be instead of using daily_node_metrics for the Vec use the concept of rewarding period as I did. wdyt?
# Conflicts: # Cargo.Bazel.Fuzzing.json.lock # Cargo.Bazel.json.lock
The PR adds to
ic-registry-node-provider-rewardslib the v1 modules for computing Node Provider rewards with the following differences with respect to v0: