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

contracts: move schema gen boilerplate to a binary crate #760

Merged
merged 1 commit into from
Aug 15, 2022

Conversation

uint
Copy link
Contributor

@uint uint commented Jul 20, 2022

Closes #755

This should make the layout of a contract a bit less confusing. How's this look?

Copy link
Contributor

@hashedone hashedone left a comment

Choose a reason for hiding this comment

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

I have one problem with this approach, and it's serious - see the comment. It obviously applies to all instances of this dependency.

@@ -19,6 +19,7 @@ library = []
test-utils = []

[dependencies]
cosmwasm-schema = { version = "1.0.0" }
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want this in a normal build. Technically it would probably get optimized out, but still would prolong compilation and is not wanted there in general. The workaround for this is probably to:

  • make this dependency optional
  • add a feature schema-bin enabling this dependency
  • add a [[bin]] section like:
[[bin]]
name = "schema"
path = "src/bin/main.rs"
required-features = ["schema-bin"]

This actually might be a good reason to extract the dependency to examples... I mean it still seems better as a binary, but it requires more boilerplate in Cargo.toml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It definitely gets optimized out - I tested it before.

This change is coming anyway. I already moved the schema dependency to regular deps in cosmwasm-std 1.1.0 contracts in this PR: CosmWasm/cosmwasm#1339 I also mentioned this problem in the PR, but didn't receive feedback against it or suggested solutions.

I know this could still be changed by adding a feature flag and doing something like:

#[cfg_attr(feature = "schema", derive(QueryResponses))]
pub enum QueryMsg {
    #[cfg_attr(feature = "schema", returns(VerifierResponse))]
    Verifier {},
    ...
}

But I'm not sure I'm really sold on this. Is the extra complexity worth better compilation times?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am mostly worried about some garbage left in the output Wasm binary so it increases in size. Did you check if wasm binary size after this change is not affected? If so then build times are secondary and it's mergeable. I am not happy about increasing complexity of dependency tree, but also its not a huge problem (at least yet).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIR, I checked the size of the Wasm binary after running it through rust-optimizer and it was identical to main, but I'm happy to double-check it before merging this.

@uint
Copy link
Contributor Author

uint commented Jul 20, 2022

@hashedone For the cosmwasm-vm stuff (check_contract, module_size, etc), I'm tempted to set up a new package called cosmwasm-toolbox and move all of them there as binary crates. That way we avoid extra dependencies in cosmwasm-vm, but also avoid adding more feature flags, right? But that's a separate PR for another time.

@hashedone
Copy link
Contributor

@uint I would personally create a separate package for each of those utils, so if we publish them you just do cargo install cw-check-contract or smthng. Also integration with cargo might be a thing (like calling cargo check_contract to ensure build wasm binary and check it.

@@ -3,4 +3,4 @@ wasm = "build --release --target wasm32-unknown-unknown"
wasm-debug = "build --target wasm32-unknown-unknown"
unit-test = "test --lib"
integration-test = "test --test integration"
schema = "run --example schema"
schema = "run --bin schema"
Copy link
Member

Choose a reason for hiding this comment

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

In the end, the only real effect is changing --example to --bin?
Or does this have other positive improvements? Or needed as a future building block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it also removes the schema gen code from the 'examples' directory so that a person new to CosmWasm contracts doesn't have to wonder "what the hell?" when looking at the source of one and trying to figure out what's what.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Recently Dan mentioned this seemed odd and asked if we can rename the examples directory. When I first looked at how schemas are generated in contracts, I also had a "what the hell" moment.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, no blocker my side. Just seemed odd. But I guess I made this weird setup and an used to it.

If it makes CosmWasm users happy, let's do it.

Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

LGTM. I guess we'll have to change this in all the other repos for consistency.

@uint
Copy link
Contributor Author

uint commented Jul 22, 2022

LGTM. I guess we'll have to change this in all the other repos for consistency.

I think it mainly matters for the "public goods" ones where people are likely to look for inspiration, like here, cosmwasm, cw-template. For other projects it's a nice-to-have IMO.

@uint uint force-pushed the schema-gen-in-bin-crate branch from 678dcdf to 972b693 Compare August 15, 2022 20:20
@uint
Copy link
Contributor Author

uint commented Aug 15, 2022

Rebased

@uint uint merged commit 8246967 into main Aug 15, 2022
@uint uint deleted the schema-gen-in-bin-crate branch August 15, 2022 20:40
@webmaster128
Copy link
Member

Is there a reason for keeping those unchanged?

$ find . -name schema.rs | grep examples/schema.rs
./packages/cw1/examples/schema.rs
./packages/cw20/examples/schema.rs
./packages/cw3/examples/schema.rs
./packages/cw4/examples/schema.rs
./packages/cw1155/examples/schema.rs

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.

Consider moving schema boilerplate from examples to a binary crate
5 participants