Transfer hook support #15

grod220 · 2025-02-27T15:47:03Z

If the mint has support for the TransferHooks extension, the program will now execute them upon a wrap or unwrap action.

Makes use of helpers exposed by token-2022 repo. However, there is a dependency on solana-program/token-2022#233 for multisig support.

New test-transfer-hook program created to test out new functionality.

package.json

grod220 · 2025-02-27T20:22:21Z

program/Cargo.toml

 thiserror = "2.0.11"

-# Only needed until solana-sysvar is bumped to 2.1
-solana-program = "2.1.13"


grod220 · 2025-02-27T20:23:48Z

scripts/rust/test-sbf.mjs

+await $`RUST_LOG=error cargo test-sbf --manifest-path ${testProgramManifestPath} ${args}`;
+await $`RUST_LOG=error cargo test-sbf --manifest-path ${mainProgramManifestPath} ${args}`;


Important to add the test program here as the main program tests depend on the .so file

package.json

buffalojoec · 2025-02-28T08:42:10Z

program/Cargo.toml

+solana-account-info = "2.2.1"
+solana-decode-error = "2.2.1"
+solana-cpi = "2.2.1"
+solana-instruction = "2.2.1"
+solana-msg = "2.2.1"
+solana-program-entrypoint = "2.2.1"
+solana-program-error = "2.2.1"
+solana-program-option = "2.2.1"
+solana-program-pack = "2.2.1"
+solana-pubkey = "2.2.1"
+solana-rent = "2.2.1"


Assuming these all needed to be bumped to use the newer token-2022 lib? In the future, this is typically better in a standalone PR.

Yeah, makes sense. However, this PR depends on solana-program/token-2022#233 (which depends on the other deps being updated).

buffalojoec · 2025-02-28T08:50:00Z

program/Cargo.toml

+spl-tlv-account-resolution = "0.9.0"
+solana-account = "2.2.1"
+solana-system-program = "2.2.0"
+test-transfer-hook = { path = "tests/helpers/test-transfer-hook", features = ["no-entrypoint"] }


One idea for this program, so it's not so weird to use in the workspace, is you can actually define it as a builtin program, rather than an on-chain BPF program.

You can use declare_process_instruction! from the program-runtime and add_builtin on Mollusk's program_cache field to add it to the test environment.

That way, you can just define the program's simple processor in a test module and you don't need to compile a new crate. On the flip side, I noticed we do this in the token-2022 tests already, so not a big deal either way. What do you think?

Originally I was following the pattern that Mollusk had for its CPI tests. I worry if we don't do it this way, it doesn't quite emulate a program-to-program test. Perhaps I am off though?

It should be the same, but I suppose there's no harm in testing with true BPF-BPF cross-program invocations, as you're doing here. Maybe we can leave as you have it, then.

One small nit would be to consider committing the program ELF for the test program, like we do in Token-2022, if you think it makes life easier for CI/etc.
https://github.com/solana-program/token-2022/tree/main/clients/rust-legacy/tests/fixtures

scripts/rust/build-sbf.mjs

buffalojoec · 2025-02-28T10:33:41Z

program/src/instruction.rs

@@ -172,6 +172,7 @@ pub fn wrap(
    unwrapped_escrow_address: &Pubkey,
    transfer_authority_address: &Pubkey,
    multisig_signer_pubkeys: &[&Pubkey],
+    transfer_hook_metas: Vec<AccountMeta>,


nit: This can be a slice.

Also, if you want, you can leave these helpers alone as the "base" helpers, and you can roll a set of additional helpers that look just like these ones in Token-2022. You could even use an offchain module to keep with the pattern in the interface and the Token-2022 program.

nit: This can be a slice.

Given the accounts require owned AccountMeta, if this were a slice it would need to clone:

for meta in transfer_hook_metas.iter() { accounts.push(meta.clone()); }

So this would be a net neutral change?

if you want, you can leave these helpers alone as the "base" helpers

Ah, yes I think it's helpful to have the offchain/onchain helpers segmented off. However, all of these just form instructions, so I think all three would be pulled to the offchain file.

Given the accounts require owned AccountMeta, if this were a slice it would need to clone:

Ah, gotcha. If AccountMeta can't be dereferenced, then yeah makes sense.

buffalojoec · 2025-02-28T10:40:34Z

program/src/processor.rs

    solana_program_error::{ProgramError, ProgramResult},
    solana_program_pack::Pack,
    solana_pubkey::Pubkey,
    solana_rent::Rent,
    solana_system_interface::instruction::{allocate, assign},
+    spl_token::solana_program::sysvar::Sysvar,


Is this used?

Used for:

let rent = Rent::get()?;

buffalojoec · 2025-02-28T10:58:36Z

program/src/processor.rs

+    let mut multisig_signers = Vec::new();
+    let mut transfer_hook_accounts = Vec::new();
+    for account in additional_accounts {
+        if account.is_signer {
+            multisig_signers.push(account);
+        } else {
+            transfer_hook_accounts.push(account.clone());
+        }
+    }
+


A transfer hook account extra can also be a signer. What happens if you just pass all the accounts to burn and then use the onchain transfer helper for transfer?

So this works for burn (will update this), however it's tricker for transfer. If I use the helper invoke_transfer_checked() and signed multisig accounts were passed to the unwrap instruction (relevant to the burn), it will attempt to incorrectly add those to the escrow transfer (it should be PDA authorized transfer) https://github.com/solana-program/token-2022/blob/4829c03a0b34d86db3b663262883a434268874fa/program/src/instruction.rs#L1689-L1692 and throw MissingRequiredSignature.

Sounds like we have a few options:

Not support signed extra transfer hook accounts (feels less than ideal)

Update invoke_transfer_checked() helper in token-2022 repo to properly detect multisig accounts and only add those to the transfer invocation. Aka, instead of this https://github.com/solana-program/token-2022/blob/4829c03a0b34d86db3b663262883a434268874fa/program/src/onchain.rs#L49-L58, parse specifically multisig accounts like so: https://github.com/solana-program/token-2022/blob/4829c03a0b34d86db3b663262883a434268874fa/program/src/processor.rs#L1949-L1956

Thoughts?

Ah, I see, it will add the multisigs first before adding the "extra metas", and the multisigs are only required for burning here.

Maybe here, since we know we don't want to add multisig signers to the transfer instruction (only the burn), we can maybe circumvent invoke_transfer_checked and just build things out with spl_transfer_hook_interface::onchain::add_extra_accounts_for_execute_cpi directly.

What do you think?

Actually, was a bit confused. There are two problems. Both require multisig account detection.

invoke_transfer_checked() does not support signed extra transfer hook accounts.

Going to open a subsequent PR to this PR to address that. There is a current bug where it adds a redundant AccountMeta to the instruction likely messing up account ordering and dropping the signed account meta from the authority.

The Unwrap call to invoke_transfer_checked() includes possible multisig accounts which aren't relevant to the PDA escrow transfer (creates a transfer authority non-signature account meta which causes a MissingRequiredSignature error in token-2022 program).

The challenge with using add_extra_accounts_for_execute_cpi() directly is that I would still need to call it conditionally (dependent on mint hook), assemble the instruction & remove the multisig accounts from the accounts list. If I can just do the removal step, I can re-use invoke_transfer_checked() all the same.

grod220 added 3 commits February 27, 2025 16:40

Wrap transfer hook support

5ad644e

Unwrap transfer hook support

4969731

CI/CD updates

771ed4a

grod220 commented Feb 27, 2025

View reviewed changes

package.json Outdated Show resolved Hide resolved

grod220 commented Feb 27, 2025

View reviewed changes

grod220 marked this pull request as ready for review February 27, 2025 20:24

grod220 requested review from joncinque and buffalojoec February 27, 2025 20:24

buffalojoec reviewed Feb 28, 2025

View reviewed changes

Review updates

b9191aa

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transfer hook support #15

Transfer hook support #15

grod220 commented Feb 27, 2025

grod220 Feb 27, 2025

grod220 Feb 27, 2025

buffalojoec Feb 28, 2025

grod220 Feb 28, 2025

buffalojoec Feb 28, 2025

grod220 Feb 28, 2025

buffalojoec Feb 28, 2025

buffalojoec Feb 28, 2025

buffalojoec Feb 28, 2025

grod220 Feb 28, 2025

buffalojoec Feb 28, 2025

buffalojoec Feb 28, 2025

grod220 Feb 28, 2025

buffalojoec Feb 28, 2025

buffalojoec Feb 28, 2025

grod220 Feb 28, 2025

buffalojoec Feb 28, 2025 •

edited

Loading

grod220 Feb 28, 2025 •

edited

Loading

		await $`RUST_LOG=error cargo test-sbf --manifest-path ${testProgramManifestPath} ${args}`;
		await $`RUST_LOG=error cargo test-sbf --manifest-path ${mainProgramManifestPath} ${args}`;

Transfer hook support #15

Are you sure you want to change the base?

Transfer hook support #15

Conversation

grod220 commented Feb 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buffalojoec Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

grod220 Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

buffalojoec Feb 28, 2025 •

edited

Loading

grod220 Feb 28, 2025 •

edited

Loading