[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());
    }

[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.

solana-program-library/token/program-2022/src/extension/transfer_fee/mod.rs

Lines 95 to 96 in c609163

	} else if transfer_fee_basis_points == ONE_IN_BASIS_POINTS || post_fee_amount == 0 {
	Some(0)

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

solana-program-library/token/program-2022/src/extension/transfer_fee/mod.rs

Lines 61 to 62 in c609163

	if transfer_fee_basis_points == 0 || pre_fee_amount == 0 {
	Some(0)

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:

solana-program-library/token/program-2022/src/extension/transfer_fee/mod.rs

Lines 98 to 100 in c609163

	let numerator = (post_fee_amount as u128).checked_mul(ONE_IN_BASIS_POINTS)?;
	let denominator = ONE_IN_BASIS_POINTS.checked_sub(transfer_fee_basis_points)?;
	let raw_pre_fee_amount = Self::ceil_div(numerator, denominator)?;

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.