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

Find a better place for check_contract etc #1371

Closed
uint opened this issue Jul 20, 2022 · 21 comments
Closed

Find a better place for check_contract etc #1371

uint opened this issue Jul 20, 2022 · 21 comments
Assignees
Labels
good first issue Good for newcomers
Milestone

Comments

@uint
Copy link
Contributor

uint commented Jul 20, 2022

This came up in CosmWasm/cw-plus#755.

Currently there are a few utilities that live as examples in the cosmwasm-vm package - check_contract, module_size, multi_threaded_cache.

It would be nice to find a better place for them - they're not really examples. The most proper thing I can think of is to set up at least one new package (cosmwasm-toolbox? or separate packages?) that depends on cosmwasm_vm and put them there as binary crates - this way we can remove dev-dependencies like clap from cosmwasm_vm entirely.

@hashedone
Copy link
Contributor

hashedone commented Aug 1, 2022

It can stay in examples for now to not affect every CI flow in the world, but I would strongly consider creating separate binary project for that and publish it as separate packages on crates.io (maybe with better names reflecting it being part of cw) so one can easily install it with cargo install cw-check-contract or so - I actually use check contract all the time to make sure contract is proper without uploading it to BC. I think that it is good starter issue, I think @jawoznia can take it.

@uint uint added the good first issue Good for newcomers label Aug 8, 2022
@AbhinavMir
Copy link

The cw-toolbox idea is great. Currently working with CosmWasm and definitely see this as potentially important. Happy to solve this one, if no one's working on it already.

@jawoznia
Copy link
Contributor

This is now stored under https://github.com/CosmWasm/cw-tools
It will also stay in examples of comswasm-vm for compatibility reasons.

https://crates.io/crates/cw-check-contract
https://crates.io/crates/cw-module-size
https://crates.io/crates/cw-multi-threaded-cache

@uint
Copy link
Contributor Author

uint commented Aug 12, 2022

This is now stored under https://github.com/CosmWasm/cw-tools

Nice! Good job!

It will also stay in examples of comswasm-vm for compatibility reasons.

Do we have a deprecation plan? Should we add some sort of notice to the examples binaries? Like echo this to stderr:

`check_contract` will be removed from the next version of `cosmwasm-vm` - please use `cw-check-contract` instead.
> cargo install cw-check-contract

The next step would be to update places we check contracts from CI like these:

@uint
Copy link
Contributor Author

uint commented Aug 12, 2022

The cw-toolbox idea is great. Currently working with CosmWasm and definitely see this as potentially important. Happy to solve this one, if no one's working on it already.

@AbhinavMir Thanks for the interest! Jan was already working on this one, but we're happy for other contributions

@hashedone
Copy link
Contributor

One thing to consider is to figure out the process (some CI check on cosmwasm-vm?) so we don't forget to update those binaries when cosmwasm-vm is updated. Ideas?

@uint
Copy link
Contributor Author

uint commented Aug 16, 2022

One thing to consider is to figure out the process (some CI check on cosmwasm-vm?) so we don't forget to update those binaries when cosmwasm-vm is updated. Ideas?

The only thing that comes to mind is having the cosmwasm-vm binaries return an error code, but that's probably too drastic. Maybe a version later we'd do that?

@hashedone
Copy link
Contributor

hashedone commented Aug 16, 2022

The problem is that we don't want to fail CI right after updating VM, as it would make it painfully to deliver anything. But does github allow for some notifications on release (like: keep in mind to update xxxx?). Or maybe we want to have something like dependabot running on cw-tools verifying the cosmwasm-vm version?

@webmaster128
Copy link
Member

Can we make this a new package in this monorepo? If we move the binary to a different place, we will run into maintenance issue. Also cosmwasm-vm will include breaking changes from time to time in minor versions, as nobody really uses this crate except wasmvm and we want to keep versions in sync. So it's better if those things stay together.

multi_threaded_cache and module_size are examples and can stay there I think.

@hashedone
Copy link
Contributor

We created the separate repo for this, but actually, I think you are right - we can move it here and it would solve issues. Probably as separate projects to package directory. It shouldn't be much work, can you handle it @jawoznia?

@webmaster128
Copy link
Member

as separate projects

Why plural? It's only check_contract that matters, right?

@jawoznia
Copy link
Contributor

Sure, I can handle it once I finish my current task.

@uint
Copy link
Contributor Author

uint commented Aug 16, 2022

The problem is that we don't want to fail CI right after updating VM, as it would make it painfully to deliver anything.

Exactly. But I think we should have a deprecation plan to eventually get rid of examples/check_contract.rs. Something like:

  1. cosmwasm 1.1 - examples/check_contract still works, but echoes an stderr deprecation notice:
    `check_contract` will be removed from the next version of `cosmwasm-vm` - please use `cw-check-contract` instead.
    > cargo install cw-check-contract
    
  2. cosmwasm 1.2 - Have examples/check_contract exit with an error code and a message:
    `check_contract` has been removed from `cosmwasm-vm` examples - please use `cw-check-contract` instead.
    > cargo install cw-check-contract
    
  3. cosmwasm 1.3 - remove examples/check_contract completely.

@webmaster128
Copy link
Member

What's the point of getting rid of examples? Two of the three are perfectly fine there. One was an example but developed into a tool and can be moved out.

@uint
Copy link
Contributor Author

uint commented Aug 16, 2022

What's the point of getting rid of examples? Two of the three are perfectly fine there. One was an example but developed into a tool and can be moved out.

Hm. I guess I got confused by the fact module_size takes CLI arguments. That made it look more like a tool than an example to me.

@webmaster128
Copy link
Member

I guess I got confused by the fact module_size takes CLI arguments. That made it look more like a tool than an example to me.

Today module_size is more a dev note than anything else. Feel free to remove the clap dependency and dramatically simplify it. With Wasmer 3 the module size calculation will change anyways (🤞 it still exists) because Wasmer removed the loupe dependency.

@webmaster128
Copy link
Member

I think we have a plan now. @uint could you peer with Jan on this?

@uint
Copy link
Contributor Author

uint commented Aug 16, 2022

Alright, I edited my "deprecation plan" post above to reflect we would be eventually removing only examples/check_contract.rs... I think?

@uint
Copy link
Contributor Author

uint commented Aug 22, 2022

@webmaster128 Shall we close this? We found a better place for check_contract and we have a plan.

@webmaster128
Copy link
Member

Yeah, thanks all

@webmaster128
Copy link
Member

webmaster128 commented Sep 5, 2022

The standalone application cosmwasm-check is now released as part of the 1.1.0 release: https://crates.io/crates/cosmwasm-check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

5 participants