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 helper: support signed transfer hooks #253

Merged
merged 7 commits into from
Mar 7, 2025
Merged

Conversation

grod220
Copy link
Contributor

@grod220 grod220 commented Mar 3, 2025

Follow up from #233

At the moment the invoke_transfer_checked() function is indiscriminately adding signed accounts to the CPI instruction. Normally this is fine, but when a transfer hook is involved, a bug is apparent. It mistakenly gets added to the list of accounts and then later again as it's handled by add_extra_accounts_for_execute_cpi().

This PR fixes this bug by parsing multisig accounts specifically versus signed accounts generically.

@@ -473,7 +534,7 @@ mod tests {

let extra_account2_info = AccountInfo::new(
&extra_account2_key,
false,
true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The key variable change. When making an extra transfer hook account signed, it previously caused the tests to fail.

Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Just to make sure I understand, the problem is that if you mark an extra transfer hook account as a signer, it mistakenly gets added as a multisig signer as well?

So, for example, if we have two multisig signers and a signed extra hook account:

Instruction: "TransferChecked"
    * [w] Source
    * [ ] Mint
    * [w] Destination
    * [ ] Authority (multisig account)
    * [s] Multisig signer #1
    * [s] Multisig signer #2
    * [s] Extra transfer hook meta
    * [s] Extra transfer hook meta
    * [ ] Validation account
    * [ ] Transfer hook program

But where does this throw?

@grod220 grod220 force-pushed the multisig-helper-fix branch from 612ef33 to a5cfb19 Compare March 4, 2025 15:33
@grod220
Copy link
Contributor Author

grod220 commented Mar 4, 2025

By adding all signed additional accounts to the signer pubkeys vec, you'd get this base instruction:

Instruction: "TransferChecked"
    * [w] Source
    * [ ] Mint
    * [w] Destination
    * [ ] Authority (multisig account)
    * [s] Multisig signer #1
    * [s] Multisig signer #2
    * [s] Extra transfer 1 hook meta
    * [s] Extra transfer 2 hook meta

and then add_extra_accounts_for_execute_cpi() would append the extra accounts:

    * [s] Extra transfer 1 hook meta
    * [s] Extra transfer 2 hook meta
    * [ ] Validation account
    * [ ] Transfer hook program

So the resulting call would look like this:

Instruction: "TransferChecked"
    * [w] Source
    * [ ] Mint
    * [w] Destination
    * [ ] Authority (multisig account)
    * [s] Multisig signer #1
    * [s] Multisig signer #2
    * [s] Extra transfer 1 hook meta
    * [s] Extra transfer 2 hook meta
    * [s] Extra transfer 1 hook meta
    * [s] Extra transfer 2 hook meta
    * [ ] Validation account
    * [ ] Transfer hook program

@buffalojoec
Copy link
Contributor

So the resulting call would look like this:

Wow, I guess we really didn't test this thing with multisigs and transfer hooks. 😂 😂

@grod220 grod220 requested a review from buffalojoec March 5, 2025 10:53
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.

Just a few comments, looks good otherwise!

@grod220 grod220 force-pushed the multisig-helper-fix branch from d4e342c to 2b35332 Compare March 5, 2025 14:46
@grod220 grod220 requested a review from joncinque March 5, 2025 15:21
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.

A couple more things on my side, then this can go in!

@grod220 grod220 force-pushed the multisig-helper-fix branch from 2b35332 to ea742a7 Compare March 6, 2025 10:43
@grod220 grod220 requested a review from joncinque March 6, 2025 11:06
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!

Copy link
Contributor

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

Lgtm, but I still think we could avoid the vector.

@grod220 grod220 force-pushed the multisig-helper-fix branch from 9092f76 to 00e0f47 Compare March 7, 2025 08:57
@grod220 grod220 enabled auto-merge (squash) March 7, 2025 09:11
@grod220 grod220 disabled auto-merge March 7, 2025 09:12
@grod220 grod220 force-pushed the multisig-helper-fix branch from 03b8c0a to 00e0f47 Compare March 7, 2025 10:32
@grod220 grod220 force-pushed the multisig-helper-fix branch from 00e0f47 to c3d5aaa Compare March 7, 2025 11:18
@grod220 grod220 enabled auto-merge (squash) March 7, 2025 11:19
@grod220 grod220 merged commit 659b12a into main Mar 7, 2025
19 of 20 checks passed
@grod220 grod220 deleted the multisig-helper-fix branch March 7, 2025 11:33
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.

3 participants