feat: added warning messages for when sierra file is too big or too many felts #3183
Conversation
…bled and too much felts
|
@glihm is this the intended solution? |
WalkthroughOhayo sensei! This change updates the build command in the Sozo CLI to always load world statistics and check contract size limits during every compilation, regardless of whether the Changes
Assessment against linked issues
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
bin/sozo/src/commands/build.rs (2)
170-193: The warning messages look good, but found a typo in the error messageThe implementation correctly checks Sierra file size and CASM bytecode size against network limits, emitting warnings to stderr when exceeded. This helps users identify potential deployment issues early.
However, there's a minor issue with the error message text:
- "{}: Sierra file size ({:.2}MB, {} bytes) exceeds network limit of 4,089,446 \ - bytes bytes - {}", + "{}: Sierra file size ({:.2}MB, {} bytes) exceeds network limit of 4,089,446 \ + bytes - {}",
173-173: Standardize limit values using constantsThe limit values are hardcoded in multiple places with an inconsistency between the check value (81,920) and the table coloring value (81,290). Consider defining these as constants at the module level to avoid duplication and ensure consistency.
// Add at module level + const MAX_SIERRA_SIZE_BYTES: usize = 4_089_446; + const MAX_CASM_FELTS: usize = 81_920; // Then use these constants in both places - if stat.sierra_file_size > 4_089_446 { + if stat.sierra_file_size > MAX_SIERRA_SIZE_BYTES { - if stat.casm_bytecode_size > 81_920 { + if stat.casm_bytecode_size > MAX_CASM_FELTS { // Also update line 278 - const MAX_CASM_FELTS: usize = 81_290; + const MAX_CASM_FELTS: usize = 81_920;Also applies to: 185-185, 277-278
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/sozo/src/commands/build.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: fmt
🔇 Additional comments (2)
bin/sozo/src/commands/build.rs (2)
158-168: Ohayo sensei! LGTM - Always collecting stats for limit checksThe code now unconditionally loads the World data and collects statistics for all resources, which is necessary for the limit checks. This changes the previous behavior where stats were only loaded when the
--statsflag was provided.
195-195: LGTM - Conditional display of statistics tableThe code now properly separates the limit checking (always done) from the statistics table display (only when requested). This gives users important warnings while keeping the output clean when detailed stats aren't needed.
There was a problem hiding this comment.
I think this should be dynamic based on the network we're migrating the World to. These limits are only being enforced by Starknet Mainnet/Sepolia at the moment, and not on katana-based chains yet. So, ideally, we should first try to determine what chain we may be migrating to, using the rpc url specified in the dojo_*.toml, and show the warnings accordingly.
We should also be very clear about which chain the limits apply to.
There was a problem hiding this comment.
Thank you @PoulavBhowmick03 for the work there. Some remarks. :)
| // Check limits and emit warnings for EVERY compilation | ||
| for stat in &stats { | ||
| // Sierra file size check (4,089,446 bytes) | ||
| if stat.sierra_file_size > 4_089_446 { |
There was a problem hiding this comment.
Could we move those magic values to constants? They are used below in the code, they may be extracted from the from implementation and put at the top of this module (with a link to the sn doc that mentioned those limits).
| // Sierra file size check (4,089,446 bytes) | ||
| if stat.sierra_file_size > 4_089_446 { | ||
| eprintln!( | ||
| "{}: Sierra file size ({:.2}MB, {} bytes) exceeds network limit of 4,089,446 \ |
There was a problem hiding this comment.
Let's rework a bit the text to make it more readable:
- We can remove warning word, and print in yellow is ok to note a warning.
- Wdyt about something like
Resource <TAG> exceeds network Sierra file size (#bytes)
Resource <TAG> exceeds network Casm byte code size (#felts)
I do agree on the dynamism this should have. However at the build time, we may not be aware of the chain. So we could move those changes into |
|
> I do agree on the dynamism this should have. However at the build time, we may not be aware of the chain.
Yes, agree on this. Makes more sense to show the warning at The built contracts are agnostic to the chain that it would be migrated to, so let's push this feature over to |
|
@PoulavBhowmick03 if you have any blocker or questions, don't hesitate to ask. 🫡 |
|
gm @glihm , i was actually at the hospital. getting it done by tonight |
Oh shoot, hopefully you're doing good man.. No pressure, when you have a chance. |
|
Any update @PoulavBhowmick03? Hopefully you're feeling better. 🙏 |
|
@PoulavBhowmick03 I do hope you're doing better man, and don't hesitate to drop off to recover on your end. 🙏 |
|
Will close this since we now rely on Scarb to build files. So it would be a different code, using the Hope you're doing better @PoulavBhowmick03 and things are going well. |
3 participants
Description
Related issue
Fixes #3149
Tests
Added to documentation?
Checklist
scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit