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

impl NearSchema derive macro #891

Merged
merged 28 commits into from
Nov 8, 2022
Merged

impl NearSchema derive macro #891

merged 28 commits into from
Nov 8, 2022

Conversation

miraclx
Copy link
Contributor

@miraclx miraclx commented Aug 17, 2022

use near_sdk::NearSchema;

#[derive(NearSchema)]
#[abi(json, borsh)]
struct Value {
    field: InnerValue
}

Effectively translates into

#[derive(schemars::JsonSchema, borsh::BorshSchema)]
struct Value {
    field: InnerValue
}

When compiled targeting WASM, this is a no-op.

@miraclx
Copy link
Contributor Author

miraclx commented Aug 17, 2022

One question though. Do we want to proxy attr macros like #[schemars], #[serde], #[validate] for schemars, and #[borsh_skip] for borsh?

@itegulov
Copy link
Contributor

Hmm, does this mean that all inner values have to implement both JsonSchema and BorshSchema regardless of which one is actually needed? This is one of the things I could not implement when I tried to do this: something like "if there is a derivation for BorshSerialize & BorshDeserialize then derive BorshSchema" (and also analogously for JsonSchema). Do you think this would be impossible to achieve?

@itegulov
Copy link
Contributor

One question though. Do we want to proxy attr macros like #[schemars], #[serde], #[validate] for schemars, and #[borsh_skip] for borsh?

schemars actually respects many serde attributes when deriving a schema, so yeah that would be nice. Same goes for borsh, it checks for borsh_skip when deriving BorshSchema.

@miraclx
Copy link
Contributor Author

miraclx commented Aug 17, 2022

"if there is a derivation for BorshSerialize & BorshDeserialize then derive BorshSchema" (and also analogously for JsonSchema). Do you think this would be impossible to achieve?

using a derive macro, unfortunately yes.

Best we can do is leave BorshSchema out of NearAbi since borsh is already reexported via near_sdk::borsh so devs can manually implement BorshSchema on the types they want.

@itegulov
Copy link
Contributor

Maybe we could derive JsonSchema by default and add something like this for borsh?

#[derive(NearAbi)]
#[abi(borsh)] // makes NearAbi derive `BorshSchema` instead of `JsonSchema`
struct Value {
    field: InnerValue
}

Does not seem like there is a great solution for this either way.

@austinabell
Copy link
Contributor

I agree with Daniyar, both should not be expected. Would require all inner types to have both implemented. Unclear if it would be better to have one derive with attributes or just two different derives. My intuition is that the latter seems more natural, but that is very weakly held.

The issue with your example, Daniyar, is that it limits the ability to derive both types. Yes, a very niche use case, but might be a bit of a footgun

@miraclx
Copy link
Contributor Author

miraclx commented Aug 18, 2022

limits the ability to derive both types

I actually think this might be possible, tested a small example, and it seems to work. Here's the syntax I'm working with:

#[derive(NearAbi)]
#[abi(json, borsh)]
struct Value {
    field: InnerValue
}

And omitting the attribute entirely defaults to JsonSchema.

I'll go ahead to implement this, I'll report here if I hit any blockers.

An alternative to the attribute syntax might be (if you prefer being explicit):
#[derive(NearAbi)]
#[abi(schema = "json")]
#[abi(schema = "borsh")]
struct Value {
    field: InnerValue
}

@miraclx
Copy link
Contributor Author

miraclx commented Aug 18, 2022

Done. So, I went with the first syntax.

#[derive(NearAbi)]
#[abi(json, borsh)]
struct Value {
    field: InnerValue
}
  • Defaults to implement JsonSchema when the attribute is missing.
  • #[abi(borsh)] implements BorshSchema.
  • #[abi(json, borsh)] implements both.

This already proxies schemars, serde attributes on the item. I'll follow this with proxying field and variant attributes as well as a test suite for guaranteeing stability.

In the meantime, let me know what you think.

near-sdk-macros/src/lib.rs Outdated Show resolved Hide resolved
near-sdk-macros/src/lib.rs Outdated Show resolved Hide resolved
near-sdk-macros/src/lib.rs Outdated Show resolved Hide resolved
near-sdk-macros/src/lib.rs Outdated Show resolved Hide resolved
near-sdk-macros/src/lib.rs Outdated Show resolved Hide resolved
near-sdk-macros/src/lib.rs Outdated Show resolved Hide resolved
near-sdk-macros/src/lib.rs Outdated Show resolved Hide resolved
@itegulov
Copy link
Contributor

Also would be cool to add some use cases of this macro to compilation_tests.

@miraclx miraclx changed the title impl NearAbi derive macro impl NearSchema derive macro Aug 23, 2022
TokenStream::from(quote! {
#[cfg(not(target_arch = "wasm32"))]
const _: () = {
mod __near_abi_private {
Copy link
Contributor

Choose a reason for hiding this comment

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

what does putting this in a mod do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NearSchema's expansion effectively adds derives to a clone of the structure.

#[derive(NearSchema)]
struct Value {
    field: InnerValue
}

expands into

struct Value {
    field: InnerValue
}

const _: () = {
    mod __near_abi_private {
        #[derive(schemars::JsonSchema)]
        struct Value {
            field: InnerValue
        }
        
        impl schemars::JsonSchema for super::Value { ... }
        //                            ^^^^^
        // disambiguates the cloned structure from the original one
    }
};

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, I see, so you want to avoid the type name being something different from the original for the purposes of schemars, which uses the type name or something?

Copy link
Contributor Author

@miraclx miraclx Aug 29, 2022

Choose a reason for hiding this comment

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

Yeah, borsh also uses the type name. Honestly, it's mostly because of Borsh. Depending on the structure, borsh uses the name quite extensively.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bump on this. super::* requires the type to exist in a module. And would fail in this case.

mod x {
    fn a() {
        #[derive(NearSchema)]
        pub struct MyType;
    }
}

super::MyType from the injected __near_abi_private module would reference x::MyType, which is nonexistent.

Copy link
Contributor Author

@miraclx miraclx Nov 7, 2022

Choose a reason for hiding this comment

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

Resolved to expanding to this instead:

struct Value {
    field: InnerValue
}

const _: () = {
    type Value__NEAR_SCHEMA_PROXY = Value;
    {
        #[derive(schemars::JsonSchema)]
        struct Value {
            field: InnerValue
        }
        
        impl schemars::JsonSchema for Value__NEAR_SCHEMA_PROXY { ... }
    }
};

near-sdk-macros/src/lib.rs Outdated Show resolved Hide resolved
@@ -5,6 +5,8 @@
#[cfg(test)]
extern crate quickcheck;

#[cfg(feature = "abi")]
Copy link
Contributor

Choose a reason for hiding this comment

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

can we put this export under unstable as well for now? Doesn't seem clear this will be more intuitive than deriving the traits directly yet and the internal semantics aren't tested yet.

@miraclx miraclx merged commit e37005b into master Nov 8, 2022
@miraclx miraclx deleted the miraclx/near-abi-derive branch November 8, 2022 15:18
@Sdsfghy

This comment was marked as off-topic.

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.

4 participants