Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unexpected behavior of createInitializeTransferFeeConfigInstruction #6113

Closed
ngtanvo opened this issue Jan 11, 2024 · 3 comments · Fixed by #6164
Closed

Unexpected behavior of createInitializeTransferFeeConfigInstruction #6113

ngtanvo opened this issue Jan 11, 2024 · 3 comments · Fixed by #6164
Labels
bug Something isn't working good first issue Good for newcomers javascript

Comments

@ngtanvo
Copy link

ngtanvo commented Jan 11, 2024

When using createInitializeTransferFeeConfigInstruction with transferFeeConfigAuthority set to null but with a fee and/or maxFee defined:

Expected behavior: Either the TX fails, because you're trying to set a fee and there's no relevant authority -- OR -- the fee is set, but cannot be further changed.

Actual behavior: The TX containing this instruction succeeds but fee and maxFee are set to 0.

Tested on DevNet. Specifying any public key (e.g. with unknown private key) makes it correctly set the fee and maxFee.

According to the documentation here, transferFeeConfigAuthority should accept either a PublicKey or null:

https://solana-labs.github.io/solana-program-library/token/js/functions/createInitializeTransferFeeConfigInstruction.html

I've also found that setting maxFee to undefined sets it to zero. Not a bug -- the docs say it must be a BigInt. Maybe more intuitive if it returns an error if undefined?

@ghost

This comment was marked as spam.

@buffalojoec
Copy link
Contributor

buffalojoec commented Jan 18, 2024

Is this accurately reproducing your issue?

it('initialize with null authority', async () => {
    const mintAuthority = Keypair.generate();
    const mintKeypair = Keypair.generate();
    mint = mintKeypair.publicKey;
    const mintLen = getMintLen(MINT_EXTENSIONS);
    const mintLamports = await connection.getMinimumBalanceForRentExemption(mintLen);
    const mintTransaction = new Transaction().add(
        SystemProgram.createAccount({
            fromPubkey: payer.publicKey,
            newAccountPubkey: mint,
            space: mintLen,
            lamports: mintLamports,
            programId: TEST_PROGRAM_ID,
        }),
        createInitializeTransferFeeConfigInstruction(
            mint,
            null, // Authority set to null on initialization
            withdrawWithheldAuthority.publicKey,
            FEE_BASIS_POINTS,
            MAX_FEE,
            TEST_PROGRAM_ID
        ),
        createInitializeMintInstruction(mint, TEST_TOKEN_DECIMALS, mintAuthority.publicKey, null, TEST_PROGRAM_ID)
    );
    await sendAndConfirmTransaction(connection, mintTransaction, [payer, mintKeypair], undefined);

    // Fees should match
    const mintInfo = await getMint(connection, mint, undefined, TEST_PROGRAM_ID);
    const transferFeeConfig = getTransferFeeConfig(mintInfo);
    expect(transferFeeConfig).to.not.be.null;
    if (transferFeeConfig !== null) {
        expect(transferFeeConfig.olderTransferFee.transferFeeBasisPoints).to.eql(0); // Should be `FEE_BASIS_POINTS`
        expect(transferFeeConfig.olderTransferFee.maximumFee).to.eql(0n); // Should be `MAX_FEE`
        expect(transferFeeConfig.newerTransferFee.transferFeeBasisPoints).to.eql(0); // Should be `FEE_BASIS_POINTS`
        expect(transferFeeConfig.newerTransferFee.maximumFee).to.eql(0n); // Should be `MAX_FEE`
    }
});

If so, it appears you're correct and I can re-produce the issue. This test passes with all those to.eq(0) checks, when the transfer fees should in fact be the values I've set in the instruction.

If I write the same test in Rust:

#[tokio::test]
async fn success_initialize_fee_with_null_authority() {
    let TransferFeeConfigWithKeypairs {
        withdraw_withheld_authority,
        transfer_fee_config,
        ..
    } = test_transfer_fee_config_with_keypairs();
    let mut context = TestContext::new().await;
    let transfer_fee_basis_points = u16::from(
        transfer_fee_config
            .newer_transfer_fee
            .transfer_fee_basis_points,
    );
    let maximum_fee = u64::from(transfer_fee_config.newer_transfer_fee.maximum_fee);
    context
        .init_token_with_freezing_mint(vec![ExtensionInitializationParams::TransferFeeConfig {
            transfer_fee_config_authority: None,
            withdraw_withheld_authority: withdraw_withheld_authority.pubkey().into(),
            transfer_fee_basis_points,
            maximum_fee,
        }])
        .await
        .unwrap();
    let TokenContext {
        token,
        ..
    } = context.token_context.take().unwrap();

    // The fees should be the values we set
    let state = token.get_mint_info().await.unwrap();
    let extension = state.get_extension::<TransferFeeConfig>().unwrap();
    assert_eq!(
        extension.transfer_fee_config_authority,
        Option::<Pubkey>::None.try_into().unwrap()
    );
    assert_eq!(
        extension.older_transfer_fee.transfer_fee_basis_points,
        transfer_fee_basis_points.into()
    );
    assert_eq!(
        extension.older_transfer_fee.maximum_fee,
        maximum_fee.into()
    );
    assert_eq!(
        extension.newer_transfer_fee.transfer_fee_basis_points,
        transfer_fee_basis_points.into()
    );
    assert_eq!(
        extension.newer_transfer_fee.maximum_fee,
        maximum_fee.into()
    );
}

It passes, and the fees are set correctly.

Problem

From my investigations, it appears what's happening is the following:

On the Rust side of things, the instruction builder function takes an Option<&Pubkey> and maps it to a COption<Pubkey>, both of which use a 1 plus the bytes for Some(T), and simply a 0 for None.

When that's packed into a buffer, None values are only a single 0.

pub(crate) fn pack_pubkey_option(value: &COption<Pubkey>, buf: &mut Vec<u8>) {
match *value {
COption::Some(ref key) => {
buf.push(1);
buf.extend_from_slice(&key.to_bytes());
}
COption::None => buf.push(0),
}
}

On the JavaScript side of things, we're following the pattern of a OptionalNonZeroPubkey, by providing a 0 and then a buffer of length 32 all-zero.

This is causing the program's deserializer to see a leading 0 for the COption, deciding to move onto the next value, and seeing more zeroes rather than the actual values it should be seeing.

You'll notice that since there's 32 zeroes after the leading Option 0, not only the transfer fee config authority, but also the withdraw authority are None, in addition to the fee and max fee being 0.

In summary, we need to modify the JavaScript instruction serializer to serialize for COption rather than OptionalNonZeroPubkey, where an option value for None is just one 0.

Thanks for reporting this!

@buffalojoec buffalojoec added bug Something isn't working javascript labels Jan 18, 2024
@buffalojoec
Copy link
Contributor

Here's some code I used to investigate the issue, for anyone who might be interested:

// Rust
let ix = spl_token_2022::extension::transfer_fee::instruction::initialize_transfer_fee_config(
    &spl_token_2022::id(),
    token.get_address(),
    None,
    Some(&withdraw_withheld_authority.pubkey()),
    transfer_fee_basis_points,
    maximum_fee,
).unwrap();
println!("Data length: {}", ix.data.len());
println!("{:#?}", ix.data);
let ix = spl_token_2022::extension::transfer_fee::instruction::initialize_transfer_fee_config(
    &spl_token_2022::id(),
    token.get_address(),
    Some(&withdraw_withheld_authority.pubkey()),
    Some(&withdraw_withheld_authority.pubkey()),
    transfer_fee_basis_points,
    maximum_fee,
).unwrap();
println!("Data length: {}", ix.data.len());
println!("{:#?}", ix.data);
// JavaScript
const instructionWithAuthority = createInitializeTransferFeeConfigInstruction(
  new PublicKey(1),
  new PublicKey(3),
  new PublicKey(4),
  100,
  10_000n
);
const dataWithAuthority = instructionWithAuthority.data;

// Instruction without authority set
const instructionWithoutAuthority =
  createInitializeTransferFeeConfigInstruction(
    new PublicKey(1),
    null,
    new PublicKey(4),
    100,
    10_000n
  );
const dataWithoutAuthority = instructionWithoutAuthority.data;

console.log("Data length:", dataWithAuthority.length);
console.log(dataWithAuthority.toJSON());
console.log("Data length:", dataWithoutAuthority.length);
console.log(dataWithoutAuthority.toJSON());

In Rust, the two lengths are 46 then 78. In JavaScript, both lengths are 78.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers javascript
Projects
None yet
2 participants