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

Generate schema through workspace build script #303

Merged
merged 17 commits into from
Apr 11, 2024

Conversation

kaimen-sano
Copy link
Contributor

Description and Motivation

We currently generate schemas by having a binary schema.rs on each contract which will use the cosmwasm_schema API to write a schema.

While this works, it requires us to maintain the binary for each contract. This is tedious (extra files per contract, including a .cargo alias definition), and also requires us to correctly use the cosmwasm_schema per contract. Additionally, we were getting Cargo binary name collisions:

The targets should have unique names.
Consider changing their names to be unique or compiling them separately.
This may become a hard error in the future; see <https://github.com/rust-lang/cargo/issues/6313>.

This PR aims to standardize the process of creating a schema for a contract by moving the logic to a new xtask: generate_schemas. This task will generate all the schema files for the entire workspace at once.

This is similar work to the previous just schemas (which was broken), however we now get the benefit of being able to remove the binary from each contract. This could potentially speed up compile times.

This PR also moves the schema PR workflow check to a new workflow file, and aims to add a comment to the PR if the schemas failed.


Checklist:

  • I have read Migaloo's contribution guidelines.
  • My pull request has a sound title and description (not something vague like Update index.md)
  • All existing and new tests are passing.
  • I updated/added relevant documentation.
  • The code is formatted properly cargo fmt --all --.
  • Clippy doesn't report any issues cargo clippy -- -D warnings.
  • I have regenerated the schemas if needed with just schemas.

@kaimen-sano kaimen-sano self-assigned this Apr 6, 2024
@kaimen-sano kaimen-sano added infra CI infrastructure issue, e.g. Github actions bug V2 Everything related to the V2 implementation of White Whale labels Apr 6, 2024
Copy link

codecov bot commented Apr 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.56%. Comparing base (1aec306) to head (3be1380).
Report is 1 commits behind head on release/v2_contracts.

❗ Current head 3be1380 differs from pull request most recent head 755c4bd. Consider uploading reports for the commit 755c4bd to get more accurate results

Additional details and impacted files
@@                   Coverage Diff                    @@
##           release/v2_contracts     #303      +/-   ##
========================================================
+ Coverage                 88.50%   88.56%   +0.06%     
========================================================
  Files                       252      243       -9     
  Lines                     23602    23586      -16     
========================================================
+ Hits                      20888    20889       +1     
+ Misses                     2714     2697      -17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kaimen-sano
Copy link
Contributor Author

Elected to exclude xtask from Tarpaulin here. It is not a critical piece of software for us to test.

Testing GitHub actions to see if the workflow will trigger a comment for
forgetting to include a schema.
Fixes the build_schemas fail_diff_flag for not checking new, uncommitted
files.
Copy link

github-actions bot commented Apr 6, 2024

Schema generation failed. Please run just schemas locally and upload the generated schemas.

Copy link

github-actions bot commented Apr 6, 2024

Schema generation had missing jobs:
```${diffSchemas}````
Please run just schemas locally and upload the generated schemas.

Copy link

github-actions bot commented Apr 6, 2024

Schema generation had missing jobs:

contracts/liquidity_hub/fee_collector/schema/raw/query.json

Please run just schemas locally and upload the generated schemas.

Copy link

github-actions bot commented Apr 6, 2024

Schema generation had missing jobs:


Please run just schemas locally and upload the generated schemas.

Copy link

github-actions bot commented Apr 6, 2024

Schema generation had missing jobs:

contracts/liquidity_hub/pool-manager/schema/raw/execute.json
contracts/liquidity_hub/pool-manager/schema/pool-manager.json

Please run just schemas locally and upload the generated schemas.

@kaimen-sano
Copy link
Contributor Author

Good to review, finally :)

Copy link
Contributor

@0xFable 0xFable left a comment

Choose a reason for hiding this comment

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

LGTM great work never heard of xtask until now but looks like its even used by Cargo 🤯

@kaimen-sano
Copy link
Contributor Author

LGTM great work never heard of xtask until now but looks like its even used by Cargo 🤯

It was a great find to run scripts for the workspace that need access to our Rust interfaces. I was also not aware of how widespread the pattern is in the Rust ecosystem, but it certainly looks like a nice complement to just for our case.

Copy link
Contributor

@kerber0x kerber0x left a comment

Choose a reason for hiding this comment

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

Great job!!

@kerber0x
Copy link
Contributor

:shipit: @kaimen-sano

@kaimen-sano kaimen-sano merged commit 426c5c2 into release/v2_contracts Apr 11, 2024
5 checks passed
@kaimen-sano kaimen-sano deleted the build-scripts branch April 11, 2024 03:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra CI infrastructure issue, e.g. Github actions bug V2 Everything related to the V2 implementation of White Whale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants