feat: add proper support for borsh schema containers in ABI #872
Conversation
| }, | ||
| Struct { | ||
| #[serde(with = "FieldsDef")] | ||
| fields: Fields, |
There was a problem hiding this comment.
The fields key feels redundant. Is there a reason this and others can't be inlined to avoid unnecessary data in abi?
There was a problem hiding this comment.
I am just following borsh-rs here: https://github.com/near/borsh-rs/blob/7325a0aab74049237b9f978e1e2c0fcf7bb799c2/borsh/src/schema.rs#L46
The structs need to be duplicated and match the other struct one for one so I can use serde's remote derivation (see https://serde.rs/remote-derive.html).
examples/abi/res/abi.json
Outdated
| "Struct": { | ||
| "fields": { | ||
| "UnnamedFields": [ | ||
| "u32", | ||
| "u32" | ||
| ] | ||
| } | ||
| } |
There was a problem hiding this comment.
Intuitively, feels like this should be
| "Struct": { | |
| "fields": { | |
| "UnnamedFields": [ | |
| "u32", | |
| "u32" | |
| ] | |
| } | |
| } | |
| "Struct": [ | |
| "u32", | |
| "u32" | |
| ] |
And I can't see why you wouldn't be able to do this because the struct is either a JSON map, an array, or empty (empty map or null, idk what it's used for), am I missing something?
There was a problem hiding this comment.
Hmm yeah that's completely fair. I will see if I can use remote derivation with custom serde attributes to control the format a bit more.
There was a problem hiding this comment.
Done! This introduced even more complexity to the code, but at least it seems to work. This definitely needs some tests though, will add them next week.
There was a problem hiding this comment.
The workarounds seem a bit unnecessary since we can modify borsh to include clone and update the schema format, but this seems fine for now to unlock the functionality.
Can you open an issue in borsh identifying how we want the serde serialization format to look, so we can get some input before making any changes to avoid the extra logic here? I opened near/borsh-rs#97 because it was unopinionated
|
I finished tests and this seems to behave as I expect it to, so I will merge this. @austinabell created the serde derivation issue in borsh repo: near/borsh-rs#98 |
The main problem with this approach is that all schema containers are independent of each other. This can make the ABI files bloated when working with many complex types. Unfortunately, it seems like
borsh-rsdoes not have a generator model likeschemarsdoes, so I can't generate one big schema like I do with JSON Schema. Maybe this is something that would make sense to add to borsh-rs in the future though?Related to near/cargo-near#14