-
Notifications
You must be signed in to change notification settings - Fork 510
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 Metadata v1.12 -- Initial Fees #1106
Conversation
Workflow |
Workflow |
Workflow |
Workflow |
token-metadata/program/src/processor/collection/set_collection_size.rs
Outdated
Show resolved
Hide resolved
if let Some(account) = md_account { | ||
assert_eq!(account.data.len(), 0); | ||
} |
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.
imo this should just use md_account.unwrap()
since you know it will still be open, but if you want to leave it as is to be flexible that's fine with me too, I just don't really like if let
without else
when we are talking verifying a test result.
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.
There are two situations here: one where the Metadata account is still open either because it has not been garbage collected yet, or because it still has fees on it. In that case we use the if let
to see if it has an account data length of 0. In the case where it's been garbage collected it's the None
case and the else is implicit. The only time to fail is if it exists and the account length isn't 0.
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.
Ok that is fine. I thought all the nonfungible accounts used in tests would now have uncollected fees on them from create
and remain open since the tests don't remove fees.
if let Some(account) = md_account { | ||
assert_eq!(account.data.len(), 0); | ||
} |
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.
Ok that is fine. I thought all the nonfungible accounts used in tests would now have uncollected fees on them from create
and remain open since the tests don't remove fees.
Token Metadata v1.12 Summary
Adds the system_program and sysvar_instruction accounts to all instructions that don't already have them to facilitate future fee-taking.
Levies fees on the following instructions:
"Print" commands, which create edition NFTs, will have the same fee applied but will be shipped after the pNFT unified
Print
command is implemented for consistency.Makes edition account mutable for the following instructions:freeze_delegated_accountthaw_delegated_accountMakes metadata account mutable for the following instructions:approve_collection_authorityrevoke_collection_authorityapprove_use_authorityrevoke_use_authorityWrites a Fee Flag to the last byte of the Metadata account or the second to last byte of the Master Edition account to indicate the account has fees to be collected.
Fees
Strategy
For most instructions, we need thesystem_program
account passed in so we can CPI into it and call the transfer function to move funds from a user wallet to one of our PDA accounts. We also include thesysvars_instruction
account so we can determine ordering of instructions in a transaction to try to avoid double charging users who use certain combinations of instructions in the same transaction. An example of this would be candy machine mint transactions which call bothcreate_metadata
andupdate_metadata
.Instead of using a fee treasury account, and passing it in, we are opting to store fees in metadata and edition accounts and harvesting them later with an automated service. The advantage of this is we already have these accounts passed into most instructions, and storing funds in them does not incur any additional write lock penalties, unlike using a single treasury account across multiple instructions. Even using multiple fee treasury accounts may not be sufficient, as a single instruction such as
set_and_verify
could be run thousands of times in a collection migration scenario. Multiple thousand item collections being migrated concurrently could result in an unacceptably severe write lock on the fee treasury account.Adding additional accounts is a breaking change, so the strategy here is to only collect fees on instructions that already havesystem_program
, which is only thecreate
instructions. We also can collect fees on burn instructions as they are a unique case, but have opted to do this later.For all the other instructions we wish to collect fees on, we add in the extra accounts now into the instructions, but do not use them in the actual processor handlers for the instructions. This keeps this on-chain version backwards compatible with other instructions but also gives users new instructions they can migrate to now, and which will be required for the next version when we start processing the instructions in the handlers on-chain.Accounts that already have the edition or metadata accounts passed in, but as readonly have these accounts changed to mutable in these new instructions but won't be used mutably on chain until the next version.Implementation
There's a new source file called
fees.rs
where most of the fee logic is contained.levy()
calculates the correct fee amount based on the instruction type (IxType
). Currently only the create instructions are implemented here. All future fee instructions that require a system transfer from a user's wallet should be added here.Charging fees then, is a simple matter of calling this function within the instruction handler and passing in the correct
IxType
and a few other parameters:The
burn_nft
andburn_print_edition
instructions have also had the additional accounts added for maximum future flexibility, though they shouldn't ever need them.