CosmWasm / cosmwasm

Framework for building smart contracts in Wasm for the Cosmos SDK
https://www.cosmwasm.com/
Apache License 2.0
1.07k stars 336 forks source link

Remove additional properties in schema generation #1307

Closed ethanfrey closed 2 years ago

ethanfrey commented 2 years ago

This is a bit annoying with the generated typescript types, with {[k: string]: unknown}.

We can turn it off by adding additionalProperties: false in the json schema we export.

ethanfrey commented 2 years ago

It seems you can accomplish this with.. #[serde(deny_unknown_fields)]: https://github.com/GREsau/schemars/blob/master/CHANGELOG.md#074---2020-05-16

webmaster128 commented 2 years ago

additionalProperties is used already. It came with some upgrade of schemars many months ago.

Where exactly do you see the problem?

ethanfrey commented 2 years ago

Basically, it works if you just use:

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
#[serde(deny_unknown_fields)]
pub struct MigrateMsg {
    pub payout: String,
}
webmaster128 commented 2 years ago

I see. Then we only have "additionalProperties": false for the enums now.

ethanfrey commented 2 years ago

Yes, but only on the top level, not inside each variant. Or the results.

ethanfrey commented 2 years ago

@JakeHartnell check this out

pyramation commented 2 years ago

Should we start using #[serde(deny_unknown_fields)] in other contracts or should we explore other ways of setting the additional properties field to false?

I'm not a rust dev, but I'm still curious — is it possible to make a macro for the macro?

#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
#[serde(deny_unknown_fields)]

becomes super simplified, something like

#[cosmjson(JsonSchema)]

which would expand into the stuff it needs. I guess it could also give us flexibility later to change whatever the macro does in case we create new methods for packaging contracts.

webmaster128 commented 2 years ago

Should we start using #[serde(deny_unknown_fields)] in other contracts

I think this is fine for now. It will disallow additional fields during deserialization as a side effect which might even be helpful to detect wrong JSON requests sent by the client.

A custom macro would be more work, also not trivial because of interactions with other macros. We can go that route mid term for sure but it will not be quick. Good to know what we need before.

iboss-ptk commented 2 years ago

@webmaster128 shouldn't this be simple as like

#[proc_macro_attribute]
pub fn cosmwasm_schema(_metadata: proc_macro::TokenStream, input: proc_macro::TokenStream)
                 -> proc_macro::TokenStream {
    let input: TokenStream = input.into();
    let output = quote! {
        #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, JsonSchema)]
        #[serde(deny_unknown_fields)]
        #input
    };
    output.into()
}
#[cosmwasm_schema]
struct QuerySomething { .. }

utilizing attribute macro to modify derives.

webmaster128 commented 2 years ago

@uint could you check out this suggestion? 👆

iboss-ptk commented 2 years ago

And with attribute macro, we should theoretically, do things like

#[cosmwasm_schema_mod]
mod msg {
  #[cosmwasm_schema]
  pub enum QueryMsg { .. }
}

That is generate single export_schema command so that whenever new type (mostly response type) being introduced, there is no need to keep adding lines in schema.rs.

uint commented 2 years ago

@uint could you check out this suggestion? point_up_2

Yeah, I tried to avoid attribute macros since derives seem less "invasive", but they also complicate things for us implementation-wise. I think we should go for the attribute macro.

I don't know if I'd go so far as to create an attribute macro for the whole module. At that point we're duplicating the work Sylvia is already doing and we should probably focus on releasing Sylvia faster instead.

uint commented 2 years ago

@iboss-ptk I started a PR that implements your suggestion: #1345