Common validation functions #11
Conversation
| let mut approved = false; | ||
| match Asset::check_update() { | ||
| CheckResult::CanApprove => { | ||
| if asset.validate_update(ctx.accounts.authority)? == ValidationResult::Approved { | ||
| approved = true; | ||
| } | ||
| } | ||
| CheckResult::CanReject => return Err(MplCoreError::InvalidAuthority.into()), | ||
| CheckResult::None => (), | ||
| }; |
There was a problem hiding this comment.
Note this should have been a Collection check rather than Asset check. It is now fixed in the common function.
| // Check the collection plugins first. | ||
| ctx.accounts.collection.and_then(|collection_info| { | ||
| fetch_core_data::<Collection>(collection_info) | ||
| .map(|(_, _, registry)| { | ||
| registry.map(|r| { | ||
| r.check_registry(Key::Collection, PluginType::check_burn, &mut checks); | ||
| r | ||
| }) | ||
| }) | ||
| .ok()? | ||
| }); |
There was a problem hiding this comment.
Note this is a case of lost error propagation that was previously fixed in transfer, but still incorrect in burn. It is now fixed in the common function that both use.
| asset, | ||
| collection, | ||
| plugin_validate_fp, | ||
| )? || approved; |
There was a problem hiding this comment.
Sorry for necroposting.
Wouldn't approve variable in the second place prevent short-circuit evaluation? I'm not quite sure tbh, though seems it could.
There was a problem hiding this comment.
Yes, that's by design. Validations by plugins can both approve and deny an action (e.g. the Freeze plugin can reject a transfer because the token is frozen) so we always want to evaluate.
There was a problem hiding this comment.
I think its a good callout, such that a comment would be helpful to someone looking at the code. So I added one that basically explains this reason with pretty much the response above:
f088080
Summary
validate_asset_permissionsandvalidate_collection_permissions. Then within instructions can call with relevant function pointers:Notes
transfer,update,burnto check out the idea. Still needs to be done forcompressdecompressfor example.Optionfor an unusednew_ownerto the other validations.Benefits:
Drawbacks:
TransferArgsorBurnArgsin the validations. Currently it looks like the specific args have mostly been removed. They are still left over oncompress/decompressbut I don't think they would be used there once updated.Overall, we just need to consider if the validation function signatures/prototypes can look similar enough to enable the function pointer usage. I think they are now. Plus we could also change it back if too rigid.
Testing
JS tests pass locally.