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

chore!: split ValidationError from MaxSizeError; validate and max_serialized_size made BorshSchemaContainer's methods #219

Merged
merged 4 commits into from
Sep 15, 2023

Conversation

dj8yfo
Copy link
Collaborator

@dj8yfo dj8yfo commented Sep 13, 2023

  • summary of changes:
    • most of max_serialized_size code moved from borsh/src/schema_helpers.rs to borsh/src/schema/container_ext/max_size.rs
    • max_serialized_size made a method of BorshSchemaContainer
    • borsh::MaxSizeError moved to borsh::schema::SchemaMaxSerializedSizeError
    • SchemaMaxSerializedSizeError::MissingDefinition -> SchemaMaxSerializedSizeError::MissingDefinition(Declaration)
    • is_zero_size private function tries to detect recursion too, its return type changed to Result<bool, ()> (Err on recursive call)
    • MaxSizeError::ZSTSequenceNotArray check and enum variant split into separate types and module borsh/src/schema/container_ext/validate.rs
      • validate method of BorshSchemaContainer

split

pub enum MaxSizeError {
    Overflow,
    Recursive,
    MissingDefinition,
    ZSTSequenceNotArray,
}

into

pub enum MaxSizeError {
    Overflow,
    Recursive,
    MissingDefinition,
}

and

pub enum ValidateError {
    /// sequences of zero sized types of dynamic length are forbidden by definition
    /// see <https://github.com/near/borsh-rs/pull/202> and related ones
    ZSTSequence,
}

@dj8yfo
Copy link
Collaborator Author

dj8yfo commented Sep 13, 2023

a valid alternative might be to remove second commit and newly added module borsh/src/schema/container_ext/validate.rs,
and just remove MaxSizeError::ZSTSequenceNotArray variant and corresponding check

@dj8yfo dj8yfo force-pushed the split_validate_schema_method branch 7 times, most recently from 89b781c to d790ba4 Compare September 14, 2023 12:14
@dj8yfo dj8yfo marked this pull request as ready for review September 14, 2023 12:25
@dj8yfo dj8yfo requested a review from frol as a code owner September 14, 2023 12:25

let schema = BorshSchemaContainer::for_type::<RecursiveNoExitStructUnnamed>();
assert_eq!(Err(()), is_zero_size(schema.declaration(), &schema));
}
Copy link
Collaborator Author

@dj8yfo dj8yfo Sep 14, 2023

Choose a reason for hiding this comment

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

a similar test in current master was causing stack overflow

    #[test]
    fn test_is_zero_size_recursive_check_err() {
        use crate as borsh;

        #[derive(::borsh_derive::BorshSchema)]
        struct RecursiveNoExitStructUnnamed(Box<RecursiveNoExitStructUnnamed>);

        let schema = BorshSchemaContainer::for_type::<RecursiveNoExitStructUnnamed>();
        assert_eq!(false, is_zero_size(schema.declaration(), &schema));
    }
thread 'schema_helpers::tests::test_is_zero_size_recursive_check_err' has overflowed its stack
fatal runtime error: stack overflow

Though rustc prevents from constructing a type, which would hit a branch with recursing indefinetely into is_zero_size inside of max_serialized_size_impl:

error: values of the type `[[[RecursiveNoExitStructUnnamed2; 4294967295]; 2]; 4294967295]` are too big for the current architecture
// can be defined without error
// cannot be used as the value of type parameter in a function
struct RecursiveNoExitStructUnnamed2(Box<[[[RecursiveNoExitStructUnnamed2; u32::MAX as usize]; 2]; u32::MAX as usize]>);

borsh/src/lib.rs Outdated Show resolved Hide resolved
borsh/src/schema.rs Outdated Show resolved Hide resolved
…emaMaxSerializedSizeError::MissingDefinition`
Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

borsh/src/schema/container_ext/validate.rs Outdated Show resolved Hide resolved
@dj8yfo dj8yfo merged commit d22259a into master Sep 15, 2023
7 checks passed
@dj8yfo dj8yfo deleted the split_validate_schema_method branch September 15, 2023 07:29
@frol frol mentioned this pull request Sep 12, 2023
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