Transfer hook offchain and onchain helpers bugfix #6065

buffalojoec · 2024-01-05T08:34:29Z

This PR introduces some modifications to Token2022's off-chain and on-chain helpers that simply convert a transfer instruction into an Execute instruction before resolving the extra account metas.

Since anyone storing extra account meta configs should be considering the list of accounts as it will arrive in their Transfer Hook program's Execute instruction, it makes sense to resolve for Execute.

After all, the main problem we have here is that we're asking TLV Account Resolution to resolve the extra account metas for an Execute instruction, but we're not actually feeding it a valid Execute instruction. Instead, we're feeding it a transfer instruction, typically TransferChecked.

I've also added some tests!

Closes #6064

A Note on Token2022 Release

A release is necessary to publish this bugfix. However, since this change is specific to Token2022's off-chain and on-chain helpers, the on-chain program does not need to be modified. Developers will simply need to use the newer version of the crate, as well as the newer versions of the Token Client and CLI if they'd like to use the repaired helpers.

We can communicate the bug in question by describing the exact use cases one may encounter issues:

Defining extra account metas that have seed configurations pointing to instruction data by index
Defining extra account metas that have seed configurations pointing to other extra account metas by index (not instruction accounts; these will work just fine)

This means the following extra account meta configurations will still work as expected:

Defining extra account metas that have fixed addresses
Defining extra account metas that have seed configurations with literal bytes

Also, while we're at it, I think we need to publish some extremely detailed documentation on how to use Transfer Hook extra metas.

buffalojoec · 2024-01-08T18:43:56Z

Hey @joncinque I'm going to break this up into 3 PRs for easier review, but I'd like for you to have a look at the linked issue and the general proposed solution, as well as my notes on release.

joncinque · 2024-01-08T22:32:56Z

Great catch! The issue is definitely valid, it's definitely best to split this up, the release plan makes sense, and we definitely need more docs 😅

It makes me realize that seed configurations won't work in all cases. For example, if you're writing a swap program, and a token derives a PDA based on instruction data, then there's no way to get the right address, since the amount will be variable based on the swap calculation.

I'll make some comments on the interface

joncinque · 2024-01-08T22:34:13Z

token/program-2022/src/offchain.rs

 pub async fn resolve_extra_transfer_account_metas<F, Fut>(
    instruction: &mut Instruction,
    fetch_account_data_fn: F,
    mint_address: &Pubkey,
+    amount: u64,
 ) -> Result<(), AccountFetchError>


Since this is specific to transfers, let's deprecate this function, and create a new one which requires passing in all the transfer parameters, and that way we don't have to assume that a proper transfer instruction is provided.

joncinque · 2024-01-08T22:35:32Z

token/program-2022/src/onchain.rs

+pub fn resolve_extra_transfer_account_metas_for_cpi<'a>(
+    cpi_instruction: &mut Instruction,
+    cpi_account_infos: &mut Vec<AccountInfo<'a>>,
+    mint_info: &AccountInfo<'a>,
+    additional_accounts: &[AccountInfo<'a>],
+    amount: u64,
+) -> Result<(), ProgramError> {


Same here, let's pass the transfer parameters by name, similar to invoke_transfer_checked, rather than implicitly in the instruction

joncinque · 2024-01-08T22:37:06Z

token/program-2022-test/tests/transfer_hook.rs

    offchain::resolve_extra_transfer_account_metas(
-        &mut instruction,
+        &mut transfer_1_instruction,


By updating the params on this function, this test should look much clearer!

buffalojoec · 2024-01-08T22:42:39Z

It makes me realize that seed configurations won't work in all cases. For example, if you're writing a swap program, and a token derives a PDA based on instruction data, then there's no way to get the right address, since the amount will be variable based on the swap calculation.

Actually I'm glad you brought this up. I have been mulling on an idea for a more dynamic approach to some account keys, but I haven't thought through all of the implementation yet.

The use case I had in mind was "what if I want to require an extra account meta that's not a PDA but can be different every time someone invokes my program?". For example, let's say I require some program ID to CPI to, and maybe that program just has to implement some interface. Right now you'd have to make that a static fixed address, but it could be set to [0u8; 32] and that just means "wildcard". That way it can be whatever address!

Note if we did add more seed support we still wouldn't have to change Token2022's on-chain program. 🙏🏼

As far as the instruction data issue you presented, I think that's probably okay and not our concern at the moment. Technically no matter what program you're writing, you have to implement the Execute instruction. That means your instruction data is always going to be <discriminator (8)> + <amount (8)>. If you extend inputs through account state, you're covered with Seed::AccountData and if you are doing calculations within the program, that's a really far-out edge case in my opinion that's hard to use as a seed even natively.

Curious your thoughts on either of these points.

buffalojoec · 2024-01-08T22:46:22Z

FYI I've added the comprehensive docs to my list, and I'll produce something next week.

joncinque · 2024-01-08T22:59:05Z

That way it can be whatever address!

Seems like that should be a different ExtraAccountMeta type entirely. It should be fine to implement that as a non-breaking change and use the newer library in token-2022 for the next deployment if you really want to support it. Has anyone asked for it? I'm not even sure how the resolver would work for that. Given a huge number of accounts, how does it pick which one is the "bonus" one?

if you are doing calculations within the program, that's a really far-out edge case in my opinion that's hard to use as a seed even natively

We need to always consider supporting DeFi or trading apps, so doing calculations shouldn't be considered an edge case. We'll just have to hope that people won't use instruction data for deriving their transfer hook accounts 😬

buffalojoec · 2024-01-08T23:04:44Z

That way it can be whatever address!

Seems like that should be a different ExtraAccountMeta type entirely. It should be fine to implement that as a non-breaking change and use the newer library in token-2022 for the next deployment if you really want to support it. Has anyone asked for it? I'm not even sure how the resolver would work for that. Given a huge number of accounts, how does it pick which one is the "bonus" one?

Agreed, it would be another ExtraAccountMeta type. And yeah, this was where I hit the snag, lol.

if you are doing calculations within the program, that's a really far-out edge case in my opinion that's hard to use as a seed even natively

We need to always consider supporting DeFi or trading apps, so doing calculations shouldn't be considered an edge case. We'll just have to hope that people won't use instruction data for deriving their transfer hook accounts 😬

But isn't the instruction data always Execute, ie <discriminator (8)> + <amount (8)>? That's the Transfer Hook interface. You couldn't use another instruction.

joncinque · 2024-01-08T23:10:24Z

But isn't the instruction data always Execute, ie <discriminator (8)> + <amount (8)>? That's the Transfer Hook interface. You couldn't use another instruction.

Correct. I was focusing on the amount part, which will be impossible to figure out off-chain for trading applications.

buffalojoec · 2024-01-09T22:37:51Z

Closing in favor of the segmented PRs going in for this.

buffalojoec marked this pull request as draft January 6, 2024 00:27

buffalojoec force-pushed the transfer-hook-offchain-bugfix branch from 1d3f628 to dc43dcc Compare January 6, 2024 01:09

This comment was marked as off-topic.

Sign in to view

buffalojoec force-pushed the transfer-hook-offchain-bugfix branch from dc43dcc to 4ea07a5 Compare January 8, 2024 08:23

buffalojoec added 3 commits January 8, 2024 02:35

token-2022: modify offchain extra metas helper

7d3ecc8

token-2022: add test for the modified offchain extra metas helper

e015464

token-2022: refactor transfer hook program test

6b915a7

buffalojoec force-pushed the transfer-hook-offchain-bugfix branch from 4ea07a5 to 093a4e1 Compare January 8, 2024 08:42

buffalojoec changed the title ~~Transfer hook offchain bugfix~~ Transfer hook offchain and onchain helpers bugfix Jan 8, 2024

buffalojoec added 4 commits January 8, 2024 12:40

token-2022: modify onchain extra metas helper

ff029c3

token-2022: add test for the modified onchain extra metas helper

85359e1

token js: modify offchain extra metas helper

87e6525

token js: add test for the modified offchain extra metas helper

c35964e

buffalojoec force-pushed the transfer-hook-offchain-bugfix branch from 093a4e1 to c35964e Compare January 8, 2024 18:41

joncinque reviewed Jan 8, 2024

View reviewed changes

buffalojoec mentioned this pull request Jan 9, 2024

token-2022: repair offchain extra metas helper #6088

Closed

buffalojoec closed this Jan 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transfer hook offchain and onchain helpers bugfix #6065

Transfer hook offchain and onchain helpers bugfix #6065

buffalojoec commented Jan 5, 2024 •

edited

Loading

This comment was marked as off-topic.

buffalojoec commented Jan 8, 2024

joncinque commented Jan 8, 2024

joncinque Jan 8, 2024

joncinque Jan 8, 2024 •

edited

Loading

joncinque Jan 8, 2024

buffalojoec commented Jan 8, 2024

buffalojoec commented Jan 8, 2024

joncinque commented Jan 8, 2024

buffalojoec commented Jan 8, 2024

joncinque commented Jan 8, 2024

buffalojoec commented Jan 9, 2024

Transfer hook offchain and onchain helpers bugfix #6065

Transfer hook offchain and onchain helpers bugfix #6065

Conversation

buffalojoec commented Jan 5, 2024 • edited Loading

A Note on Token2022 Release

This comment was marked as off-topic.

buffalojoec commented Jan 8, 2024

joncinque commented Jan 8, 2024

joncinque Jan 8, 2024

Choose a reason for hiding this comment

joncinque Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

joncinque Jan 8, 2024

Choose a reason for hiding this comment

buffalojoec commented Jan 8, 2024

buffalojoec commented Jan 8, 2024

joncinque commented Jan 8, 2024

buffalojoec commented Jan 8, 2024

joncinque commented Jan 8, 2024

buffalojoec commented Jan 9, 2024

buffalojoec commented Jan 5, 2024 •

edited

Loading

joncinque Jan 8, 2024 •

edited

Loading