[transfer-fee] calculate_inverse_fee logic error when transfer_fee_basis_points is MAX_FEE_BASIS_POINTS

[0x777A]

when transfer_fee_basis_points is MAX_FEE_BASIS_POINTS, the function should return the maximum fee. Otherwise, the following test will fail.

    #[test]
    fn calculate_fee_exact_out_max11() {
        let transfer_fee = TransferFee {
            epoch: PodU64::from(0),
            maximum_fee: PodU64::from(5_000),
            transfer_fee_basis_points: PodU16::from(MAX_FEE_BASIS_POINTS),
        };

        assert_eq!(
            5_000,
            transfer_fee.calculate_inverse_fee(u64::MAX / 2).unwrap()
        );

        assert_eq!(5_000, transfer_fee.calculate_inverse_fee(1).unwrap());
    }

[metamania01]

Incorrect Function Return Values for Two Specific Scenarios

Issue Description

The function in question is returning 0 for two distinct scenarios where this output is not expected or correct. The scenarios and the suggested corrections are as follows:

1. Scenario 1: post_fee_amount Greater Than 0

   When post_fee_amount is greater than 0, it implies that transfer_fee must have been equal to max_fee_amount. To rectify the returned value, max_fee_amount should be added back to obtain the pre_fee_amount. This adjustment will yield the correct output and allow the function to proceed as intended.

2. Scenario 2: post_fee_amount Equals 0

   In this scenario, the amount could have ranged anywhere between 0 and max_fee_amount. To address this, it is proposed that the function should always return max_fee_amount. This approach will ensure a more accurate representation of the possible fee range when post_fee_amount is 0.

Suggested Changes

• For Scenario 1: Add max_fee_amount back to the output when post_fee_amount > 0.
• For Scenario 2: Return max_fee_amount when post_fee_amount is 0.

[buffalojoec]

This appears to be intentional behavior. When calculating the inverse_fee, the pre_fee_amount resolves to 0 automatically when transfer_fee_basis_points is MAX_FEE_BASIS_POINTS.

https://github.com/solana-labs/solana-program-library/blob/c609163f6503f65f2b9e4e2a4dc24e0c15d91407/token/program-2022/src/extension/transfer_fee/mod.rs#L95-L96

Then of course that resolved 0 will yield another 0 from calculate_fee.

https://github.com/solana-labs/solana-program-library/blob/c609163f6503f65f2b9e4e2a4dc24e0c15d91407/token/program-2022/src/extension/transfer_fee/mod.rs#L61-L62

Are you sure calculate_inverse_fee is what you want here, and not calculate_fee?

[metamania01]

It's still wrong because it cannot be the case that the fee is always 0 for ONE_IN_BASIS_POINTS.

Imagine a token that has the rate set as ONE_IN_BASIS_POINTS and also has max_fee set, the inverse_fee should be max_fee in this case.

[buffalojoec]

@metamania01 It's not that the fee is 0, it's that it can't be calculated by calculate_inverse_fee if the basis points are 100%.

MAX_BASIS_POINTS == ONE_IN_BASIS_POINTS == 100%.

You'd end up dividing by zero for the pre fee:

https://github.com/solana-labs/solana-program-library/blob/c609163f6503f65f2b9e4e2a4dc24e0c15d91407/token/program-2022/src/extension/transfer_fee/mod.rs#L98-L100

It seems you're suggesting if transfer_fee_basis_points == ONE_IN_BASIS_POINTS it should in fact return Some(maximum_fee as u128)?

cc @joncinque @samkim-crypto

[joncinque]

This is a good point, thanks for bringing it up! we were a bit lazy in bailing out if the fee is 100%.

You're definitely right, so let's go through the situations:

• if post_fee_amount is greater than 0, and the fee is 100%, then the inverse fee needs to be the maximum fee
• if post_fee_amount is 0, and the fee is 100%, what do we say? The fee could have been anything between 1 and maximum fee. Should we return an error?

Keep in mind that the maximum fee is important here, however. If it's set to u64::MAX - 1, and the post_fee_amount is 100, then we'll need to return an error since there's no solution. I believe that should be covered by the existing code.

What do you think about all this?

[samkim-crypto]

I think generally, these functions are not perfect because the solution is not really well-defined in some edge cases.

• if post_fee_amount is 0, and the fee is 100%, what do we say? The fee could have been anything between 1 and maximum fee. Should we return an error?

It seems like the fee could have also been 0 in this case since the pre_fee_amount could have been 0, independent of the maximum fee?

I think the question is what to do when there are multiple solutions possible. For calculate_pre_fee_amount, we chose to always return the smallest possible pre_fee_amount. If I want to transfer an amount to a destination account so that the destination account receives a certain amount after fees are deducted, then I can use this function to calculate the exact amount I need to transfer from the perspective of the sender. In this case, I do not want to overpay, so it makes sense to choose the smallest amount.

So if we add this special case to calculate_inverse_fee:

• if post_fee_amount is greater than 0, and the fee is 100%, then the inverse fee needs to be the maximum fee

then things are consistent in that both calculate_pre_fee_amount and calculate_inverse_fee returns the smallest possible number. If we update calculate_inverse_fee to return an error when post_fee_amount is 0 and the fee is 100%, then it seems like we should also update calculate_pre_fee_amount to return errors on undetermined outputs for consistency.