GH-711: Review Two #568
GH-711: Review Two #568
Conversation
…, next I should check if the test is still passing
…ion and can be left as done
…sions, adjustment runners)
…ion, disqualification_arbiter and helper_functions
…id of the adjustment runners
…ts (to enable easier understanding)
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub struct PurePercentage { | ||
| degree: u8, | ||
| } |
There was a problem hiding this comment.
It would be better if we keep the implementations of PurePercentage together with the struct. So, feel free to move things around in the way you seem fit.
| return zero; | ||
| } | ||
|
|
||
| let a = match N::from(self.per_cent).checked_mul(&num) { | ||
| let product_before_final_div = match N::try_from(self.degree as i8) | ||
| .expect("Each type has 100") |
There was a problem hiding this comment.
You'd like the expect message to use the word "integer", instead of the ambiguous type. You'd also like to replace this at other places. 😉
|
|
||
| let rounding = if Percentage::should_be_rounded_down(a) { | ||
| N::from(0) | ||
| fn return_zero<N>(&self, num: N) -> Option<N> |
There was a problem hiding this comment.
Maybe if we can call it with a more explicit name, something more intention revealing, that'll improve the readability. How about a suggestion from your side, prepare_zero_if_necessary or check_zero_and_prepare_it (this one is from me but both works).
| let is_signed = num < N::try_from(0).expect("Each type has 0"); | ||
| let divider = N::try_from(50).expect("Each type has 50"); | ||
| let abs_of_significant_digits = | ||
| Self::abs_of_least_significant_digits(least_significant_digits, is_signed); |
There was a problem hiding this comment.
You'd like to fix the name of this variable to contain the word least, as it's present in the fn name but missing in the variable name. Also, if you could rename is_signed to is_negative it'll be more explicit. I didn't mean to say, is_signed is wrong. So, please change it accordingly.
| } | ||
| } | ||
|
|
||
| fn div_by_100_and_round<N>(num: N) -> N |
There was a problem hiding this comment.
If you'd prefer to write something for this fn, then maybe a comment explaining the 'why' behind the rounding will actually help in understanding it better.
| { | ||
| num.checked_sub(&self.of(num)) | ||
| let to_subtract = self.of(num); | ||
| num.checked_sub(&to_subtract) | ||
| .expect("should never happen by its principle") |
There was a problem hiding this comment.
You could say, "mathematically impossible" (your suggestion)
| assert_eq!(subject.of(num), expected); | ||
| let half = num / N::try_from(2).unwrap(); | ||
| let one = N::try_from(1).unwrap(); | ||
| assert!((half - one) <= half && half <= (half + one)) |
There was a problem hiding this comment.
Please repair this assert, I don't think adding and subtracting 1 is necessary. Maybe I am wrong, but I am confident that's the case.
Apart from that, you may find passing the third argument useless in case you're calculating the half as the type N and asserting on the multiplied output.
If you could retain the literal numbers, it'll be amazing. Since, it makes it more readable.
|
|
||
| test_end_to_end(100, expected_values, |percent, base| { | ||
| PurePercentage::try_from(percent).unwrap().of(base) | ||
| }) |
There was a problem hiding this comment.
Here is what I think a quick reorder:
let base_value = 100;
let act = |percent, base| {
PurePercentage::try_from(percent).unwrap().of(base)
};
let expected_values = (0..=100).collect::<Vec<i8>>();
test_end_to_end(act, base_value, expected_values)
| .map(|num| num as u64) | ||
| .collect::<Vec<u64>>(); | ||
| assert_eq!(round_returned_range, expected) | ||
| assert_eq!(round_returned_range, expected_values) | ||
| } | ||
|
|
||
| #[test] | ||
| fn only_numbers_up_to_100_are_accepted() { |
There was a problem hiding this comment.
You could rename PurePercentage to Percentage. It was your idea, and I am convinced it's good to keep the name like this. Once you modify that, let's incorporate it in the name.
| (100, true), | ||
| (49, true), | ||
| (50, false), | ||
| (49, RoundingTo::SmallerPositive, RoundingTo::SmallerNegative), |
There was a problem hiding this comment.
As we both were looking at this test, an idea came up in your mind, to call these enums as BelowFifty and AboveFifty in place of Smaller and Bigger.
It sounds like a good improvement to me. 👍
|
|
||
| let result = percentage.subtract_percent_from(100); | ||
| let unsigned = subject.subtract_percent_from(100); | ||
| let signed = subject.subtract_percent_from(-100); |
There was a problem hiding this comment.
When I looked the assertion of the signed number, I was wrapped under the impression that the number will end up as -155. So, I got confused with the word subtract.
So, a better name for the function subtract_percent_from() will be decrease_by_percent_for(). Similarly, you'd like to rename the sibling function add_percent_from() to increase_by_percent_for().
| test_end_to_end(100, expected_values, |percent, base| { | ||
| LoosePercentage::new(percent as u32).of(base).unwrap() | ||
| }) | ||
| } |
There was a problem hiding this comment.
It would be good if you may reorder items in this test similar to an earlier comment, such that base value is on the top, then act and expected value.
You may like to do the same for the test below.
| } | ||
|
|
||
| #[test] | ||
| fn loose_percentage_multiplying_input_number_hits_limit() { |
There was a problem hiding this comment.
You'd like to rephrase this test name and in the test below such that hits_limit is right after loose_percentage.
A rephrase of this test name would be loose_percentage_hits_limit_at_multiplication
For the test below this it would be loose_percentage_hits_limit_at_addition_from_remainder.
|
|
||
| #[test] | ||
| fn verify_bill_payment() { | ||
| fn full_payments_were_processed_for_sufficient_balances() { |
There was a problem hiding this comment.
You'd like to rename it to full_payments_were_processed_as_balances_were_sufficient().
| let owed_to_serving_node_2_minor = debt_threshold_wei + 456_789; | ||
| let owed_to_serving_node_3_minor = debt_threshold_wei + 789_012; | ||
| let consuming_node_initial_service_fee_balance_minor = debt_threshold_wei * 4; | ||
| let test_inputs = TestInputsBuilder::default() |
There was a problem hiding this comment.
You'd like to call it TestInput instead of TestInputs.
| fn activating_serving_nodes_for_test_with_sufficient_funds( | ||
| cluster: &mut MASQNodeCluster, | ||
| wholesome_values: &WholesomeConfig, | ||
| ) -> [MASQRealNode; 3] { |
There was a problem hiding this comment.
It turns out that we are firing too many nodes - 1 (consuming node) + 6 (stimulating consuming nodes) + 6 (serving nodes).
If we can manage with a lower number of nodes, it'll be better. 😉
| .into_iter() | ||
| .map(|balance_minor| Debt::new(balance_minor, quite_long_ago)) | ||
| .collect_vec(); | ||
| DebtsSpecs::new(debts[0], debts[1], debts[2]) |
There was a problem hiding this comment.
This function could take an array as an argument. (your idea)
| let owed_to_serv_node_1_minor = to_wei(payment_thresholds.debt_threshold_gwei + 5_000_000); | ||
| let owed_to_serv_node_2_minor = to_wei(payment_thresholds.debt_threshold_gwei + 20_000_000); | ||
| // Account of Node 3 will be a victim of tx fee insufficiency and will fall away, as its debt | ||
| // is the heaviest, implying the smallest weight evaluated and the last priority compared to |
There was a problem hiding this comment.
In this comment you want least instead of last (just a typo).
You'd also like to mention that due to insufficiency only 2 payments can be processed on the blockchain.
| let enough_balance_for_serving_node_1_and_2 = | ||
| owed_to_serv_node_1_minor + owed_to_serv_node_2_minor; | ||
| let consuming_node_initial_service_fee_balance_minor = | ||
| enough_balance_for_serving_node_1_and_2 - to_wei(2_345_678); |
There was a problem hiding this comment.
If you'd make a variable out of the subtracted value, maybe something like unpayable_portion.
| serving_node_2: serving_node_2_ui_port, | ||
| serving_node_3: serving_node_3_ui_port, | ||
| }), | ||
| const AFFORDABLE_PAYMENTS_COUNT: u128 = 2; |
There was a problem hiding this comment.
You may migrate this constant to the top, maybe along with a new constant for GAS_PRICE_MARGIN (only in this test). I think once you do that you might not need to introduce the comment for affordable payments.
| // It's important to prevent the blockchain server handle being dropped too early | ||
| let (mut cluster, global_values, _blockchain_server) = establish_test_frame(test_inputs); | ||
| let consuming_node = | ||
| global_values.prepare_consuming_node(&mut cluster, &global_values.blockchain_interfaces); |
There was a problem hiding this comment.
The second argument of this function is a reference to a field on global_values. Maybe it was intended or maybe it could be improved. Please take a look. 👀
| .borrow_mut() | ||
| .take() | ||
| .unwrap(), | ||
| ); |
There was a problem hiding this comment.
It was noticed during the review that wholesome_config.consuming_node.common is common in all the three arguments. I believe (and so do you) it is justified if we create a new variable called cn_common.
| test_inputs: TestInputs, | ||
| assertions_values: AssertionsValues, | ||
| stimulate_consuming_node_to_pay: StimulateConsumingNodePayments, | ||
| start_serving_nodes_and_activate_their_accountancy: StartServingNodesAndLetThemPerformReceivablesCheck, |
There was a problem hiding this comment.
Please make these names same to avoid any confusion. I believe it's no big deal, just an enhancement.
| consuming_node_initial_service_fee_balance_minor_opt: Option<u128>, | ||
| debts_config_opt: Option<DebtsSpecs>, | ||
| payment_thresholds_all_nodes_opt: Option<PaymentThresholds>, | ||
| consuming_node_gas_price_opt: Option<u64>, |
There was a problem hiding this comment.
You'd like to specify major in this field name. The new name will beconsuming_node_gas_price_major_opt.
| let mut ui_ports_as_opt = | ||
| ui_ports.serving_nodes.into_iter().map(Some).collect_vec(); | ||
| let serving_nodes_array: [Option<u16>; 3] = | ||
| core::array::from_fn(|_| ui_ports_as_opt.remove(0)); |
There was a problem hiding this comment.
Maybe you can call try_into.
No description provided.