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

feat: Add support for serde_json::Value #312

Closed
wants to merge 3 commits into from

Conversation

larry0x
Copy link

@larry0x larry0x commented Oct 3, 2024

Implement BorshSerialize and BorshDeserialize for serde_json::Value.

This is useful, for example, if a program is to take an arbitrary JSON as input (as Value), and store it as raw bytes.

We found that for this use case, borsh produces much more compact bytes, is faster, and results in smaller program size, compared to using JSON strings.

Thank you!

@larry0x larry0x requested review from frol and dj8yfo as code owners October 3, 2024 01:38
@larry0x
Copy link
Author

larry0x commented Oct 3, 2024

Some workflows fail, but they don't seem to be related to my code...

Copy link
Collaborator

@dj8yfo dj8yfo left a comment

Choose a reason for hiding this comment

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

@larry0x i've considered adding this to lib and complementing it with BorshSchema impl
(#314),

but than i took a closer look at serde_json features
image
vs ascii crate features, support for which has been accepted earlier
image

which made me realize, that serde_json::Value really qualifies for a high-level type
versus relatively straightforward low-level types from ascii crate,
as e.g. serde_json has arbitrary_precision/raw_value non-default features, which
change the way Value is serialized and affect corresoponding borsh impl-s too.

Therefore i've decided to alter pr to bridge BorshSerialize/BorshDeserialize support to 3rd-party
serde_json::Value in context of a specific application with specific selected serde_json features,
rather than provide limited support to default-only serde_json::Value in the library without really taking into
account any of its intricacies.

Line wise implementation for 3rd party isn't much longer than change of this pr, notable differences being:
https://github.com/dj8yfo/borsh-rs/blob/serde_json_value_examples/borsh/examples/serde_json_value.rs#L72-L81
https://github.com/dj8yfo/borsh-rs/blob/serde_json_value_examples/borsh/examples/serde_json_value.rs#L199-L213
https://github.com/dj8yfo/borsh-rs/blob/serde_json_value_examples/borsh/examples/serde_json_value.rs#L108-L111
https://github.com/dj8yfo/borsh-rs/blob/serde_json_value_examples/borsh/examples/serde_json_value.rs#L182-L191

Also i've commemorated your code in borsh's crate runnable examples and linked to it from README and documentation.

suggested #317 as an alternative instead of this pr

@frol
Copy link
Collaborator

frol commented Oct 30, 2024

Closing in favor of #317

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.

3 participants