Permissioned burn extension #818
Conversation
|
@joncinque Would really appreciate a quick review, just to confirm that I’m moving in the right direction before adding the tests. |
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
Thanks for your contribution! This is definitely on the right track, but I think we'll need a bit more functionality. Let me know what you think!
|
@joncinque I would need a bit of guidance on adding tests for the permissioned burn. I see tests in rust-legacy and js-legacy, but since they’re marked legacy I’m unsure of the current convention. Where should new tests go? |
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
I see tests in rust-legacy and js-legacy, but since they’re marked legacy I’m unsure of the current convention. Where should new tests go?
Thankfully, legacy doesn't mean deprecated, so please add the tests to the rust-legacy client 😄
This is on the right track, keep it up! Let me know if you need anything else on my side, apologies for the slowness on the re-review
|
@joncinque Everything should be resolved now :) |
joncinque
left a comment
joncinque
left a comment
There was a problem hiding this comment.
This looks really good! Mostly nits, and a question about what to do if the authority is set to None.
We'll eventually need to add the ability to specify a burn authority in the CLI and rust-legacy clients, but that can happen later
| // Standard burns cannot be used when the permissioned burn | ||
| // extension is present. | ||
| if permissioned_ext.is_ok() { | ||
| return Err(TokenError::InvalidInstruction.into()); | ||
| } |
There was a problem hiding this comment.
This is a good question actually -- do we allow a standard burn if the authority is set to None?
It's an edge case, but there's no reason to prohibit it, and those tokens will be impossible to burn otherwise. Unless that's the exact behavior that we want -- if the authority is None, it means that tokens are unburnable.
@gitteri what do you think?
There was a problem hiding this comment.
Good catch, here is my take on this:
We require no permission for burns when set to None. In case the authority wants to disallow burns they would be able to set the address to the zero address.
This would require changing this type to allow the zero address: https://github.com/solana-program/token-2022/pull/818/files#diff-5731894f298eedfe845a6e732182028c623e97fe6c6adb868f067d7e5d67a095R19
The only downside is that this can be considered a bit of a hacky solution, so I’m open to hearing your opinions.
| // Pull the required extra signer from the accounts | ||
| let approver_ai = next_account_info(account_info_iter)?; | ||
|
|
||
| if !approver_ai.is_signer { | ||
| return Err(ProgramError::MissingRequiredSignature); | ||
| } | ||
|
|
||
| let maybe_burn_authority: Option<Pubkey> = ext.authority.into(); | ||
| if Some(*approver_ai.key) != maybe_burn_authority { | ||
| return Err(ProgramError::InvalidAccountData); | ||
| } |
There was a problem hiding this comment.
Depending on what we decide for standard burns when the authority is set to None, we can print a log saying to use the standard burn and then return an error.
|
I kicked off CI, and there are a few little things to fix:
Let me know if you need any help with these pieces 😄 |
2 participants
This PR implements a permissioned-burn extension as described in #772
Summary
Introduces the
PermissionedBurnmint extension, enabling bothburnandburn_checkedoperations that require approval from a designated authority configured during mint initialization.When this extension is enabled, standard
burnandburn_checkedinstructions are disallowed for the mint. This PR does not modify the existing behavior of regular burns for mints that do not use the extension.TODO:
Closes #772