-
Notifications
You must be signed in to change notification settings - Fork 52
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
solana: support tokens with transfer hooks #404
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No opposition to the changes so far. Looks like this needs a bit of work to get past CI
84a72f2
to
dd8dbbd
Compare
Hm, interesting. |
0028237
to
943e5bd
Compare
@kcsongor I'm getting the same issue as CI when I run Maybe check your local CLI tool versions? Here are mine:
|
for me |
I have |
Downgraded to 0.29.0, it still works. But I managed to reproduce this on my x86 ubuntu machine (it's working on arm64/macOS). What arch/OS are you running on? |
I'm using |
aec029b
to
e3fe990
Compare
I haven't done an in-depth review yet but I've assigned myself and will return to it later today or this week. Quick note: Are the changes to the Solidity files meant to be here or are they the result of the rebase? I'm unclear how they're related to the changes for the Solana code. |
10c2d3e
to
b312d0d
Compare
604e25a
to
29fb437
Compare
Done, the diff shows up correctly now -- thanks for pointing it out! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions about the API. Will approve after we discuss.
solana/programs/example-native-token-transfers/src/instructions/release_inbound.rs
Show resolved
Hide resolved
solana/programs/example-native-token-transfers/src/instructions/release_inbound.rs
Outdated
Show resolved
Hide resolved
Added a new commit which makes some non-trivial changes. tldr we now perform a transfer before burning on the way out, and a transfer after minting on the way in. This is done in order to trigger the transfer hooks of the underlying token, which minting and burning don't trigger. CI fails because now there are too many accounts in the transfer transaction (since it does an approve, then a transfer, then a release outbound via wormhole. The transfer hook adds 3 extra accounts, which just about tips it over the limit. I'll have to add a lookup table to the sdk). |
cfeeccb
to
a3ccab3
Compare
b312d0d
to
63c3743
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some notes, mostly to confirm that removing some of the checks should be safe. I like the Anchor constraint changes for the Mode checks and the comment additions quite a lot.
solana/programs/example-native-token-transfers/src/instructions/release_inbound.rs
Show resolved
Hide resolved
solana/programs/example-native-token-transfers/src/instructions/release_inbound.rs
Show resolved
Hide resolved
solana/programs/example-native-token-transfers/src/instructions/release_inbound.rs
Show resolved
Hide resolved
solana/programs/example-native-token-transfers/src/instructions/release_inbound.rs
Show resolved
Hide resolved
solana/programs/example-native-token-transfers/src/instructions/transfer.rs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good on my side from a security perspective. Csongor and I discussed some ideas on Slack and I'm comfortable with the state of the PR.
4b564c0
to
a72e72f
Compare
TODO: the old token program still works -- is this secure? I think so but double check
add script to sync/check crate versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no additional changes since my last review from what I can tell
the only thing that stuck out to me when taking a second glance is that the ntt quoter version also got bumped to version 2.0 even though for the ntt quoter itself nothing actually changed, which might be slightly confusing ("What's the difference between ntt-quoter 1.x and 2.0?" "No difference.") though ofc not bumping it is maybe also bad ("Is this the right version of the quoter for that version of NTT?").
This PR adds support for token2022 tokens with transfer hooks. For more details on transfer hooks, see https://solana.com/developers/guides/token-extensions/transfer-hook.
At a high level, a transfer hook is a program that is called (by the token program) each time a transfer (of a token of a specific mint) is made. The key difficulty is that the transfer hook program may require additional accounts, which need to be propagated through the instruction.
This PR modifies the on-chain code to call the appropriate helper function that propagates the necessary accounts, and also the off-chain code (ts sdk) that derives these account addresses based on the on-chain schema standard that token2022 introduces for transfer hooks. The rust tests (
cargo test-sbf
) still test against the legacy spl token, while in this PR I modified the typescript tests to test against a token with a transfer hook. This way, both paths are tested appropriately.There is a dummy transfer hook in the test that bumps a counter on each transfer, and has a dummy account seeded by the owner field of the sender token account, which is just used to test that the account information is properly propagated.
Adding this was the motivation for #391, because the on-chain helper code is simply completely unusable (broken) in earlier sdk versions.
ABI changes
Breaking changes
This change is breaking, so it warrants a major ABI version upgrade (bumped the version to 2.0.0). This is because we now have to perform an extra transfer when burning and minting tokens in order to trigger any transfer hooks. This extra transfer in turn requires some additional accounts -- refer to the documentation in the
transfer_burn
andrelease_inbound_mint
functions for more details.New features
Also added an extra set of instructions to programmatically create/manage address lookup tables. These were necessary to add extra transaction size headroom for transfer hooks to be triggered. Without a lookup table, a transfer hook could use up to 2 custom accounts, but now there is space for 15+. See the documentation in
lut.rs
for more details.