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 update instruction #5662

Closed
joncinque opened this issue Oct 25, 2023 · 8 comments
Closed

transfer-hook: Add update instruction #5662

joncinque opened this issue Oct 25, 2023 · 8 comments
Labels
good first issue Good for newcomers

Comments

@joncinque
Copy link
Contributor

Problem

A program implementing the transfer-hook interface only absolutely requires one instruction, the execute hook which is called by token-2022.

However, the interface does also include an initialization function, which is needed in some form to write data to the extra-account-metas account. An implementer may choose to do something else for that instruction.

Similarly, there's no way to update the extra-account-metas account with just the interface.

Solution

This behavior is fine as is, but it would be nicer to have a recommended-but-not-required update instruction for devs looking to implement the transfer hook.

cc @buffalojoec

@joncinque joncinque added the good first issue Good for newcomers label Oct 25, 2023
@gdutk007
Copy link

would it be possible for me to work on this issue? I'm a bit of a newcomer to this domain but wouldn't mind tackling it.

@buffalojoec
Copy link
Contributor

would it be possible for me to work on this issue? I'm a bit of a newcomer to this domain but wouldn't mind tackling it.

Of course! I’d start with the interface change, then in separate PRs we can address the different clients.

Feel free to request my review.

@tonton-sol
Copy link
Contributor

I am also trying to implement this. I have added the new instruction to the interface and example program. I have also added a subcommand to the CLI. When testing with the CLI, the RPC simulation fails with custom program error: 0x47af3bc1.

How can I decode this? Also is there an easy way to show logs of the failed RPC simulation?

@tonton-sol
Copy link
Contributor

tonton-sol commented Nov 24, 2023

I figured out the error, it is TlvError::TypeAlreadyExists.

I thought I could simply just create a new TransferHookInstruction::UpdateExtraAccountMetaList in the interface similar to TransferHookInstruction::InitializeExtraAccountMetaList, then change the example program to handle this instruction.

The error arises when:

ExtraAccountMetaList::init::<ExecuteInstruction>(&mut data, extra_account_metas)?;

is called in the new update instruction in the example program. Also why is the TLV instruction type ExecuteInstuction and not InitializeExtraAccountMetaListInstruction or UpdateExtraAccountMetaListInstruction?

@buffalojoec
Copy link
Contributor

The error arises when:

ExtraAccountMetaList::init::<ExecuteInstruction>(&mut data, extra_account_metas)?;

is called in the new update instruction in the example program.

This is going to try to initialize a new TLV entry and it will error if it exists, so it's not appropriate for updating an existing one! You'll have to add an update method to ExtraAccountMetaList and inside of it drive TlvStateMut and realloc_and_pack_* or something similar.

Also why is the TLV instruction type ExecuteInstuction and not InitializeExtraAccountMetaListInstruction or UpdateExtraAccountMetaListInstruction?

This is because the instructions' discriminator is used to identify which set of extra accounts are required for a particular instruction. For example:

----------------------------------------------------------
| <ExecuteInstruction>  | <extra accounts for execute>[] |
| <OtherInstruction>    | <extra accounts for other>[]   |
----------------------------------------------------------

The confusing bit here is that besides "initializing extra required accounts", Transfer Hook has only one instruction: execute the transfer.

@tonton-sol
Copy link
Contributor

Thank you! I have a working implementation now, that updates tlv-account-resolution, the transfer-hook-interface, and the example program. How can I let you review it?

@buffalojoec
Copy link
Contributor

Thank you! I have a working implementation now, that updates tlv-account-resolution, the transfer-hook-interface, and the example program. How can I let you review it?

I saw! If you can't request my review just tag me in a comment or the PR description.

@buffalojoec
Copy link
Contributor

Done as of #5894 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants