fix: cap felt252_to_byte_array at 31 bytes to prevent 'bad append len'#74
fix: cap felt252_to_byte_array at 31 bytes to prevent 'bad append len'#74starknetdev merged 1 commit intonextfrom
Conversation
ByteArray.append_word requires len <= 31, but U256BytesUsedTraitImpl::bytes_used can return 32 for felt252 values where the u256 high limb is non-zero (any felt252 >= 2^248). This causes a 'bad append len' panic in token_uri rendering for tokens with large felt252 game detail values or player names. Cap the length at 31 in felt252_to_byte_array and replace inline append_word + bytes_used calls in metadata.cairo and svg.cairo with felt252_to_byte_array to centralize the fix. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical runtime panic occurring during Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Claude finished @starknetdev's task —— View job Claude Review - General Engineering Reviewrun=22804131459 attempt=1 sha=bbf1d396bc7c9ad390b42cc4528aeaa3891caf8f scope=general Todo List
Review ResultsAfter analyzing the PR diff with the command: git diff 3322d946804c02ea0b56c821ad81b80388c24fdb...bbf1d396bc7c9ad390b42cc4528aeaa3891caf8f -- . ':(exclude)packages/**'No issues found. This PR contains only changes within the
No changes were made to:
The Cairo-specific code review is handled by the dedicated Cairo reviewer. Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO |
|
Claude finished @starknetdev's task —— View job Claude Review - Cairo/Starknet Contract Reviewrun=22804131459 attempt=1 sha=bbf1d396bc7c9ad390b42cc4528aeaa3891caf8f scope=packages Review FindingsThis PR addresses a critical runtime panic in SVG/metadata rendering caused by ✅ SECURITY REVIEW: No Issues Found
✅ CAIRO IDIOMS: Clean Implementation
✅ TESTING: Comprehensive CoverageStrong test suite with 5 new tests:
The fuzz test is particularly valuable—it confirms the function never panics for any felt252 input. ✅ GAS OPTIMIZATION: Improvement
✅ ARCHITECTURE: Well-Designed
Root Cause Analysis Verification ✓The PR description accurately identifies the issue: // Before (encoding.cairo:189) - PROBLEMATIC
result.append_word(value, U256BytesUsedTraitImpl::bytes_used(value.into()).into());
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
// Can return 32 for felt252 >= 2^248 → panic: 'bad append len'
// After (encoding.cairo:189-198) - FIXED
let len: u8 = U256BytesUsedTraitImpl::bytes_used(value.into());
let len: usize = if len > 31 { 31 } else { len.into() }; // Cap at 31
result.append_word(value, len);Mathematical validation: For felt252 values where Code Quality Assessment ✓packages/utilities/src/utils/encoding.cairo:186-201 - The core fix is excellent:
packages/utilities/src/renderer/metadata.cairo:107 & packages/utilities/src/renderer/svg.cairo:253 - Clean refactoring:
Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFOAPPROVED ✅ This is a high-quality bug fix that:
The fix is production-ready and should prevent this class of error across the entire codebase. |
Codex Review - General Engineering ReviewNo issues found. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a panic caused by ByteArray.append_word when handling large felt252 values. The fix, which involves capping the length at 31 bytes within a centralized felt252_to_byte_array function, is well-implemented. The refactoring in metadata.cairo and svg.cairo to use this new function is clean. The addition of comprehensive tests, including a fuzz test, is excellent and ensures the robustness of the fix. I have one minor suggestion to improve conciseness in the encoding.cairo file.
| let len: usize = if len > 31 { | ||
| 31 | ||
| } else { | ||
| len.into() | ||
| }; | ||
| result.append_word(value, len); |
There was a problem hiding this comment.
For conciseness and to avoid variable shadowing, you can combine the length capping and the function call into a single expression. This makes the code slightly more direct and idiomatic.
result.append_word(value, if len > 31 { 31 } else { len.into() });
Codex Review - Cairo/Starknet Contract ReviewNo issues found. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
2 participants
Summary
ByteArray.append_word(word, len)requireslen <= 31, butU256BytesUsedTraitImpl::bytes_used()returns 32 for felt252 values >= 2^248 (where the u256 high limb is non-zero). This causes abad append lenpanic duringtoken_uriSVG/metadata rendering.felt252_to_byte_arraysince a felt252 can encode at most 31 bytes of short-string data.append_word+bytes_usedcalls inmetadata.cairoandsvg.cairowithfelt252_to_byte_arrayto centralize the fix.Root Cause
U256BytesUsedTraitImpl::bytes_used()computesbytes_used(high) + 16, which can be up to 32. Butappend_wordonly acceptslen <= 31.Changes
packages/utilities/src/utils/encoding.cairo— cap at 31 infelt252_to_byte_arraypackages/utilities/src/renderer/metadata.cairo— usefelt252_to_byte_arrayfor player_namepackages/utilities/src/renderer/svg.cairo— usefelt252_to_byte_arrayfor player_namepackages/utilities/src/utils/tests/test_encoding.cairo— 5 new tests including 256-run fuzz testTest plan
snforge test -p game_components_utilities— 222 tests passing (5 new)felt252_to_byte_arraynever panics for any felt252 valuescarb fmt --check— clean🤖 Generated with Claude Code