-
Notifications
You must be signed in to change notification settings - Fork 0
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
Final demo contract suggestions #102
Final demo contract suggestions #102
Conversation
Bincode was unnecessarily included with default features, so it included panic handlers from `std`. It is better to make contract a `no_std`, so it's smaller. For such case, panic handlers can be registered with `no-std-helpers` feature in `casper-contract`.
@marijanp It seems that reverting to stable toolchain for non-WASM targets breaks builds. Could you help with making this dual toolchain setup? Update: I was informed that unstable toolchain will be used in a prover as well, and having 2 different toolchains will be too heavy for CI/CD. So I am reverting my change; will explore how to build contracts with stable toolchain later. |
For now we can stick to unstable one, to avoid heavy CI/CD.
@jonas089 Ready for review, even some tests are failing 🤔
@marijanp It it nix/cachix issue? |
Minimum allowed coverage is Generated by 🐒 cobertura-action against 0c3ff1a |
You have to scroll up in the build log, it says that the port is already in use. Current state of investigation: calling Interestingly if you run it on more powerful machines, the issue does not apper. Thus I believe it has something to do with GHA runners just not having enought computational power. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wasm_helper
module basically allows running tests without nix by requiring to build the contracts manually first and then running the tests. PATH_TO_WASM_BINARIES
is still being defaulted to if it is sourced (still compatible with nix develop).
I'm fine with the other changes, I also like the changes to use the casper deps from crates.io.
} else { | ||
"release" | ||
}; | ||
panic!("WASM directory does not exist: {}. Please build smart contracts at `./kairos-contracts` with `cargo build` for {}.", base_path.display(), build_type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not very nice imo, but if it allows you to run the tests without needing to be in a nix devShell it's fine to me.
Final suggestions before merging demo contract.
Refactor Cargo dependencies
base64
dependency.test-support
feature from contract dependency.bincode
dependency and useno-std-helpers
.Cargo.toml
files.Minor fixes/improvements
Allow running WASM tests without extra setup
No longer need to set
PATH_TO_WASM_BINARIES
, installwasm-opt
or use Nix 😮💨. Just compile contracts, and run integration tests withcargo test
- it will take care of WASM optimization during runtime.wasm-opt
dependency.