Fix to Issue #27 - Metadata

[L0STE]

Preventing Overwrites on Creators and Royalties

I assumed we should only allow public keys for the "Creators" field. To enforce this, I set up an add_metadata function. It checks if any fields passed are public keys and errors out if they are. This way, we ensure that the only way to add metadata (aside from royalty data) is through this controlled process, safeguarding against overwrites.

for metadata_arg in args {
    // Check if the field is a public key
    match Pubkey::from_str(&metadata_arg.field) {
        Ok(_) => {
            return Err(MetadataErrors::InvalidField.into());
        },
        Err(_) => {
            ctx.accounts.update_token_metadata_field(Field::Key(metadata_arg.field), metadata_arg.value.to_string())?;
        }
    }
}

Updating Creators

Instead of separate instructions for adding and removing creators, I took advantage of the way the update_metadata instruction works: it either creates a new field (if it doesn't exist) or updates an existing one.

To remove creators, I used a combo of remove_key instruction and a custom check against the current metadata. If a creator isn't in the new list, we remove them.

// Identify and remove creators no longer needed
let keys_to_remove: Vec<String> = metadata
    .additional_metadata
    .iter()
    .filter_map(|(key, _)| match Pubkey::from_str(key) {
        Ok(pubkey) => {
            if !creators_arg.iter().any(|creator| creator.address == pubkey.to_string()) {
                Some(key.clone())
            } else {
                None
            }
        },
        Err(_) => None,
    })
    .collect();

for key in keys_to_remove {
    ctx.accounts.remove_token_metadata_field(key)?;
}

_Note: For the removekey instruction I'm sticking with creating it raw for now. Let me know if you think we should add a macro for it in the BrisgeSplit Repo

Problem created by this instruction: Account Lamport Adjustments Ran into an issue where update_account_lamports_to_minimum_balance doesn't account for reductions in account space. I whipped up update_account_lamports_to_minimum_balance2 to handle cases where the space decreases. It attempts to refund lamports, but since the Mint needs to sign, this might not fly in production. Might need to include this check in all instructions since we could have this problem even if the instruction doesn't decrease the space in that exact instruction.

Updated the distribution instruction to only search for creators when issuing payments When distributing payments, I made sure we only consider valid creator public keys. Any invalid keys are ignored, simplifying the process.

.filter_map(|(key, value)| match Pubkey::from_str(key) {
    Ok(pubkey) => Some(CreatorShare {
        address: pubkey,
        pct: u8::from_str(value).unwrap(),
    }),
    Err(_) => None,
})
.collect::<Vec<CreatorShare>>();

Add validation to Add Royalties to ensure all creators are valid pub keys Added a quick check to ensure all creators are valid public keys before adding royalties. This catches any invalid keys early on.

for creator in args.creators {
    // Ensure each creator has a valid public key
    match Pubkey::from_str(&creator.address) {
        Ok(_) => {
            total_share = total_share
            .checked_add(creator.share)
            .ok_or(MetadataErrors::CreatorShareInvalid)?;
            ctx.accounts
            .update_token_metadata_field(Field::Key(creator.address), creator.share.to_string())?;
        },
        Err(_) => return Err(MetadataErrors::CreatorAddressInvalid.into()),
    }
}

I also added a sanity check to make sure the royalty fee basis points don't exceed 10000, preventing future errors.

require!(args.royalty_basis_points <= 10000, MetadataErrors::RoyaltyBasisPointsInvalid);

Conclusion Let me know your thoughts or if there's anything else i should tweak! I left the test folder to simplifying the process of testing the implementation i did. Once it's tested could be left out of the merge.

Once it's all good and ready to merge i'll create the example for the new instruction i created!

[L0STE]

I pushed the remove_metadata instruction. If you guys want i could create something similar to the modify for creators for metadata (so merging add and remove and make sure that there is only one struct for Royalties and one for Metadata instead of just passing in the values)

[balmy-gazebo]

Yes I think this is a good idea, if a user tries to remove a pub key or the ROYALTY_BASIS_POINTS reserved key it should fail using this new IX. And the dedicated remove creator IX should make sure royalty shares still add up to 100. If the user want's to get rid of all royalties, we'll need to remove all pub key fields and basis points field and reset the transfer hook, though not sure what to set it to in that case.

[balmy-gazebo]

@L0STE see above

[L0STE]

Yes I think this is a good idea, if a user tries to remove a pub key or the ROYALTY_BASIS_POINTS reserved key it should fail using this new IX. And the dedicated remove creator IX should make sure royalty shares still add up to 100. If the user want's to get rid of all royalties, we'll need to remove all pub key fields and basis points field and reset the transfer hook, though not sure what to set it to in that case.

So this is already what is happening:

• remove_metadata make sure that the arg passed in are not Pubkeys so the user doesn't have the possibility of removing Creators with that instruction.
• removing/adding creator instruction always make sure that the total share adds up to 100%.

What i was referring is just to leverage the same way the modify instruction work in royalties. Instead of adding/removing single fields we can always make sure that the additional_metadata is the args passed in and use a single instruction called modify_metadata.

For the last point I can create another instruction that just remove creators (or leverage the modify instruction to check if the arg is empty or not -> if it is, remove the transfer hook and set it to Pubkey::default()).

@balmy-gazebo

[balmy-gazebo]

Yes let's do that, just do a modify creators instruction for both add and remove, and then a remove royalties IX that nullifies the transfer hook. Is setting to pub key default the best way?

[balmy-gazebo]

@L0STE tag when done