feat: borsh and json schemas support #43
Conversation
dzmitry-lahoda
changed the title
|
ping @danlehmann. |
|
Thanks for the contribution! I'll work on reviewing this (have to learn about borsh first). Will try to get back asap |
|
So what I saw from impl. It is hard to get le bytes of number like u35. Really I can borsh it into 5 bytes. While doing it for 8 now. So support getting LE bytes minimally needed would be awesome, not underlying. |
Cargo.toml
Outdated
| [dependencies] | ||
| num-traits = { version = "0.2.17", default-features = false, optional = true } | ||
| defmt = { version = "0.3.5", optional = true } | ||
| serde = { version = "1.0", optional = true, default-features = false} | ||
| borsh = { version = "1.5.1", optional = true, features = ["unstable__schema"], default-features = false} |
There was a problem hiding this comment.
Do you see a way to do this without relying on an unstable feature? I could see this break in the future
There was a problem hiding this comment.
I could see this break in the future
I can add it as separate feature for BorshSchema?
Problem with borsh is next. Creators of Borsh already use this feature in production. And we too. BorshSchema does not requires nightly, it is just state of thing.
Why I do and have to add it? Because BorshShema is supported for u8/u16/u32/.../i8/... . So in order to replace any number with arbitrary, it to be supported. So what I say is that in our and NEAR chain scenarios, in order to replace Rust native numbers with arbitrary we need Schema.
There was a problem hiding this comment.
TLDR;
Assuming everyday will want to plug arbitrary int instead of existing int with highest possible fidelity,
it means that if serde/schema/etc crate does something to native primitive int,
than it must do same for arbitrary-int.
Arbitrary int should blend into like if they are native primitives.
| let mut bytes = 0; | ||
| let mask: T = u8::MAX.into(); | ||
| while bytes < length { | ||
| let le_byte: u8 = ((value >> (bytes << 3)) & mask) |
There was a problem hiding this comment.
This can be simplified:
let le_bytes = (value >> (bytes << 3)) as u8;
No need for the expect, try_into() etc
There was a problem hiding this comment.
value is not primitive, so 'as' does not work as i tried
There was a problem hiding this comment.
could use https://docs.rs/num/latest/num/traits/trait.AsPrimitive.html , but do not want to force num traits usage
| assert_eq!(buf.len(), (u63::BITS + 7) / 8); | ||
| assert_eq!(input, output); | ||
|
|
||
| let schema = BorshSchemaContainer::for_type::<u9>(); |
There was a problem hiding this comment.
just to understand this: BorschSchemaContainer does NOT require schemars? So we have two different things that are called schema?
There was a problem hiding this comment.
One schema is JSON/OpenAPI shema integrated with serde.
Other shema is borshschema integrated with borsh.
There are schemas for protobuf/scale/etc.
There was a problem hiding this comment.
do not require, just supporting several schemas
|
@danlehmann about tests. I tested that arbitrary ints work same as native primitive ints, minus some well know diff. |
|
I'd like to have full binary tests for these. Right now, there are many incorrect implementations that would pass those tests. However, we can merge and I'll work on the test in a follow-up. Please give me a day or two |
|
This patch didn't actually build (see https://github.com/danlehmann/arbitrary-int/actions/runs/10065181794/job/27823714420) Working on a fix now |
|
Reworked things somewhat: #45 |
|
@dzmitry-lahoda Please take a look at #45 . That adds a lot of test coverage, removes the allocation in deserialize and simplifies serialize. Review appreciated. |
No description provided.