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

Adds --blockhash arg to Close #7456

Merged
merged 2 commits into from
Nov 12, 2024
Merged

Conversation

brooksprumo
Copy link
Contributor

@brooksprumo brooksprumo commented Nov 4, 2024

I'd like the --blockhash arg on the Close instruction. This is done by calling .offline_args().

output from `spl-token close --help`

spl-token-close 
Close a token account

USAGE:
    spl-token close [OPTIONS] [--] [TOKEN_MINT_ADDRESS]

ARGS:
    <TOKEN_MINT_ADDRESS>    Token of the associated account to close. To close a specific
                            account, use the `--address` parameter instead

OPTIONS:
        --address <TOKEN_ACCOUNT_ADDRESS>
            Specify the token account to close [default: owner's associated token account]

        --blockhash <BLOCKHASH>
            Use the supplied blockhash

    -C, --config <PATH>
            Configuration file to use

        --close-authority <KEYPAIR>
            Specify the token's close authority if it has one, otherwise specify the token's owner
            keypair. This may be a keypair file or the ASK keyword. Defaults to the client keypair.

        --dump-transaction-message
            Display the base64 encoded binary transaction message in sign-only mode

        --fee-payer <KEYPAIR>
            Specify the fee-payer account. This may be a keypair file, the ASK keyword
            or the pubkey of an offline signer, provided an appropriate --signer argument
            is also passed. Defaults to the client keypair.

    -h, --help
            Print help information

        --multisig-signer [<MULTISIG_SIGNER>...]
            Member signer of a multisig account

        --nonce <PUBKEY>
            Provide the nonce account to use when creating a nonced
            transaction. Nonced transactions are useful when a transaction
            requires a lengthy signing process. Learn more about nonced
            transactions at https://docs.solanalabs.com/cli/examples/durable-nonce

        --nonce-authority <KEYPAIR>
            Provide the nonce authority keypair to use when signing a nonced transaction

        --output <FORMAT>
            Return information in specified output format [possible values: json, json-compact]

        --owner <OWNER_ADDRESS>
            Address of the primary authority controlling a mint or account. Defaults to the client
            keypair address.

    -p, --program-id <ADDRESS>
            SPL Token program id

        --program-2022
            Use token extension program token 2022 with program id:
            TokenzQdBNbLqP5VEhdkAS6EPFLC1PHnBqCXEpPxuEb

        --recipient <REFUND_ACCOUNT_ADDRESS>
            The address of the account to receive remaining SOL [default: --owner]

        --sign-only
            Sign the transaction offline

        --signer <PUBKEY=SIGNATURE>
            Provide a public-key/signature pair for the transaction

    -u, --url <URL_OR_MONIKER>
            URL for Solana's JSON RPC or moniker (or their first letter): [mainnet-beta, testnet,
            devnet, localhost] Default from the configuration file.

    -v, --verbose
            Show additional information

        --with-compute-unit-limit <COMPUTE-UNIT-LIMIT>
            Set compute unit limit for transaction, in compute units.

        --with-compute-unit-price <COMPUTE-UNIT-PRICE>
            Set compute unit price for transaction, in increments of 0.000001 lamports per compute
            unit.

Copy link
Contributor

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good to me! The close implementation already takes into account the sign_only field, so this should be the only change needed.

Once you've had a chance to try this locally just to double-check, we can merge it. Thanks for your contribution!

@brooksprumo
Copy link
Contributor Author

I finally got around to testing this change. I was successfully able to call spl-token close --blockhash, and it worked.

However, it returned an error, even though the transaction was sent, and the token account was closed... I'm not sure if this is related or not; do you know?

Error: Error { request: None, kind: RpcError(ForUser("unable to confirm transaction. This can happen in situations such as transaction expiration and insufficient fee-payer funds")) }

joncinque added a commit to joncinque/solana-program-library that referenced this pull request Nov 11, 2024
#### Problem

As noticed with solana-labs#7456, a transaction with a nonced blockhash fails to
confirm even though it works.

This might be because of the confirmation logic in the RPC client, which
looks for the provided blockhash:
https://github.com/anza-xyz/agave/blob/b46c87ea538a1508ed4ae36f8dbf6c75dd57c883/rpc-client/src/nonblocking/rpc_client.rs#L1090

#### Summary of changes

Specify a default transaction confirmation timeout.
@joncinque
Copy link
Contributor

However, it returned an error, even though the transaction was sent, and the token account was closed... I'm not sure if this is related or not; do you know?

Hm, that typically means that there was an issue confirming the transaction. I wonder if there's a bug in confirm_transaction_with_spinner, specifically this bit that might fail, especially with the nonce blockhash: https://github.com/anza-xyz/agave/blob/b46c87ea538a1508ed4ae36f8dbf6c75dd57c883/rpc-client/src/nonblocking/rpc_client.rs#L1090

We might need to create the RPC client with timeouts in token-cli, similar to what we're doing in the main CLI. I put in #7484 for that -- let me know what you think!

@joncinque
Copy link
Contributor

Do you want to test again with my PR too? My hunch is that this is good to go either way

@brooksprumo
Copy link
Contributor Author

specifically this bit that might fail, especially with the nonce blockhash

I didn't use a nonce in my testing, just a recent blockhash I got from getLatestBlockhash. Do you think that code you highlighted would still fail in this circumstance?

@joncinque
Copy link
Contributor

I didn't use a nonce in my testing, just a recent blockhash I got from getLatestBlockhash. Do you think that code you highlighted would still fail in this circumstance?

Hm, it would only fail if the transaction got in right before the blockhash expired. Anyway, I've tested this locally and it works without my PR, so let's merge this in

@joncinque
Copy link
Contributor

Actually, I noticed that just passing in --blockhash doesn't actually use that blockhash, so I modified the nonce test to also close the account with a specified blockhash. Do you mind if I push the test change to your branch?

@brooksprumo
Copy link
Contributor Author

Do you mind if I push the test change to your branch?

Go for it!

@joncinque
Copy link
Contributor

Ok cool, let me know how this looks!

@brooksprumo
Copy link
Contributor Author

Ok cool, let me know how this looks!

The tests look good to me, thanks!

@joncinque joncinque merged commit 920ab31 into solana-labs:master Nov 12, 2024
31 checks passed
@brooksprumo brooksprumo deleted the blockhash-arg branch November 12, 2024 14:41
joncinque added a commit that referenced this pull request Nov 12, 2024
#### Problem

As noticed with #7456, a transaction with a nonced blockhash fails to
confirm even though it works.

This might be because of the confirmation logic in the RPC client, which
looks for the provided blockhash:
https://github.com/anza-xyz/agave/blob/b46c87ea538a1508ed4ae36f8dbf6c75dd57c883/rpc-client/src/nonblocking/rpc_client.rs#L1090

#### Summary of changes

Specify a default transaction confirmation timeout.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants