token-2022: Add transfer fee types and instructions #2608
Conversation
|
All right, I think this covers all of the feedback. Thanks for taking such a quick / close look at all this! |
|
I've added in the mixin concept as just structs and instructions, let me know if this is clear or needs more detail. |
|
Ok, one more round of improvements! |
|
One more round! |
There was a problem hiding this comment.
I'm just down to pedantic nits, which means 
! This is all feeling pretty tidy to me
@CriesofCarrots - it'd be great if you're able to give this a final once over as well
There was a problem hiding this comment.
Awesome, looks good. I think the extension approach works well, and I didn't catch any fundamental issues with the design for fees and mint-closing; just added a few small comments/questions. (Apologies if some/all came up in previous threads! I wish I'd been following more closely)
| /// 1. `[writable]` The destination account. | ||
| /// 2. `[]` The mint's `fee_withdraw_authority`'s multisignature owner/delegate. | ||
| /// 3. ..3+M `[signer]` M signer accounts. | ||
| HarvestWithheldTokensFromMint, |
There was a problem hiding this comment.
I got confused here. Based on the doc and instruction name -- esp the word "Harvest" -- I thought this instruction was the same as HarvestWithheldTokensToMint, except it sends the tokens to an account instead of the mint. But since no source accounts are provided, I must be wrong.
Perhaps this should be WithdrawWithheldTokensFromMint?
There was a problem hiding this comment.
That makes a lot of sense, we use the term WithdrawWithheld everywhere else. The idea is there's one instruction to harvest from accounts to the mint, and another to withdraw from the mint to an account.
For better parallelization, we should probably also add a Withdraw instruction to withdraw straight from a bunch of accounts and into another one, which wouldn't require a writable mint. What do you think?
There was a problem hiding this comment.
Yeah I like that idea. The withdraw to mint operation is really just to facilitate emptying all the withheld tokens before closing an account. If we can assert that an account with 0 tokens will always have 0 withheld tokens then we could remove withdrawing to mint entirely
There was a problem hiding this comment.
Done.
If we can assert that an account with 0 tokens will always have 0 withheld tokens then we could remove withdrawing to mint entirely
I can't come up with a model that guarantees this 🤔
There was a problem hiding this comment.
Excellent, these changes allay all my confusion :)
I can't come up with a model that guarantees this 🤔
I was going to suggest moving the sender's withheld_amount to the destination account, but I think I just duplicated the same thought process you two already did here
| decimals: u8, | ||
| /// Expected fee assessed on this transfer, calculated off-chain based on | ||
| /// the transfer_fee_basis_points and maximum_fee of the mint. | ||
| fee: u64, |
There was a problem hiding this comment.
If the mint doesn't include an MintTransferFee extension, should we still allow this instruction to be used but the fee must be 0?
There was a problem hiding this comment.
Seems reasonable to me
joncinque
changed the title
mergify
bot
dismissed stale reviews from mvines and CriesofCarrots
Pull request has been modified.
|
Merging this round of changes so we have a base to iterate on, thanks for reviews! |
Problem
It isn't possible to assess fees on token transfers.
Solution
Add new account types for Mints / Accounts with transfer fees, along with an over-type to handle any type, see
UberMintfor more info.Add instructions for initializing mints with transfer fees, setting fees, transferring tokens, and transferring withheld tokens.