Better UX for defining query response types and schema generation #1339

uint · 2022-06-27T08:31:20Z

The goal here is to remove the following schema generation boilerplate for contract authors:

        [
            ("verifier", schema_for!(VerifierResponse)),
            ("other_balance", schema_for!(AllBalanceResponse)),
            ("recurse", schema_for!(RecurseResponse)),
            ("get_int", schema_for!(IntResponse)),
        ]
        .into_iter()
        .collect()

And replace it with attributes added to the QueryMsg itself:

#[derive(QueryResponses)]
pub enum QueryMsg {
    #[returns(VerifierResponse)]
    Verifier {},
    #[returns(AllBalanceResponse)]
    OtherBalance { address: String },
    #[returns(RecurseResponse)]
    Recurse { depth: u32, work: u32 },
    #[returns(IntResponse)]
    GetInt {},
}

The drawback right now is that cosmwasm_schema is imported as a regular (not dev) dependency in the contract.

Does this bloat the wasm blob? EDIT: nope, the size of the artifact is the same either way! 👌

contracts/hackatom/src/msg.rs

uint · 2022-06-27T12:19:04Z

Note: when merged to master, remember to add && (cd packages/schema-derive && cargo publish) \ to the Releasing wiki.

uint · 2022-06-28T08:00:21Z

TODO:

doc comments
tests for panic msgs when a contract dev forgets to annotate a QueryMsg variant
a macro or builder pattern for constructing an Api object by giving it types - also an opportunity to remove more schema.rs boilerplate

uint · 2022-06-29T06:42:58Z

Tagging @hashedone since this might be relevant to Sylvia in the future. I know Sylvia will replace the derive macro here, but I imagine we will keep the rest and get Sylvia to generate the QueryResponses trait implementation.

uint · 2022-06-29T07:02:56Z

I'm on the fence about how Api should be built here. I want to make that less fragile since contract devs still have to interact with this until they start using Sylvia. Ideas:

A (sort of) builder pattern:

// examples/schema.rs
fn main() {
    // ...
    let api = Api::builder()
        .name(env!("CARGO_PKG_NAME"))
        .version(env!("CARGO_PKG_VERSION"))
        .instantiate::<InstantiateMsg>()   // InstantiateMsg has to be JsonSchema
        .query::<QueryMsg>()               // QueryMsg has to be JsonSchema and QueryResponses
        .sudo::<SudoMsg>()
        .build();
}

Function-like macro:

// examples/schema.rs
fn main() {
    // ...
    generate_api! {
        instantiate = InstantiateMsg,
        query = QueryMsg,
        sudo = SudoMsg,
    };
}

I'm leaning toward the macro.

webmaster128

Very very nice stuff

packages/schema/src/idl.rs

packages/schema/src/query_response.rs

packages/schema-derive/src/lib.rs

uint · 2022-06-29T13:26:51Z

Rebased

Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>

webmaster128 · 2022-06-29T13:30:47Z

packages/schema-derive/src/lib.rs

+
+/// Extract the query -> response mapping out of an enum variant.
+fn parse_query(v: Variant) -> TokenStream {
+    let query = to_snake_case(&v.ident.to_string());


This assumes the user also sets rename_all = "snake_case". Is there a way for us to enforce that?

Our derive macro could panic if the #[serde(rename_all = "snake_case")] attribute is missing, I suppose. But we'd have to write (or borrow) code to parse them the same way serde parses them. I don't know if I like that.

For one thing, serde allows them to be chained like this:

#[serde(deny_unknown_fields, rename_all = "snake_case")]

I'll take a look at it either way.

Panic sounds good. The other option is to let QueryResponses do the serde configuration. Then users can remove the line and get the right (tm) thing from us.

So the problem is a derive macro cannot rewrite the enum or add attributes to it. It can only add extra code (a trait implementation usually).

We'd need to use an attribute macro and that feels more heavy-handed. Attribute macros consume the input and allow us to output a modified version of it.

Okay. But that means we can add extra code as part of response_schemas() that does a bit of serialization and check if snake case was used?

Huh. Maybe you're onto something. I think if we add a trait bound like this:

trait QueryResponses: JsonSchema { // ... }

then response_schemas() should be able to inspect the schema with something like this?

let schema = schema_for!(Self);

And maybe checking the schema for the presence of all the expected (snake_case) variants is a better integrity check than checking if a serde attribute is present? It sounds pretty good.

By the way, the parsing of serde attributes might not actually be hard. Serde exposes some internals for that.

Okay, so.

The derive macro checking for serde attributes is a no go - it can be done in theory, but requires serde_derive_internals, which then causes dependency resolution problems in contracts (another dep tries to pull an earlier version of it). It's probably best to stay away from that can of worms and just not risk dependency problems for the future there.

I'm going for the runtime integrity check described above since it just seems like a damn good idea to have. That throws an error during schema generation though (rather than a panicking macro during contract development), so pretty late.

If we add this, we're golden.

> cargo schema --- snip --- thread 'main' panicked at '{}: InconsistentQueries { query_msg: ["get-int", "other-balance", "recurse", "verifier"], responses: ["get_int", "other_balance", "recurse", "verifier"] }', examples/schema.rs:41:49 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

This reverts commit 7faefd3.

uint · 2022-07-05T19:49:16Z

The coverage report looks broken. @webmaster128 are you aware of a way to clean the cache, or do you have any other ideas?

webmaster128 · 2022-07-06T06:13:02Z

The coverage report looks broken. @webmaster128 are you aware of a way to clean the cache, or do you have any other ideas?

Looking at this diff coverage report, it seems like our coverage tool does not understand lines that are fields only:

uint · 2022-07-06T14:35:09Z

Looking at this diff coverage report, it seems like our coverage tool does not understand lines that are fields only

Weird!

uint · 2022-07-06T16:23:52Z

So, I'm also trying to reduce schema generation boilerplate to this:

use cosmwasm_schema::generate_api;

use hackatom::msg::{ExecuteMsg, InstantiateMsg, MigrateMsg, QueryMsg, SudoMsg};

fn main() {
    generate_api! {
        instantiate: InstantiateMsg,
        query: QueryMsg,
        execute: ExecuteMsg,
        sudo: SudoMsg,
        migrate: MigrateMsg,
    }
}

For contract devs, there's now no "open this file handle, write to that directory" boilerplate to maintain. Or fumbling with filenames and risking a typo. Just give it the types it needs and be done. How's that look?

uint · 2022-07-07T12:01:34Z

I think this is ready for review whenever someone has time. I know it's a fair bit; no rush.

I'm going to implement #1307 (comment) in the next PR.

webmaster128

Very nice!

Is there no hackatom.json in this PR? Why is CI not complaining?
I'm a bit unsure regarding naming. One "generate" macro create a struct, one "generate" writes a file to disk. What about generate_*/write_* to differentiate the two?

codecov.yml

packages/schema/src/idl.rs

packages/schema/src/query_response.rs

uint · 2022-07-11T10:35:31Z

Is there no hackatom.json in this PR? Why is CI not complaining?

I may have forgotten to commit hackatom.json. On it.

I'm a bit unsure regarding naming. One "generate" macro create a struct, one "generate" writes a file to disk. What about generate_*/write_* to differentiate the two?

Sounds like an improvement. Will do!

uint · 2022-07-11T10:44:11Z

Very nice!

Is there no hackatom.json in this PR? Why is CI not complaining?

I may have forgotten to commit hackatom.json. On it.

Apparently it is commited (see here), but doesn't show up in this PR's diff. Odd.

.circleci/config.yml

codecov.yml

webmaster128 · 2022-07-11T11:44:56Z

Apparently it is commited (see here), but doesn't show up in this PR's diff. Odd.

There is simply no diff to the version already available on main.

Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>

uint · 2022-07-11T13:38:42Z

There is simply no diff to the version already available on main.

Ahh, right. I forgot it was there already. If there's no diff, that's what I aimed for.

So answering your question, there is hackatom.json in this PR, but the format hasn't changed one iota.

uint changed the title ~~PoC: better UX for definit~~ PoC: better UX for defining query response types Jun 27, 2022

webmaster128 reviewed Jun 27, 2022

View reviewed changes

contracts/hackatom/src/msg.rs Outdated Show resolved Hide resolved

Base automatically changed from idl to main June 28, 2022 10:17

uint requested review from ethanfrey and hashedone June 29, 2022 06:38

uint requested a review from pyramation June 29, 2022 06:49

webmaster128 reviewed Jun 29, 2022

View reviewed changes

packages/schema/src/idl.rs Outdated Show resolved Hide resolved

packages/schema/src/query_response.rs Outdated Show resolved Hide resolved

packages/schema-derive/src/lib.rs Outdated Show resolved Hide resolved

uint added 9 commits June 29, 2022 15:26

schema: add a way to tie queries to responses

1be774c

schema: derive macro for QueryResponses

f793c24

schema-derive: don't gen trait when compiling to wasm

21b3ef3

schema-derive: fix query names in api file

27579ed

Update contracts

1612747

update codecov.yaml with schema-derive

94d1513

schema-derive: remove unnecessary dep

82d06d9

circleci: build schema-derive

e279d8a

schema-derive: add some tests

1a66e17

uint force-pushed the idl-ux branch from 35621a4 to 1a66e17 Compare June 29, 2022 13:26

schema-derive: style

a5605f6

Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>

webmaster128 reviewed Jun 29, 2022

View reviewed changes

uint added 6 commits June 29, 2022 15:43

schema: owned strings only for simplicity

a955db0

schema: rename query_responses -> response_schemas

b495a6c

schema: update IDL tests

4a3caa0

schema-derive: more tests

17b6308

schema-derive: verify rename_all = "snake_case" is used

7faefd3

Revert "schema-derive: verify rename_all = "snake_case" is used"

aebf859

This reverts commit 7faefd3.

schema: doc comment style

90d8355

schema-derive: panic msg tests

163a9d7

uint changed the title ~~PoC: better UX for defining query response types~~ Better UX for defining query response types and schema generation Jul 6, 2022

uint requested a review from webmaster128 July 6, 2022 16:26

uint marked this pull request as ready for review July 6, 2022 16:26

pyramation mentioned this pull request Jul 8, 2022

Use deny_unknown_fields and combine all query responses in one file CosmWasm/cw-plus#722

Closed

uint mentioned this pull request Jul 11, 2022

Implement msg macro to simplify configuring serialization #1345

Merged

webmaster128 reviewed Jul 11, 2022

View reviewed changes

codecov.yml Show resolved Hide resolved

packages/schema/src/idl.rs Show resolved Hide resolved

packages/schema/src/query_response.rs Show resolved Hide resolved

uint added 4 commits July 11, 2022 13:02

put schema-derive in separate codecov flag

88a8f59

schema: rename API generation macros for clarity

ce17cf4

format codecov.yml file

ff89d44

schema: fix failing doc test

da722a4

uint requested a review from webmaster128 July 11, 2022 11:16

webmaster128 reviewed Jul 11, 2022

View reviewed changes

.circleci/config.yml Show resolved Hide resolved

codecov.yml Outdated Show resolved Hide resolved

uint and others added 2 commits July 11, 2022 15:34

CI: run clippy for schema-derive

c203560

Update codecov.yml

4e0985a

Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>

webmaster128 approved these changes Jul 11, 2022

View reviewed changes

uint merged commit c42ded7 into main Jul 11, 2022

uint deleted the idl-ux branch July 11, 2022 14:40

webmaster128 mentioned this pull request Jul 12, 2022

Extend cosmwasm-schema to be able to annotate the resultant JsonSchemas with Parameter/ReturnType/State tags. (Or similar) #1276

Closed

uint mentioned this pull request Jul 20, 2022

contracts: move schema gen boilerplate to a binary crate CosmWasm/cw-plus#760

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better UX for defining query response types and schema generation #1339

Better UX for defining query response types and schema generation #1339

uint commented Jun 27, 2022 •

edited

Loading

uint commented Jun 27, 2022

uint commented Jun 28, 2022 •

edited

Loading

uint commented Jun 29, 2022

uint commented Jun 29, 2022

webmaster128 left a comment

uint commented Jun 29, 2022

webmaster128 Jun 29, 2022

uint Jun 29, 2022 •

edited

Loading

uint Jun 29, 2022

uint Jun 29, 2022

webmaster128 Jun 29, 2022

uint Jun 29, 2022

webmaster128 Jun 29, 2022

uint Jun 29, 2022 •

edited

Loading

uint Jul 4, 2022 •

edited

Loading

uint Jul 4, 2022

uint commented Jul 5, 2022 •

edited

Loading

webmaster128 commented Jul 6, 2022

uint commented Jul 6, 2022

uint commented Jul 6, 2022 •

edited

Loading

uint commented Jul 7, 2022

webmaster128 left a comment •

edited

Loading

uint commented Jul 11, 2022 •

edited

Loading

uint commented Jul 11, 2022

webmaster128 commented Jul 11, 2022

uint commented Jul 11, 2022

Better UX for defining query response types and schema generation #1339

Better UX for defining query response types and schema generation #1339

Conversation

uint commented Jun 27, 2022 • edited Loading

uint commented Jun 27, 2022

uint commented Jun 28, 2022 • edited Loading

uint commented Jun 29, 2022

uint commented Jun 29, 2022

webmaster128 left a comment

Choose a reason for hiding this comment

uint commented Jun 29, 2022

webmaster128 Jun 29, 2022

Choose a reason for hiding this comment

uint Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

uint Jun 29, 2022

Choose a reason for hiding this comment

uint Jun 29, 2022

Choose a reason for hiding this comment

webmaster128 Jun 29, 2022

Choose a reason for hiding this comment

uint Jun 29, 2022

Choose a reason for hiding this comment

webmaster128 Jun 29, 2022

Choose a reason for hiding this comment

uint Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

uint Jul 4, 2022 • edited Loading

Choose a reason for hiding this comment

uint Jul 4, 2022

Choose a reason for hiding this comment

uint commented Jul 5, 2022 • edited Loading

webmaster128 commented Jul 6, 2022

uint commented Jul 6, 2022

uint commented Jul 6, 2022 • edited Loading

uint commented Jul 7, 2022

webmaster128 left a comment • edited Loading

Choose a reason for hiding this comment

uint commented Jul 11, 2022 • edited Loading

uint commented Jul 11, 2022

webmaster128 commented Jul 11, 2022

uint commented Jul 11, 2022

uint commented Jun 27, 2022 •

edited

Loading

uint commented Jun 28, 2022 •

edited

Loading

uint Jun 29, 2022 •

edited

Loading

uint Jun 29, 2022 •

edited

Loading

uint Jul 4, 2022 •

edited

Loading

uint commented Jul 5, 2022 •

edited

Loading

uint commented Jul 6, 2022 •

edited

Loading

webmaster128 left a comment •

edited

Loading

uint commented Jul 11, 2022 •

edited

Loading