GH-711: review three

[bertllll]

No description provided.

[cursor]

You have run out of free Bugbot PR reviews for this billing cycle. This will reset on October 14.

To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

[utkarshg6]

You don't need to keep the word _for in the functions's name. Please eliminate it.

[utkarshg6]

This shorthand is not helping here, maybe even introducing jargon. I'd suggest if you use the name convert_qualified_payable_into_analyzed_payable, or if you want shorter than qualified_to_analyzed_payable.

[utkarshg6]

Instead of saying, "It looks like...", we can just direct the user by saying, "The user will have to resolve the issue manually."

[utkarshg6]

I think the word, meaningless is unnecessary, unless you have an alternate fn which says, meaningful, the prefix word make is enough to signify that it's meant for the test, and the developer should consider that it's fields were generated for testing purposes.

I mean this for both make_meaningless_unconfirmed_adjustment and make_meaningless_weighed_account.

[utkarshg6]

I've no idea why you decided to remove this... I thought you prefer stronger tests.

[utkarshg6]

Does it mean that these accounts are disqualified or qualified? I'd suggest you either add it as a comment, or possibly rename the fn to make it clear.

[utkarshg6]

What does these enum variants? Their name doesn't appear to be mutually exclusive. For example, if you're using Unexhausted, then shouldn't the other be Exhausted? Similarly, if it's Finalized, then shouldn't the other variant be Incomplete?

Since they aren't mutually exclusive, I find them very confusing.

[utkarshg6]

I'd prefer the older name. That's clearer.

[utkarshg6]

Can't it be simply "InsufficientBalance"?

[utkarshg6]

Nicely done! 👏

[utkarshg6]

Shouldn't this be LateServiceFeeErrorFactory instead? I'm implying to use the word Late instead of Later.

[utkarshg6]

I think this could be further refactored like this:

let fold_guts = |(mut above_limit, mut below_limit): (Vec<_>, Vec<_>), current: UnconfirmedAdjustment| {
    let disqualification_limit = current.disqualification_limit_minor();
    
    if current.proposed_adjusted_balance_minor >= disqualification_limit {
        diagnostics_for_accounts_above_disqualification_limit(&current, disqualification_limit);
        
        let adjusted = UnconfirmedAdjustment {
            proposed_adjusted_balance_minor: disqualification_limit,
            ..current
        };
        above_limit.push(adjusted);
    } else {
        below_limit.push(current);
    }
    
    (above_limit, below_limit)
};

[utkarshg6]

Remove this...

[utkarshg6]

Continue with node/src/accountant/payment_adjuster/non_unit_tests/mod.rs

[utkarshg6]

Bert, this explanation is not clear and concise.

Here's an output from ChatGPT that I found easy to understand. It took me few iterations to get the right output. I request you to pick an inspiration from this text and add it accordingly.

// This is a fuzz test for the PaymentAdjuster. It generates a large number of different 
// scenarios to see how well the PaymentAdjuster can handle various situations that might 
// occur in real life. The goal is to ensure it works reliably without causing problems in Node.

// The test has an exo-parameter, which is the number of scenarios to generate. This number 
// is flexible because many scenarios will be discarded during setup if they don’t meet 
// certain requirements, like whether the accounts involved are valid for payment adjustments.

// Some scenarios will be thrown out early if the PaymentAdjuster finds mistakes, ensuring 
// that only valid scenarios are tested. This helps identify corner cases that may not have 
// been considered during the design phase.

// When the test is finished, the results are saved in a file located at:
// node/generated/test/payment_adjuster/tests/home/loading_test_output.txt. This file 
// contains important numbers and statistics that show how well the PaymentAdjuster performed.

// The output may include confusing results, especially regarding percentages that indicate 
// how much of a debt is covered. These percentages might not match the account balances, 
// which can be tricky to understand. The adjustment depends on a concept called the 
// "disqualification limit," which affects how much of the total amount is required for 
// payment. This can lead to discrepancies in reported percentages.

// It’s important to interpret the results carefully. The way payments are adjusted can 
// depend on these limits, which may cause differences in the percentages reported for 
// different accounts.

[utkarshg6]

Let's just call these arguments as output_collector and results.

[utkarshg6]

Please use this refactored code:

    let output_collector_init = first_stage_output.output_collector;
    let (output_collector, results) = second_stage_scenarios.into_iter().fold(
        (output_collector_init, vec![]),
        |(collector, mut results), scenario| {
            let adjustment = scenario.prepared_adjustment;

            let account_info =
                preserve_account_infos(&adjustment.adjustment_analysis.accounts, now);
            let adjustment_type = adjustment.adjustment_analysis.adjustment.clone();
            let wallet_balance = adjustment.agent.service_fee_balance_minor();

            let result = subject.adjust_payments(adjustment);

            let (updated_collector, single_result) = administrate_single_scenario_result(
                collector,
                result,
                account_info,
                scenario.applied_thresholds,
                adjustment_type,
                wallet_balance,
            );

            results.push(single_result);
            (updated_collector, results)
        },
    );

    render_results_to_file_and_attempt_basic_assertions(
        results,
        number_of_requested_scenarios,
        output_collector,
    )

To reduce the number of arguments passed in the fn administrate_single_scenario_result you may use this new structure.

    
// Create a struct to group scenario data
struct ScenarioData {
    applied_thresholds: AppliedThresholds,
    adjustment_type: Adjustment,
    wallet_balance: u128,
    account_info: Vec<AccountInfo>,
}

// Then your function call becomes:
let scenario_data = ScenarioData {
    applied_thresholds: scenario.applied_thresholds,
    adjustment_type,
    wallet_balance,
    account_info,
};

let (updated_collector, single_result) = administrate_single_scenario_result(
    collector,
    result,
    scenario_data,
);

[utkarshg6]

The second sentence will be clearer if you write it like this:

I wanted to make sure that accounts with balances of different sizes are generated for a single scenario.

[utkarshg6]

Few grammatical changes:

based on the order by their weights -> based on the order of their weights
up their 100% allocation -> up to their 100% allocation
until there is any money that can be distributed -> until there is no money left to be distributed

[utkarshg6]

That's good!

[utkarshg6]

Optional (If you're too excited): I think this fn is too long, maybe refactor the if-else block to an outboard method.

[utkarshg6]

Remove these commented out lines.

[utkarshg6]

Some rephrasing for clarity:

"We haven't needed to worry about this matter" -> "We have not had to worry about this issue."

"yet this is rather a future alarm" should be "but this is rather a future alarm"

"whose forming takes place back out" should be "whose formation takes place outside."

[utkarshg6]

Why did you increase it by one?

[utkarshg6]

here too

[utkarshg6]

Continue with masq_lib/src/percentage.rs

[utkarshg6]

This part is hard to understand, why's that? Specifically the number 50, it's origin.

[utkarshg6]

Let's call it rounding_adjustment instead.

[utkarshg6]

I'm noticing a pattern here, maybe it's time to create a new structure for this, what about QuotientAndRemainder?

[utkarshg6]

Some grammatical corrections:

"cumulative percents of more than only 100" -> "cumulative percents greater than 100"

"requesting percents meaning possibly multiples of 100%" -> "requesting percents that may represent multiples of 100%"

"Similarly to the PurePercentage type, also signed numbers would be accepted." -> "Similar to the PurePercentage type, signed numbers would also be accepted."

[utkarshg6]

Continue at masq_lib/src/percentage.rs:232

[utkarshg6]

Instead of calling multiplier_of_100_percent you can simply call it quotient and degrees_from_remainder to remainder.

[utkarshg6]

You may just call these values for clearer expression:

multiples_of_100_percent -> quotient
remainder -> remainder_value
degrees_from_remainder -> remainder

[utkarshg6]

Over here, please reorder them such that the error types are together, and the core and type conversions are separate, even separating with the blank lines are sufficient or you can add comments and do something like this:

        // Core
        N: PercentageInteger + TryFrom<u32>,
        // Error
        <N as TryFrom<i8>>::Error: Debug,
        <N as TryFrom<u32>>::Error: Debug,
        <i16 as TryFrom<N>>::Error: Debug,
        // Reverse Conversion
        i16: TryFrom<N>,

Apparently, you could create a trait:

pub trait ExtendedPercentageInteger: 
    PercentageInteger + TryFrom<u32>
where
        // Error
        <Self as TryFrom<i8>>::Error: Debug,
        <Self as TryFrom<u32>>::Error: Debug,
        <i16 as TryFrom<Self>>::Error: Debug,
        // Reverse Conversion
        i16: TryFrom<Self>,
{
}

[utkarshg6]

Optional: You may save the result in the variable and then return that variable from this fn.

[utkarshg6]

Instead of the second sentence, you can directly provide usage examples:

    // - To increase by X%: use `LoosePercentage::new(100 + X)`
    // - To decrease by X%: use `LoosePercentage::new(100 - X)` where X is in range 1..=99

[utkarshg6]

Continue at masq_lib/src/percentage.rs:345

[utkarshg6]

Let's call this variable rounding_tolerance.

[utkarshg6]

Maybe just call it: half_of_input 🤷

[utkarshg6]

Similarly, you can do the same here.

[utkarshg6]

This is simple, yet so effective in explaining what's going on... nicely done @bertllll.

[utkarshg6]

This name is too long, prefer this ActivateServingNodesWithAccountancy or maybe NodeActivationHandler. Please change this name, for god's sake, that's toooooooooooo long.

[utkarshg6]

You can call this argument name as serving_nodes_activator.

[utkarshg6]

Please do the same here

[utkarshg6]

Continue at multinode_integration_tests/tests/verify_bill_payment_utils/utils.rs:282

[utkarshg6]

Code in this file is well written. 👏

[utkarshg6]

You can make the arg name singular too for consistency.

[utkarshg6]

Finished! 👏