token-cli: `set-transfer-fee` has different granularity than `create-token`

[joncinque]

As reported at anza-xyz/agave#2135:

Problem

set-transfer-fee works in whole tokens. create-token with "--transfer-fee" works in 10^(-decimals) tokens.

Proposed Solution

Either make them equal or document this more clearly. The way things are now is just asking for errors.

cc @SolBugz

[Hrushi20]

Hey! I'm interested in working on this issue. Can you give me more context so I can start working on it?

[joncinque]

@Hrushi20 that would be great! Essentially, the problem is that set-transfer-fee takes in the maximum fee as an amount at:

solana-program-library/token/cli/src/clap_app.rs

Lines 2253 to 2260 in 821c55b

	.arg(
	Arg::with_name("maximum_fee")
	.value_name("TOKEN_AMOUNT")
	.validator(is_amount)
	.takes_value(true)
	.required(true)
	.help("The new maximum transfer fee in UI amount"),
	)

And parsed at:

solana-program-library/token/cli/src/command.rs

Line 4339 in 821c55b

let maximum_fee = value_t_or_exit!(arg_matches, "maximum_fee", f64);

But when creating the token, the maximum fee is taken as a raw token amount:

solana-program-library/token/cli/src/clap_app.rs

Lines 760 to 770 in 821c55b

	.arg(
	Arg::with_name("transfer_fee")
	.long("transfer-fee")
	.value_names(&["FEE_IN_BASIS_POINTS", "MAXIMUM_FEE"])
	.takes_value(true)
	.number_of_values(2)
	.help(
	"Add a transfer fee to the mint. \
	The mint authority can set the fee and withdraw collected fees.",
	),
	)

and parsed at:

solana-program-library/token/cli/src/command.rs

Lines 3474 to 3485 in 821c55b

	let transfer_fee = arg_matches.values_of("transfer_fee").map(|mut v| {
	(
	v.next()
	.unwrap()
	.parse::<u16>()
	.unwrap_or_else(print_error_and_exit),
	v.next()
	.unwrap()
	.parse::<u64>()
	.unwrap_or_else(print_error_and_exit),
	)
	});

So we want that second case to take in a UI amount instead of a raw token amount. The problem becomes the deprecation process, since it would be bad if we just change how the --transfer-fee flag is interpreted. In that case, we might want to separate the argument into two: --transfer-fee-basis-points and --transfer-fee-maximum-fee, where --transfer-fee-maximum-fee accepts a UI amount, and make the new arguments require each other. After that, we can mark --transfer-fee as deprecated and hidden.

So the complete work would be:

• in create-token, add --transfer-fee-basis-points and --transfer-fee-maximum-fee flags
• mark --transfer-fee as hidden(true), and print a message saying that it's deprecated if it's used
• make --transfer-fee-basis-points require --transfer-fee-maximum-fee, and vice versa
• make it a failure to specify --transfer-fee and either --transfer-fee-basis-points or transfer-fee-maximum-fee

And I think that would do it! Let me know if you have any other questions

[Hrushi20]

Thanks for the detailed response. Almost done with implementation.

Quick question, what does make --transfer-fee-basis-points require --transfer-fee-maximum-fee, and vice versa mean?
Shouldn't only one of them be used i.e either --transfer-fee-maximum-fee or --transfer-fee-basis-point flag?

[joncinque]

That's great news! And both flags must be specified. You have a fraction of the fee (--transfer-fee-basis-points), and a ceiling (--transfer-fee-maximum-fee), as both values are currently set with the --transfer-fee flag. Now we want to have two separate flags, which means that there should be a failure if one is set without the other.

[SolBugz]

Thanks for working on this!

There should also be a failure if one attempts to set a maximum fee which isn't representable, e.g. 1.23456 if there are only 4 decimal places. But no failure if someone sets the maximum fee to something larger than fixed suppy. Ideally one would specify the amounts in tokens instead of minimum granularity units, i.e. 1.23456 instead of 123456.

"let maximum_fee = value_t_or_exit!(arg_matches, "maximum_fee", f64);" -> f64 doesn't have enough precision to represent a maximum-precision maximum fee (38.18 or whatever it is). Also be careful not to create a bug where rounding the result will push it outside the representable range on Solana, or wrap it to 0 or something bad. Rounding shouldn't be involved at all because this is fixed-point, but I assume you're stuck with f64 due to a design compromise affecting the entire codebase.