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

Cosmwasm plus compatibility #19

Merged
merged 6 commits into from
Aug 12, 2020
Merged

Conversation

ethanfrey
Copy link
Member

Closes #18
Required for CosmWasm/cw-plus#29

This adds a second script that handles the cosmwasm-plus case nicely, while leaving the optimize.sh script untouched.

The one issue that changes in the external API is that we cannot use entrypoint anymore. The default (no argument) compile works as is, but the cosmwasm compile process now requires optimize.sh ./contracts/hackatom at the end.

Updated the README to explain these cases.

optimize.sh Outdated Show resolved Hide resolved
multi-optimize.sh Outdated Show resolved Hide resolved
Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

I think it is cool to run the optimizer once on the entire repo instead multiple runs. I don't like that this exposes the shell scripts that are used on the public interface (but see why needed).

having an a glob argument that exponds on the host but then needs guest paths is confusing at least. Isn't there a cargo wasm thing that works like cargo fmt on the entire workspace by defaut?

Anyways, I need sleep, peace

@ethanfrey
Copy link
Member Author

Interesting point Simon.

I tried cargo build --release --target wasm32-unknown-unknown on the repo root and it generated all contract wasm (even faster than once per dir) and didn't include the non-contract packages.

It doesn't even require any arguments there. I will update and talk tomorrow.
Good night

@maurolacy
Copy link
Contributor

maurolacy commented Aug 11, 2020

Interesting point Simon.

I tried cargo build --release --target wasm32-unknown-unknown on the repo root and it generated all contract wasm (even faster than once per dir) and didn't include the non-contract packages.

It doesn't even require any arguments there. I will update and talk tomorrow.
Good night

Yes, but what about non-workspace contracts?

Good night.

@webmaster128
Copy link
Member

The other issue I see with this (and any multi-target build system in general) is how source code verification can work. If we don't get verification, we don't need rust-optimizer, so those go hand in hand.

The following conventions would not be followed with the current approach:

The builder is a docker image that works out of the box with docker run .
The builder takes a volume mounted at /code which is the root of the code to be built.
The builder must create a contract.wasm in the current directory.

https://github.com/CosmWasm/cosmwasm-verify#the-builder

The reason for those is that we need to be able to verify without project specific meta information: https://github.com/CosmWasm/cosmwasm-verify/blob/master/src/main.sh#L67-L73

This then leads us to the question what the source code archive of a cosmwasm-plus contract contains.

@ethanfrey
Copy link
Member Author

The builder is a docker image that works out of the box with docker run .
The builder takes a volume mounted at /code which is the root of the code to be built.
The builder must create a contract.wasm in the current directory.

Okay, this doesn't work for cosmwasm contracts right now either. But let's revisit it. How about we change it to the following (this is a 0.9 -> 0.10 so we can break stuff):

  • The builder is a docker image that works out of the box with docker run (removed .)
  • The builder takes a volume mounted at /code which is the root of the repo/workspace to be built.
  • The builder must create one or more *.wasm files in a directory called artifacts and a contracts.txt with their respective hashes
  • The *.wasm files must be named according to the crates that were compiled

With this, the verification steps are:

  • Download the source code and uncompress it
  • Run the docker container, mounting the source code under /code
  • Check if a line in contracts.txt matches the desired hash -> grab the name of the contract
  • (optional) validate the sha256sum of the named contract is correct
  • find crate with the given name inside the code directory -> and return this source/schema

If this is not desired to change, then I can change the multi-optimizer.sh to have the same output as optimizer does (but then it takes a subpath.... not according to spec either).

In either case, I should do some workspace-detection tricks, so we don't need arguments in the validator

@webmaster128
Copy link
Member

this doesn't work for cosmwasm contracts right now either.

Right, this is why I call them "development contracts" whenever I can to express those must not be taken seriously. There is no concept for source publication for them either.

@webmaster128
Copy link
Member

The proposed rules are good. Some additional comments:

@ethanfrey
Copy link
Member Author

Okay, I would agree with these changes. I would try to make mono-repo and single-contract run in the same builder. (and say we don't support cosmwasm builds except with additional arguments, but violating the verifier approach).

This means, I can make a default CMD that only supports cosmwasm-template and cosmwasm-examples (rooted in one crate) or cosmwasm-plus (rooted in repo root) style and not cosmwasm style. The cosmwasm style could be a non-default script and require arguments. Do I understand this properly?

I think I can unify these use cases as long as cosmwasm support is separate.

I will make all scripts return a set in artifacts along with names.

@webmaster128
Copy link
Member

This means, I can make a default CMD that only supports cosmwasm-template and cosmwasm-examples (rooted in one crate) or cosmwasm-plus (rooted in repo root) style and not cosmwasm style. The cosmwasm style could be a non-default script and require arguments. Do I understand this properly?

Yes, perfect. Whatever we do in the cosmwasm repo is for VM maintainers only (relative paths to vm and std) and should never be deployed to any serious chain.

@ethanfrey
Copy link
Member Author

Okay @webmaster128 please check out the latest commit (or just check the README usage).

I tested this manually on cosmwasm-examples (mounting the erc20 package as root), cosmwasm-plus (mounting repo root), and cosmwasm (using a non-standard argument ./contracts/burner).

All have the same behavior of placing named wasm files inside artifacts along with a contracts.txt file. Given this is how we publish them (also in cosmwasm) and not committing wasm to the source repo, this seems like a nicer standard approach that works for all cases.

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Nice. This is a really great solution that improves functionality in a very clean fashion.

Dockerfile Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
optimize.sh Outdated Show resolved Hide resolved
optimize.sh Outdated Show resolved Hide resolved
# create hash
(
cd artifacts
sha256sum -- *.wasm > contracts.txt
Copy link
Member

Choose a reason for hiding this comment

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

Is this -- needed for something? Looks confusing to me

Copy link
Member Author

Choose a reason for hiding this comment

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

shellcheck requested it.

If I had a file --some-bad-thing.wasm it would be interpreted as a flag to sha256sum not a file.

Not going to happen, but I just wanted shellcheck to let me run it.

Copy link
Member

Choose a reason for hiding this comment

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

Right it is, thanks for checking

🙏 holy shellcheck

optimize.sh Outdated Show resolved Hide resolved
@ethanfrey ethanfrey merged commit 5239b3b into master Aug 12, 2020
@ethanfrey ethanfrey deleted the cosmwasm-plus-compatibility branch August 12, 2020 22:42
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.

Adapt rust optimizer for cosmwasm-plus
3 participants