GH-711: Completion of fundamental Payment Adjuster and its peripheries

[bertllll]

No description provided.

[utkarshg6]

node/src/accountant/payment_adjuster/miscellaneous/account_stages_conversions.rs:26

[utkarshg6]

If we use Address instead of Wallet, we would end up optimising it further.

[utkarshg6]

It seems that it could be overwhelming to user to see so many info notifications for each account. Ideally, it should be a summary along the lines, "X accounts haven't been paid yet, due to low MASQ balance."

[utkarshg6]

Let's mention how much balance the user needs to maintain to pass the tx fee barrier. Similar with the above service fee.

[utkarshg6]

It would be good to review the comments in this file to make them more explanatory.

[utkarshg6]

It's just the disqualification limit and not the full balance

[bertllll]

test name inconsistent with those others

[bertllll]

It might look better if that 111_222_333 was assigned to a variable with a self-explaining name.

[utkarshg6]

node/src/accountant/payment_adjuster/adjustment_runners.rs:118

[utkarshg6]

We can change it to if let.

[utkarshg6]

Let's use the name max_balance_over_threshold_of_qualified_payables.

[utkarshg6]

:)

[utkarshg6]

node/src/accountant/payment_adjuster/logging_and_diagnostics/diagnostics.rs:103

[utkarshg6]

Let's use arrays for payables and expected balance with 2 elements instead of arguments.

[utkarshg6]

Instead of using it as a trait, it could be used as an enum.

It would make an impact on the other function:

    fn propose_adjustments_recursively<AR, RT>(
        &mut self,
        unresolved_accounts: Vec<WeightedPayable>,
        adjustment_runner: AR, // It would become an enum
    ) -> RT
    where
        AR: AdjustmentRunner<ReturnType = RT>,
    {
        diagnostics!(
            "\nUNRESOLVED QUALIFIED ACCOUNTS IN CURRENT ITERATION:",
            &unresolved_accounts
        );

        adjustment_runner.adjust_accounts(self, unresolved_accounts) // we would make it a method of self and will pass the variant
    }

[utkarshg6]

You want make_weighted_payable, the t is missing.

[utkarshg6]

A suggested new name:

fn untaken_cw_balance_equals_full_two_debts_after_loosing_an_account_results_in_constrained_balances()

Note: We had a nice discussion and it turns out you'd prefer untaken over unallocated. I think that's a great change.

[utkarshg6]

Once the test rename is done, please rename this variable to include untaken.

[utkarshg6]

Add a comment here to say that the limit is set here. Maybe you want to add more explanation.

[utkarshg6]

It's consuming wallet balance.

@bertllll was puzzled by this number and it was enough to make the test pass.

[utkarshg6]

Let's call it tx_fee_adjustment

[utkarshg6]

You can create the weighted_accounts in the beginning of the test.

[utkarshg6]

You can say RUN_DIAGNOSTICS.

[utkarshg6]

node/src/accountant/payment_adjuster/disqualification_arbiter.rs:383

[utkarshg6]

If you store an Address here (which implements Copy), can help you get rid of unnecessary lifetimes.

[utkarshg6]

A better alternative to fold() is min_by_key().

https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.min_by_key

In case min_by_key fails to serve you, you can use min_by - https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.min_by

[utkarshg6]

Maybe move it to the file which contains all the from implementations.

[utkarshg6]

Returning permanent_debt_allowed_minor is better.

[utkarshg6]

If you'd merge the if condition with the function qualifies_for_double_margin (and rename it too), then you can return the required u128.

[utkarshg6]

It's better if we make this condition lighter, by changing the constant's value to 1.

[utkarshg6]

If you can add the diagrams using a tool which converts "diagram into code comments", that would make the logic easier to understand.

[utkarshg6]

Continue at node/src/accountant/payment_adjuster/miscellaneous/helper_functions.rs:120

[utkarshg6]

Let's assert on the exact proposed balance. It's preferable if you sort it and you directly put the numbers in the assert.

[utkarshg6]

Let's rename it to accounts

[utkarshg6]

If you'd call this field decided_accounts_opt and also make it an Option. Then we can safely eliminate the type DecidedAccounts.

[utkarshg6]

At the first glance, By16bits seemed like a mandatory constrain, since it's not, we decided to keep the name TransactionCounts.

[utkarshg6]

Let's collect this vector first inside a variable and then use it to calculate result.

[utkarshg6]

If you can make it more generic and it doesn't care about u128 it'll be even better.

[utkarshg6]

You'd prefer to use the guts directly instead of this wrapper function.

[utkarshg6]

You want to use descending instead.

[utkarshg6]

You may like to use the max() instead.

[utkarshg6]

You can call it cw_remaining.

[utkarshg6]

Continue at node/src/accountant/payment_adjuster/mod.rs:87

[utkarshg6]

Instead of add(), let's call it append(), and also for the handle fn, we can say update_balance_and_append().

If we split the duties of the handle_balance_update_and_add, such that we only update the balance inside the if clause and append the account to the vector outside the if-else, it would be better.

[utkarshg6]

We decided to call this remaining_cw_balance in the other place, so let's use that here too.

[utkarshg6]

The word found in the test name is an extra.

[utkarshg6]

Here too.

[utkarshg6]

Also here 😢

[utkarshg6]

A suggested name: proposed_balance_refills_to_original_balance_for_all_three_non_exhaustive_accounts.

[utkarshg6]

Instead of In the end, we have, you'd like to say Therefore, we have.

[utkarshg6]

You'd like to call it consider_adjustment. That's an exceptionally simple name and I love it.

[utkarshg6]

You wanted to give an alias to Vec<QualifiedPayableAccount>. You may use the name UnadjustedAccounts.

[utkarshg6]

If you don't need to keep this anymore, go for it.

[utkarshg6]

Continue at node/src/accountant/payment_adjuster/mod.rs:203

[utkarshg6]

There was a point to rename this, if you do rename it, reuse it.

PS: I've made peace with this name, I'd prefer if you'd not change it at all.

[utkarshg6]

If we wrap the inner with a Refcell, we won't need to use &mut self, we can use &self. Since, this function returns something it's ideal.

[utkarshg6]

We don't want to confuse info with the log level here, so prefer a rename sketch_debug_log_opt. We want to do the same on line 133 with the function complete_debug_info_if_enabled().

Also, don't forget to rename the variable name.

[utkarshg6]

Let's call it Adjustment::BeginByTxFee

[utkarshg6]

Please take a look into the code to identify if we can keep either the enum Adjustment or this struct. They both are differentiating between the same thing but resulting in different combinations.

A proposed idea was to not use this TransactionAndServiceFeeAdjustmentRunner {} at all and instead use a boolean (checking if it's an initial call) with the old enum. You might need to use an Either.

[utkarshg6]

Continue at node/src/accountant/payment_adjuster/mod.rs:277

[utkarshg6]

This will go away.

Have a nice day! 👨‍💻👽

[utkarshg6]

Let's call it weighted_accounts.

[utkarshg6]

Let's call it transaction_count_limit.

[utkarshg6]

Certain suggestions to names here:

    fn make(
        &self,
        number_of_accounts: usize, // accounts_allowed_by_tx_fee_check
        _required_service_fee_total: u128,
        cw_service_fee_balance_minor: u128,
    ) -> PaymentAdjusterError {
        PaymentAdjusterError::LateNotEnoughFeeForSingleTransaction {
            // InsufficientServiceFeePostTxFeeAdjustment
            original_number_of_accounts: self.original_number_of_accounts,
            number_of_accounts,
            original_service_fee_required_total_minor: self
                .original_service_fee_required_total_minor,
            cw_service_fee_balance_minor,
        }
    }

[utkarshg6]

We may use the name:

eliminate_accounts_by_tx_fee_limit()

[utkarshg6]

A suggestion:

result_before_exhausting_cw_balance

[utkarshg6]

This is a vague comment. I apologise for that.

It's here because I feel the need to refactor this function such that the recursive calls are visible inside the scope of the function.

If the function follows this criteria:

fn recursive_fn() -> Something {
   if breaking_condition {
       return Something;
   } else {
       recursive_fn()
   }
}

[utkarshg6]

A bit of refactoring got us here, I hope you'll find it useful during your changes:

    fn propose_possible_adjustment_recursively_new(
        &mut self,
        weighed_accounts: Vec<WeightedPayable>,
    ) -> Vec<AdjustedAccountBeforeFinalization> {
        let disqualification_arbiter = &self.disqualification_arbiter;
        let unallocated_cw_service_fee_balance =
            self.inner.unallocated_cw_service_fee_balance_minor();
        let logger = &self.logger;

        let current_iteration_result = self.service_fee_adjuster.perform_adjustment_by_service_fee(
            weighed_accounts,
            disqualification_arbiter,
            unallocated_cw_service_fee_balance,
            logger,
        );

        let remaining_undecided_accounts = current_iteration_result.remaining_undecided_accounts;
        let decided_accounts: Vec<AdjustedAccountBeforeFinalization> =
            current_iteration_result.decided_accounts.into();
        if remaining_undecided_accounts.is_empty() {
            return decided_accounts;
        }

        if !decided_accounts.is_empty() {
            self.adjust_remaining_unallocated_cw_balance_down(&decided_accounts)
        }

        let merged = if self.is_cw_balance_under_limit(&remaining_undecided_accounts) {
            // Fast return after a direct conversion into the expected type
            Self::add_decided_accounts(
                convert_collection(remaining_undecided_accounts),
                decided_accounts,
            )
        } else {
            Self::add_decided_accounts(
                self.propose_possible_adjustment_recursively(remaining_undecided_accounts),
                decided_accounts,
            )
        };

        diagnostics!(
            "\nFINAL SET OF ADJUSTED ACCOUNTS IN CURRENT ITERATION:",
            &merged
        );

        merged
    }

[utkarshg6]

Continue at node/src/accountant/payment_adjuster/mod.rs:422

[utkarshg6]

I hope you'll use the Address here in case of the Wallet, it'll prevent clone().

[utkarshg6]

You may call it fully_processed_accounts instead.

[utkarshg6]

Continue at node/src/accountant/payment_adjuster/mod.rs:807

[utkarshg6]

You mentioned that the , in this sentence is wrong.

[utkarshg6]

Let's change the panic message to say:

The PaymentAdjuster Inner is uninitialised. It was detected while executing unallocated_cw_service_fee_balance_minor().

Also, inside the test, you want to replace result with subject.

[utkarshg6]

The test name is different.

[utkarshg6]

The u128 seems unnecessary here. There's a suggestion to use the _u64 with the numbers that are provided as arguments.

[utkarshg6]

This should be gas_price.

[utkarshg6]

You mentioned that you'd prefer ( here to isolate the - 1.

[utkarshg6]

The analyzed_payables should be declared right above this assertion.

[utkarshg6]

One more time, we can use the _u64.

[utkarshg6]

You want to keep analysed_payables above this assertion.

[utkarshg6]

Maybe you want throws_err_for_tx_fee_but_service_fee_balance_is_fine()

[utkarshg6]

Continue at node/src/accountant/payment_adjuster/mod.rs:1045

[utkarshg6]

We're expecting 3 accounts, and the balance here is for 1 account. Maybe None is good enough.

[utkarshg6]

Let's make it closer to the edge with (55_000 * 100) - 1.

[utkarshg6]

This is noticed in different tests, and you'd want to change it everywhere in the codebase. The idea is to pass the type along with the numerals.

Let's rename TRANSACTION_FEE_MARGIN -> TX_FEE_MARGIN_IN_PERCENT.

[utkarshg6]

You want to revisit how the disqualification limit is picked. You should iterate through qualified_payables and choose some value for each. Make sure that PaymentThresholdIntercept == PermanentDebtAllowed. Let's add a comment too. 😉

[utkarshg6]

Another Idea would be to create a constant:

const BILLION = 1_000_000_000;

[utkarshg6]

Let's create a card for this (not a high priority, since it's just cosmetics). The intention is to use a better method to display these numbers such that a higher unit is picked, such that the number can be represented using multiple zeros.

[utkarshg6]

Let's call the error: RecursionDrainedAllAccounts.

And the string to: "The payment adjuster wasn't able to compose any combination of payables that can be paid immediately with provided finances."

[utkarshg6]

We want to discuss the name change or the test removal.

In case name change is picked, then a suggestion: account_with_larger_weight_is_prioritized_and_gains_up_to_its_disqualification_limit().

[utkarshg6]

Continue at node/src/accountant/payment_adjuster/mod.rs:1194

[utkarshg6]

It would be better if you create variables for weights here and you can supply them on line 1077.

[utkarshg6]

Optional: I think this could be done using a Builder Pattern. It'll prevent us from supplying None values.

[utkarshg6]

The whole gauge can go away.

[utkarshg6]

A new name suggestion:

illustrate_that_we_need_to_prevent_exceeding_the_original_value

[utkarshg6]

You don't want to use the before additional criterion. Instead you'd prefer due to an additional criterion used to calculate the sum.

[utkarshg6]

A modified version:

Later it was changed to a different policy, so called "outweighed" account gains automatically a balance equal to its disqualification limit. Still, later on it's very likely to be given a bit more from the remains languishing in the consuming wallet.

[utkarshg6]

Apparently, we want this value to be equal to the disqualification_limit. All you need to do is to use the variable disqualification_limit_1 here.

[utkarshg6]

It would be better if you explain the disqualification process using a comment.

The way you explained this test to me verbally was more explanatory than the current state of the test (along with the comments), so please add them.

Note: While restating or removing and replacing the comment below with a new one, it would help a lot if we keep it more specific that which account outplays which one (at what stage) and the reason behind it.

[utkarshg6]

Continue at node/src/accountant/payment_adjuster/mod.rs:1258

[utkarshg6]

Another explanation for how this mock is being differently.

[utkarshg6]

A typo here: You want multiply.

[utkarshg6]

At the moment, it seems doable to use the old agent. Also, we're not even asserting on the id stamp, since all we want to look at is the error.

[utkarshg6]

Continue at node/src/accountant/payment_adjuster/mod.rs: 1664

[utkarshg6]

I think the timestamps should be the same, when I read the test, I thought these will cause some side effects that you want and think are necessary for the test. On the other hand, it just adds more confusion when figuring out the prioirity of the accounts.

I'd suggest you also create the variables for weights together with the accounts.

[utkarshg6]

You'd like to replace the real calculator with the mock.

You should try to refactor the test such that it is more explicit which account has the lowest priority. Also, you'd like to check how the values that are being generated for disqualification limit. So, you might want to hardcode them instead. Feel free to customise it according to your needs.

You want to check what happens if you remove the condition, this test is meant for. You'd like to change the act, so that it could be closer to the level where condition lives.

[utkarshg6]

You'd want to see it like this:

let service_fee_balance_in_minor_units = balance_2 + balance_3 + ((balance_1 * 10) / 100);

[utkarshg6]

Instead of exaggerated_debt_conditions we can say impossible_debt_sizes.

[utkarshg6]

You'd like to add minor in the name of the variable.

[utkarshg6]

You want a line of demarcation here to separate the tests which have a happy path.

[utkarshg6]

To me it is quite appealing that balance_1 + balance_3 = 2 * balance_2.

I'd be spending a lot of time figuring out the relation if I'd be reading this test on my own. If this pattern is meaningless to you, then maybe eliminate it.

[utkarshg6]

An amazing idea would be if you pass the disqualification_limit instead of threshold_intercept_major and permanent_debt_allowed_major, and you'll compute these values inside the function.

[utkarshg6]

That's really cool. 🎉

[utkarshg6]

Let's create a variable for the count somewhere during the preparatory statements of the test.

[utkarshg6]

Continue at node/src/accountant/payment_adjuster/mod.rs:1917

[utkarshg6]

Let's use these points as comments for the balances. I hope it'll be easier to figure out.

1. Second Highest Weight, Partially Paid
2. Highest Weight, Fully Paid
3. Third Highest Weight, Disqualified

Note: Since you'd be moving the weights to preparatory statements in the other tests, you'd prefer to do the same to this test.

[utkarshg6]

Some function like:

let balance_1 = to_quintillion(100);

[utkarshg6]

A new test name suggestion:

late_error_after_tx_fee_adjusted_but_rechecked_service_fee_found_fatally_insufficient

[utkarshg6]

You don't need to calculate it by using the arbiter, instead you can take it out of the account.

[utkarshg6]

You should revisit the declaration of the agent, to make it use debug. It would allow us to use unwrap_err() here.

[utkarshg6]

If we can only use one unit, that'll be better, maybe Wei, because precision is important.

[utkarshg6]

We can change agreed_transaction_fee_per_computed_unit_major -> gas_price and estimated_transaction_fee_units_per_transaction -> tx_computation_units

Note: It's preferable if the rename can be done globally.

[utkarshg6]

If we can just call them:

service_fee_config_opt and tx_fee_config_opt

It'll be symmetric.

[utkarshg6]

Continue at node/src/accountant/payment_adjuster/mod.rs:2060

[utkarshg6]

If you implement the Default for TestConfigForServiceFeeBalances. Then we could use the default() directly inside the tests, and there won't be need of this function.

[utkarshg6]

I think inside the tests, we can do something like this.

let config = config_opt.unwrap_or(default_tx_fee_config(accounts_count));

[utkarshg6]

You might like to refactor it using a new function:

fn prepare_payable_accounts_from(Either<Vec<u128>, u64>);

You may call this new function in both of the scopes of if-else statement.

[utkarshg6]

You mentioned, it would be better if we use prepare_agent instead of make_agent.

[utkarshg6]

The two changes that we need here:

• The min and max should be different (97 and 103)
• We can assert for not in the range by using assert_ne!(min <= previous_result && previous_result <= max);

[utkarshg6]

node/src/accountant/payment_adjuster/mod.rs:2191

[utkarshg6]

Maybe here you can say as a comment, "Test Case for only 1st Calculator"

[utkarshg6]

You want to make this sentence more explicit, "you need to make an array of inputs" -> "you need to make a 2-element array"

[utkarshg6]

If you prefer to convert this into a struct, I think it's good.

struct CalculatorTestScenario {
   account: QualifiedPayableAccount,
   expected_weight: u128
}

type InputMatrixConfigurator = fn([QualifiedPayableAccount; 2], SystemTime) -> Vec<[CalculatorTestScenario; 2]>

[utkarshg6]

You want to add a comment here:

// Your new scenario for a unique calculator will live here

[utkarshg6]

You are right, I'd like Address here instead of Wallet.

[utkarshg6]

There was an idea that you mentioned that you'd like to make nominal_weight a constant. I think it's a great idea. Let's do that.

[utkarshg6]

Since you're asserting on equality this statement will make sense when the length of matrix is less than the calculators_count but if it's greater, the statement will loose it meaning.

Either change the assertion or the message according to your convenience.

[utkarshg6]

You'd like to rename this to prepare_inputs_with_expected_weights.

[utkarshg6]

Let's call this actual_weighted_accounts.

[utkarshg6]

Also here, actual_weighted_accounts.

[utkarshg6]

If you can combine the trait, it'll be more concise:

So, if you think generic name is the only limitation, then this is the name we both came up with: AccountWithBalanceAndDisqLimit

[utkarshg6]

provide_account_with_disqualification_limit is a new name suggestion by @bertllll

[utkarshg6]

You want comes instead of coma.

[utkarshg6]

If you have some ideas to improve it, go ahead and do it...

[utkarshg6]

Let's call them accounts_above_disq_limit and accounts_below_disq_limit.

[utkarshg6]

Since there is no field stored in this struct (and in case we plan not to add it in a near future), I'd suggest, we convert this function implemented in the impl block into a pure function.

[utkarshg6]

I'd prefer if we limit the balance to the disqualification limit inside this function here.

If you want to keep the calculations for historical reasons, maybe prefer printing the diagnostics inside this function with both the over-computed and amended values.

[utkarshg6]

You'd like to use the new to_quintillion() function here.

[utkarshg6]

I think it'd be better if we use the builder method here.

[utkarshg6]

Instead multiply by a billion.

[utkarshg6]

You'd like to change it to gas_price.

[utkarshg6]

If you'd provide a number here, it's appreciated.

[utkarshg6]

Continue from node/src/accountant/payment_adjuster/non_unit_tests/mod.rs:295

[utkarshg6]

You told that generating the age from starting a number fairly different from 1 isn't right and instead you should start from maturity_threshold_sec.

Instead of dividing by 2, you'd prefer to multiply it with 2/3 or 3/4.

[utkarshg6]

Continue at node/src/accountant/payment_adjuster/non_unit_tests/mod.rs:628

[utkarshg6]

Please add the location of the generated file:

node/generated/test/payment_adjuster/tests/home/loading_test_output.txt

Regarding adding a library, I don't want you to get in trouble with that. It might not produces anything substantial that you're happy with.

[utkarshg6]

Computing the service fee balance raises a lot of questions. So a comment could be helpful here explaining that it basically provides different magnitudes in the produced numbers.

[utkarshg6]

You'd like to move this fn closer to make_payable_account.

[utkarshg6]

Adding an explanation of X would be a good idea.

[utkarshg6]

The numbers are picked by experimenting different values. (Empirical Values)

[utkarshg6]

Using Address will be more efficient.

[utkarshg6]

For consistency, let's use the same name as the variable by using shadowing.

[utkarshg6]

If you could make a method of this structure, it would be better.

[utkarshg6]

You may include the word tail here.

[utkarshg6]

Let's replace rate with percent here and also in the required_pass_rate. Maybe all the places.

[utkarshg6]

Continue at node/src/accountant/payment_adjuster/preparatory_analyser/mod.rs

[utkarshg6]

Maybe you'd like to shorten the names with scenarios_requated and scenarios_evaluated.

[utkarshg6]

Maybe you'd like to just say, no_accounts_eliminated.

[utkarshg6]

Let's use matches!() here too, for symmetry. - @bertllll

[utkarshg6]

I found txt a bit confusing, so please change it to tx wherever possible.

Another suggestion by @bertllll - For symmetry add indentation so that all the data is in a column.

[utkarshg6]

These two fns can be converted into methods.

[utkarshg6]

If the relation in HashMap is Address -> PaymentThresholds, it would be a bit more efficient.

[utkarshg6]

Instead of ending_status it could be fulfilment_status.

[utkarshg6]

It's hard to read. Please add the comment explaining that:

The priority order will be such that - Percentage Descending, Service Fee Ascending

[utkarshg6]

Descending order would be more appropriate here.

[utkarshg6]

Let's call it switch_to_hashmap_if_needed - @bertllll

[utkarshg6]

Continue at node/src/accountant/payment_adjuster/preparatory_analyser/mod.rs:535

[utkarshg6]

Let's call it gas_price.

[utkarshg6]

I think if we create an alias here to identify ConstrainedTxCount, it would make code more readable.

[utkarshg6]

Perhaps you could pass the prepared_accounts in the make() and would perform the computations there.

[utkarshg6]

A different name: make_mock_results_doubled (suggested by @bertllll )

[utkarshg6]

If it could be migrated into a function, go ahead! 🫡

[utkarshg6]

Another suggestion by @bertllll - prepare_expected_error.

[utkarshg6]

Continue at node/src/accountant/scanners/mod.rs:325

[utkarshg6]

A new name by @bertllll -

least_demanding_account_error_in_pre_adjustment_stage

[utkarshg6]

Maybe a better name

[utkarshg6]

Let's rename the act here to say,prepare_expected_error.

Please fix it in the previous test too.

• @bertllll

[utkarshg6]

Here also, you want to start with just the least_demanding_account_...

[utkarshg6]

A small typo here - make_weighted_account

[utkarshg6]

Let's add a comment explaining why we're adding 1 here.

[utkarshg6]

Let's call the variable popped_results

2 -> len_of_queue
4 -> 2 * len_of_queue

[utkarshg6]

Let's use ChatGPT to reword into a single sentence.

[utkarshg6]

Finally, it's done! 👏

You don't know how happy I am.

Good Luck with the changes, I wish you'll enjoy the process.

[utkarshg6]

I'm particularly not fond of the idea to waste your time on asking you to refactor this change but if you'd like to spare some time on this function then please take a look whether we can construct PayableInspector with PaymentThresholds so that we don't need to pass a reference here.

[utkarshg6]

Let's revert it back to it's original name here.

[utkarshg6]

Can you please call this function to make_qualified_and_unqualified_payables because the first argument of the returned tuple is qualified payables.

[utkarshg6]

Two changes:

1. Let's append _opt in the name of the factory.
2. You can convert the argument of this function to Either so that it accepts only one.

[utkarshg6]

Let's provide a Mock instead.

[utkarshg6]

If you create a Type for the Either part, then don't forget to modify the code in the mocks too.

[utkarshg6]

// It's not guaranteed that the payables would cross the given thresholds.

[utkarshg6]

You shouldn't be hardcoding the creditor thresholds ans instead you should be using the n.

[utkarshg6]

Here too, use the n for the second argument.