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

token-2022: Execute transfer hook during confidential transfer #6098

Merged
merged 7 commits into from Jan 18, 2024

Conversation

joncinque
Copy link
Contributor

Problem

If a mint is configured with a transfer hook, the program is meant to be called during all transfers. This doesn't happen during confidential transfers though.

Summary of changes

The program changes are pretty simple: just execute the transfer hook, as in a normal transfer. This comes with a few other changes:

The biggest issue is that we don't know the amount transferred when calling the hook. I decided to go with u64::MAX as a convention, rather than adding another instruction to the interface. Transfer hooks are complicated enough as is, so forcing programs to implement another instruction seems like a bad idea. Let me know if you have better ideas!

samkim-crypto
samkim-crypto previously approved these changes Jan 11, 2024
Copy link
Contributor

@samkim-crypto samkim-crypto left a comment

Choose a reason for hiding this comment

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

Sorry it took a while to catch up on some of the transfer hook interface. All the changes look fine to me. Thanks for fixing the variables names as well. I think the convention of using u64::MAX is reasonable to keep things as simple as possible.

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

Looks good to me! You mentioned you'll want to rebase this once the PRs go in for fixing #6064, but that should be a pretty minimal change here.

@mergify mergify bot dismissed stale reviews from buffalojoec and samkim-crypto January 18, 2024 01:43

Pull request has been modified.

@joncinque
Copy link
Contributor Author

I added a new helper for this since resolve_extra_transfer_account_metas is deprecated, let me know how it looks!

buffalojoec
buffalojoec previously approved these changes Jan 18, 2024
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, just some up-to-you nits!

token/client/src/token.rs Outdated Show resolved Hide resolved
token/client/src/token.rs Outdated Show resolved Hide resolved
@mergify mergify bot dismissed buffalojoec’s stale review January 18, 2024 16:06

Pull request has been modified.

@joncinque joncinque merged commit 14db758 into solana-labs:master Jan 18, 2024
35 checks passed
@joncinque joncinque deleted the confhook branch January 18, 2024 17:54
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

3 participants