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

transfer hook: add new offchain helper #6099

Merged
merged 2 commits into from
Jan 11, 2024

Conversation

buffalojoec
Copy link
Contributor

@buffalojoec buffalojoec commented Jan 10, 2024

This PR adds a new offchain helper for adding the necessary account metas for an
ExecuteInstruction to the SPL Transfer Hook interface, deprecating the old
one.

As described in #6064, the offchain helper in Token2022 was using the original
offchain helper from the SPL Transfer Hook interface incorrectly when resolving
extra account metas for a transfer.

In order to provide a safer, more robust helper, this new function takes the
instruction, fetch account data function, as well as the individual arguments
for instruction::execute(..). This will help to ensure Token2022 as well as
anyone else using the helpers from the SPL Transfer Hook interface are properly
resolving the necessary additional accounts.

Note: Although deprecated, the original helper in the SPL Transfer Hook
interface is not broken. It's just less safe to use than this new helper, since
it can easily be misused.

@buffalojoec buffalojoec force-pushed the 01-09-transfer_hook_add_new_offchain_helper branch 2 times, most recently from 21f958e to 15ddbc8 Compare January 10, 2024 03:49
@buffalojoec buffalojoec marked this pull request as ready for review January 10, 2024 14:10
joncinque
joncinque previously approved these changes Jan 10, 2024
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.

Looks great! Just a few nits to clean up, but feel free to merge after they're addressed

token/transfer-hook/interface/src/offchain.rs Show resolved Hide resolved
token/transfer-hook/interface/src/offchain.rs Show resolved Hide resolved
token/program-2022/src/offchain.rs Outdated Show resolved Hide resolved
@mergify mergify bot dismissed joncinque’s stale review January 10, 2024 17:04

Pull request has been modified.

joncinque
joncinque previously approved these changes Jan 10, 2024
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.

@buffalojoec buffalojoec force-pushed the 01-09-transfer_hook_add_new_offchain_helper branch from 262663a to 3700ef4 Compare January 11, 2024 16:35
@mergify mergify bot dismissed joncinque’s stale review January 11, 2024 16:35

Pull request has been modified.

Copy link
Contributor Author

buffalojoec commented Jan 11, 2024

Merge activity

@buffalojoec buffalojoec merged commit 8076018 into master Jan 11, 2024
36 checks passed
@buffalojoec buffalojoec deleted the 01-09-transfer_hook_add_new_offchain_helper branch January 11, 2024 21:15
buffalojoec added a commit that referenced this pull request Jan 11, 2024
This PR adds a new offchain helper for creating a `TransferChecked` instruction 
with the necessary accounts metas for a transfer hook `ExecuteInstruction` to 
Token2022, deprecating the old one.

As described in #6064, the original offchain helper in Token2022 was being used 
incorrectly to resolve extra account metas. Rather than providing the SPL 
Transfer Hook interface helper with a valid `ExecuteInstruction`, the original 
Token2022 helper was actually providing it with a transfer instruction, causing 
erroneous account resolution.

Taking advantage of the more secure SPL Transfer Hook interface offchain helper 
provided in #6099, this new offchain helper creates a `TransferChecked` 
instruction and calls `add_extra_account_metas_for_execute(..)`, providing the 
keys used to build the transfer instruction.

Note: unlike the deprecated helper in #6099, the deprecated offchain helper in 
Token2022 *is* in fact inaccurately resolving account metas for certain use 
cases, thus it should be vigilantly avoided.
buffalojoec added a commit that referenced this pull request Jan 11, 2024
Following up from the new helpers added in #6099 and #6100, there's a transfer
hook test in `program-2022-test` that's using the now-deprecated helper to build
a swap instruction for the test program.

This PR replaces that bit of code to use the new helper from
`spl_transfer_hook_interface`.

Note: The test in question is designed to test Token2022's
`invoke_transfer_checked(..)` on-chain helper, *not* the offchain helpers. So,
it's appropriate to use the offchain helper directly from the interface crate
for this step.
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.

None yet

2 participants