-
Notifications
You must be signed in to change notification settings - Fork 21
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
📏 Report binary size on PR #2
Conversation
63641bc
to
c64283c
Compare
Blocked by andresz1/size-limit-action#30 (comment) |
size-limit report 📦
|
It's working ! Just by adding a RUSTFLAGS at the compilation time (like cosmwasm documentation said), the wasm binary decrease by 90% 😍. |
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.
Great initiative! ❤️
Some remarks we can discuss.
And just a point of detail about this commit: feat: add binary size action. It should be prefixed with "ci:" like you did for the others. 😛
f1a43af
to
2df21cd
Compare
About this commit, I've removed it instead of rename since it was a try of another GitHub action |
fba5bde
to
e005a95
Compare
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.
That's a very relevant idea, thank you 👍 .
I'll have some little suggestions though
21008a1
to
784e3ba
Compare
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.
👍
Thanks for the work! Let's merge it. |
🎉 This PR is included in version 1.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
📝 Description
Given that the size of a smart contract is very important and that it has a very strong impact on transaction costs, it seemed to me necessary to add a small action allowing to report the evolution of the size of each smart contract binary wasm.
🛠 Tool
The tool used here is size-limit-action in addition to size-limit javascript tool. It's the only github action always maintained that allowing report file size evolution into a PR comment. Cargo-bloat also exist but is not compatible with wasm target.
A limitation can be set for each file and make the PR failed if the file size exceed the limit.
🐛 Issues
package.json
😭😭, sincesize-limit
is a JS tool, to make this action work, I need to add apackage.json file
. So to avoid pollute rust with JS stuff, I've add thepackage.json
file setting into the GitHub action.size-limit
needs a configuration file to set which file it should analyse. But for the first PR, the config file doesn't exist into the main branch, so the action failed (for comparaison with the main branch of course,size-limit
need to build on it). A temporary solution is to add the configuration file into thepackage.json
into the GitHub action, it's working but I prefer set the configuration into a specific file.size-limit
. Sincepackage.json
size-limit
configuration is priority, we need after this PR merged, remove the configuration frompackage.json
.