Skip to content
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

burn-transfer-for-compressed-nfts #4

Closed
wants to merge 3 commits into from

Conversation

kstepanovdev
Copy link
Contributor

add delegates checking for compressed nfts within both burn and transfer functions

@kstepanovdev kstepanovdev self-assigned this Mar 2, 2024
@kstepanovdev
Copy link
Contributor Author

@danenbm I tried to reduce cognitive complexity (though without making methods more modular) and it seems like I made it more complex. wdyt?
I'd suggest leaving it as is with a tech debt to rewrite it when we know whether we really need those methods over hashed assets.

Comment on lines +40 to +42
if ctx.accounts.authority.key != &asset.owner {
return Err(MplCoreError::InvalidAuthority.into());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm thinking is that we want to have it be consistent across all instructions so like we do asset.check_burn(), then asset.validate_burn().... later plugin.check_burn(), then plugin.validate_burn(). And in any other ix call the same kind of check/validate methods.

But let me have a quick chat with @blockiosaurus tomorrow about the permissions model, since I have heard some talk about changes so we might have a different way to go about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok unfortunately there's been more significant changes in main due to collection permissions, and I have some other suggestions for the team for the new way it works. I think maybe we need to pair program this tomorrow. We can discuss in standup.

@danenbm
Copy link
Contributor

danenbm commented Mar 11, 2024

This has been covered by: #12

@danenbm danenbm closed this Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants