Refactor metadata validation #1974
Conversation
|
Docker tags |
Benchmark for 65eb264Click to view benchmark
|
iamyulong
changed the title
iamyulong
force-pushed
the
feature/url-validation-cost
branch
from
79eb355
to
4dffd44
Compare
| let blueprint_name = METADATA_BLUEPRINT; | ||
| let function_name = METADATA_CREATE_WITH_DATA_IDENT; | ||
|
|
||
| /* |
There was a problem hiding this comment.
Is this commented-out code intended to be deleted? :)
There was a problem hiding this comment.
Ahh no it's explaining how the below was generated, my bad! All good.
Perhaps we could have a comment here briefly explaining that? But only if you feel like it, no worries, I was just being slow.
| b.iter(|| { | ||
| black_box( | ||
| CheckedUrl::of("https://www.example.com/test?q=x") | ||
| .ok_or(MetadataValueValidationError::InvalidURL("".to_owned())), |
There was a problem hiding this comment.
MINOR: This is always, valid, right? So we should probably just unwrap() here - otherwise greedy allocation of a string might slow down the test a little.
| ValueDecodeError(DecodeError), | ||
| MetadataValidationError(MetadataValidationError), | ||
| MetadataKeyValidationError(MetadataKeyValidationError), |
There was a problem hiding this comment.
This will require temporarily using , regenerate to update the VersionedLocalTransactionExecution.
| @@ -16,10 +16,9 @@ use super::{RemoveMetadataEvent, SetMetadataEvent}; | |||
|
|
|||
| #[derive(Debug, Clone, Eq, PartialEq, ScryptoSbor)] | |||
| pub enum MetadataError { | |||
| KeyStringExceedsMaxLength { max: usize, actual: usize }, | |||
There was a problem hiding this comment.
We can't remove these error variants as it breaks the error deserialization of historic receipts in the node (until we fix that to store historic schemas - which I really want to do one day soon ;))
Even if they're unused, we need to keep them for now I'm afraid.
There was a problem hiding this comment.
You're right. Thought we had that error to string conversion already.
There was a problem hiding this comment.
We do have error to string conversion, but to save space, it happens on the API at response serving time; and uses the current RuntimeError. I'd like to store some historic schemas at each protocol update in the node so we can convert with these old schemas whilst still updating the errors.
| pub fn init_system_struct( | ||
| data: MetadataInit, | ||
| data: IndexMap<Vec<u8>, (Option<Vec<u8>>, bool)>, |
There was a problem hiding this comment.
How confident are we that these refactors are 100% backwards-compatible across Cuttlefish in terms of protocol-visible changes? e.g. we're changing which errors we return (which I think is safe I think assuming we only error in the same cases). And e.g. we aren't making any sys-calls/billing in the middle, which seems safe?
I see we've replaced some encode unwraps with e.g. MetadataKeyValidationError::InvalidValue - I guess we don't actually assume this to be hit, and indeed if it unwrapped previously, it shouldn't introduce any issues?
There was a problem hiding this comment.
How confident are we that these refactors are 100% backwards-compatible across Cuttlefish in terms of protocol-visible changes
I did check this thoroughly. As far I can tell, there is only error code difference which should not affect consensus, but would appreciate another pair of eyes.
I see we've replaced some encode unwraps with e.g. MetadataKeyValidationError::InvalidValue - I guess we don't actually assume this to be hit, and indeed if it unwrapped previously, it shouldn't introduce any issues?
This is right, considering MetadataInit often comes from an sbor-encoded inputs.
Keeping it in the validation logic so it's self-contained and no need to make assumption about sbor-able.
There was a problem hiding this comment.
Sounds great. And yes, I agree that I think it's safe, so I'm happy with this.
| @@ -262,8 +261,9 @@ impl MetadataNativePackage { | |||
| Ok(Own(node_id)) | |||
| } | |||
|
|
|||
| /// This method assumes that the data has been pre-checked. | |||
There was a problem hiding this comment.
Can we perhaps make it not pub?
Perhaps we should make it not pub and rename it to init_system_struct_from_pre_validated_data or something?
And then we can re-expose some higher-level pub method which validates and then inits?
There was a problem hiding this comment.
Will make it non-public.
Summary
Refactor metadata validation, particularly checking length before validating URLs.