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

Future proof schema API #46

Closed
Tracked by #51
matklad opened this issue Nov 29, 2021 · 5 comments · Fixed by #165
Closed
Tracked by #51

Future proof schema API #46

matklad opened this issue Nov 29, 2021 · 5 comments · Fixed by #165
Assignees

Comments

@matklad
Copy link
Contributor

matklad commented Nov 29, 2021

Currently this is a public type:

#[derive(PartialEq, Debug, BorshSerialize, BorshDeserialize, BorshSchemaMacro)]
pub struct BorshSchemaContainer {
    /// Declaration of the type.
    pub declaration: Declaration,
    /// All definitions needed to deserialize the given type.
    pub definitions: HashMap<Declaration, Definition>,
}

This is ungreat -- all public fields make it impossible to make any kind of change to this structure. See #45 for example of the change we might want to do here (replacing HashMap with a BTreeMap to avoid hashbrown).

We should make it possible to change the internal representation without breaking public API, but that itself would necessitate a semver break.

@dj8yfo
Copy link
Collaborator

dj8yfo commented Jun 21, 2023

current issue is great detail on #51 's sub-issue

@dj8yfo
Copy link
Collaborator

dj8yfo commented Jun 23, 2023

@frol
Taking into account container ser/de guarantee https://github.com/dj8yfo/borsh-rs/blob/tmp_link_container_guarantee/borsh/tests/test_schema_enums.rs#L164,
the below link is a straightforward approach, which compiles, how this could be done, taking the above into account.

https://gist.github.com/dj8yfo/da468206a03feca6d08698cbfc868ea0

@frol
Copy link
Collaborator

frol commented Jun 26, 2023

@dj8yfo Do you mean canonicity enforcement (#160) when say "container ser/de guarantee"? Can you shed some light on why it matters here?

https://gist.github.com/dj8yfo/da468206a03feca6d08698cbfc868ea0#file-f_p_schema_api-rs-L139-L174 - This interface looks good to me (I would only ask not to shorten the names to "*_def", instead, use a full word get_definition)

@dj8yfo
Copy link
Collaborator

dj8yfo commented Jun 26, 2023

@frol No, i mean, that the container BorshSchemaContainer has some format of ser/de, which has been committed to in insta test.
It also has some BorshSchema implementation which is supposed to describe the format.

In gist i've removed derived implementations of BorshSerialize, BorshDeserialize for BorshSchemaContainer, as they depend on internal BorshSchemaContainer struct representation and replaced them with versions, that depend on declared public methods declaration() and definitions(), which are supposed to allow not changing the ser/de format, which has been committed to.

I missed to do the same for impl BorshSchema for BorshSchemaContainer in the gist at first, updated it now.

@dj8yfo
Copy link
Collaborator

dj8yfo commented Jun 26, 2023

taking this issue

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 a pull request may close this issue.

3 participants