feat: renderer refactor, game versioning, and SVG gas optimization#71
feat: renderer refactor, game versioning, and SVG gas optimization#71starknetdev merged 6 commits intonextfrom
Conversation
- Remove all 14 <symbol> definitions from <defs> block - Replace 19 <use> references with inline <svg> elements at each use site - Add helper functions (icon_check, icon_x, icon_target, icon_flag) for icons used 2+ times to avoid duplicating path strings - Replace base64 encoding with url_encode_hash (#→%23) for data URI, eliminating ~33% size inflation from base64 Reduces SVG gas cost by removing unused icon definitions from output and avoiding expensive base64 encoding. 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 significantly refactors the SVG rendering logic by moving away from symbol-based icon definitions to direct inlining and helper functions, aiming to reduce the final SVG size and improve rendering efficiency. A key optimization involves switching from base64 encoding to URL encoding for SVG data URIs, further decreasing the output size. Additionally, the scope of metadata generation has been narrowed by removing the custom JSON metadata creation function. 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=22707535344 attempt=1 sha=0b1062e3d2521174b6ebb99c30e1c271f12b3c20 scope=general Todo List
Review FindingsThis PR modifies only No actionable findings for files within my review scope. Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO The Cairo contract code changes in |
Codex Review - General Engineering ReviewNo issues found. |
Includes renderer refactoring (metadata extraction, font removal), registry updates, minigame/metagame test additions, interface changes, and example contract updates. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @starknetdev's task —— View job Claude Review - General Engineering Reviewrun=22707617761 attempt=1 sha=f51114ae51fbfa2cd8373d4a87b4b3345e4ca6f1 scope=general Todo List
Review FindingsThis PR modifies only No actionable findings for files within my review scope. Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO The Cairo contract code changes in |
Codex Review - General Engineering ReviewNo issues found. |
starknetdev
changed the title
![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 refactors the SVG generation by inlining icons and switching from base64 to URL encoding for data URIs, which aims to reduce final output size and enhance code clarity. However, this change introduces a high-severity Cross-Site Scripting (XSS) vulnerability. The SVG content is now constructed from unsanitized user-controlled metadata, allowing malicious input in fields like the game name, description, or image URL to lead to arbitrary script execution when the SVG is rendered. Proper XML/SVG escaping of all user-controlled data is required to fix this issue. Additionally, the create_custom_metadata function and its associated tests have been removed, and there are opportunities for minor gas optimizations in the new helper functions by avoiding unnecessary clones and using more direct byte appending.
| let metadata = metadata.add_array("attributes", attributes.span()).build(); | ||
|
|
||
| format!("data:application/json;base64,{}", bytes_base64_encode(metadata)) | ||
| "data:image/svg+xml," + url_encode_hash(svg) |
There was a problem hiding this comment.
The switch from base64 encoding to raw SVG in the data:image/svg+xml URI introduces a significant Cross-Site Scripting (XSS) vulnerability. The svg variable contains multiple unsanitized user-controlled fields (such as game_metadata.name, game_metadata.developer, game_metadata.description, game_metadata.image, player_name, etc.) that are concatenated directly into the SVG string.
While the previous base64 encoding provided some level of protection by preventing the browser from interpreting the SVG content as part of the parent document in some contexts, serving the raw SVG allows any injected <script> tags or malicious event handlers (e.g., in foreignObject or via onerror attributes in img tags) to execute when the data URI is rendered. The url_encode_hash function only escapes the # character, which is insufficient to prevent SVG injection or XSS.
To remediate this, you must properly escape all user-controlled strings for XML/SVG before appending them to the svg ByteArray. Specifically, characters like <, >, &, ', and " should be replaced with their corresponding XML entities (e.g., <, >, &, ', "). Alternatively, consider using a full URL encoding for the entire SVG string or reverting to base64 encoding.
| } | ||
| let byte = input.at(i).unwrap(); | ||
| if byte == 0x23 { | ||
| output.append(@"%23"); |
There was a problem hiding this comment.
For better performance, it's more efficient to append bytes directly rather than appending a felt252 literal. Appending a felt252 involves a conversion to ByteArray which has some overhead. Using append_byte is more direct and saves gas.
output.append_byte(0x25); // '%'
output.append_byte(0x32); // '2'
output.append_byte(0x33); // '3'
| + "' height='" | ||
| + h | ||
| + "' viewBox='0 0 16 16'><path fill='none' stroke='" | ||
| + color.clone() |
There was a problem hiding this comment.
Since color is a snapshot (@ByteArray) and is only used once in this function, you can move it directly into the string concatenation instead of cloning it. This is a minor gas optimization.
+ color
| + "' height='" | ||
| + h | ||
| + "' viewBox='0 0 16 16'><path fill='none' stroke='" | ||
| + color.clone() |
There was a problem hiding this comment.
Since color is a snapshot (@ByteArray) and is only used once in this function, you can move it directly into the string concatenation instead of cloning it. This is a minor gas optimization.
+ color
| + "' stroke-width='1.5' cx='8' cy='8' r='6'/><circle fill='none' stroke='" | ||
| + color.clone() | ||
| + "' stroke-width='1.5' cx='8' cy='8' r='3'/><circle fill='" | ||
| + color.clone() |
There was a problem hiding this comment.
For the last use of the color snapshot in this function, you can move it instead of cloning to save some gas.
+ color
| + "' viewBox='0 0 16 16'><path fill='none' stroke='" | ||
| + color.clone() | ||
| + "' stroke-width='1.5' stroke-linecap='round' d='M4 14V2'/><path fill='" | ||
| + color.clone() |
There was a problem hiding this comment.
For the last use of the color snapshot in this function, you can move it instead of cloning to save some gas.
+ color
Remove the byte-by-byte url_encode_hash function and use data:image/svg+xml;utf8, prefix instead, avoiding any runtime encoding cost entirely. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @starknetdev's task —— View job Claude Review - General Engineering Reviewrun=22707734379 attempt=1 sha=de47a04d93f27ae841e38ccc769385c7e1b2b800 scope=general Todo List
Review FindingsThis PR modifies only No actionable findings for files within my review scope. Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO The Cairo contract code changes in |
|
Claude finished @starknetdev's task —— View job Claude Review - Cairo/Starknet Contract Reviewrun=22707734379 attempt=1 sha=de47a04d93f27ae841e38ccc769385c7e1b2b800 scope=packages Todo List
Review Findings[HIGH] packages/utilities/src/renderer/svg.cairo:787 - XSS vulnerability in SVG generation [HIGH] packages/utilities/src/renderer/svg.cairo:106-112 - Inefficient gas usage in icon helper functions [MEDIUM] packages/embeddable_game_standard/src/registry/registry_component.cairo:274 - Missing version field validation [LOW] packages/interfaces/src/registry.cairo:10-11 - Interface ID updated without documentation Summary: 2 HIGH, 1 MEDIUM, 1 LOW, 0 INFO Critical Security Issue: The most serious finding is the XSS vulnerability in SVG generation. The switch from base64 encoding to direct URL encoding removes a layer of protection against script injection attacks. This must be fixed before deployment. The game versioning implementation correctly threads the version field through all necessary components and updates interface IDs appropriately. Test coverage is comprehensive with proper version field additions throughout. |
Codex Review - General Engineering ReviewNo issues found. |
Codex Review - Cairo/Starknet Contract Review[HIGH] packages/utilities/src/renderer/metadata.cairo:130 - [HIGH] packages/utilities/src/renderer/svg.cairo:787 - SVG is returned as a raw [MEDIUM] packages/utilities/src/renderer/tests/test_renderer.cairo:192 - SVG assertions still expect Summary: 0 CRITICAL, 2 HIGH, 1 MEDIUM, 0 LOW, 0 INFO |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Replace all 14 <symbol> definitions with inline <svg> elements, remove base64 encoding in favor of data:image/svg+xml;utf8 with pre-encoded %23 in static strings and uri_encode for dynamic text. Reduces SVG gas from ~999M to ~39.6M L2 gas (96% reduction). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @starknetdev's task —— View job Claude Review - General Engineering Reviewrun=22708800594 attempt=1 sha=a232c74a7b3dfaa5851cb2845dc18b19bb33e65e scope=general Todo List
Review FindingsThis PR modifies only No actionable findings for files within my review scope. Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO The Cairo contract code changes in |
Codex Review - General Engineering ReviewNo issues found. |
Add test_loot_survivor_svg example with full game metadata. Clip game name text with clipPath to prevent overlap with player name on long titles. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @starknetdev's task —— View job Claude Review - General Engineering Reviewrun=22711813679 attempt=1 sha=416d1ea80dae5720e62efb2a3f047bb369b518d8 scope=general Todo List
Review FindingsThis PR modifies only No actionable findings for files within my review scope. Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO The Cairo contract code changes in |
|
Claude finished @starknetdev's task —— View job Claude finished @starknetdev's task —— View job Claude Review - Cairo/Starknet Contract Reviewrun=22711813679 attempt=1 sha=416d1ea80dae5720e62efb2a3f047bb369b518d8 scope=packages Todo List
Review Findings[HIGH] packages/utilities/src/renderer/svg.cairo:856 - Potential XSS vulnerability in SVG generation [MEDIUM] packages/utilities/src/renderer/svg.cairo:160 - Unnecessary clone() calls in icon helper functions [LOW] packages/interfaces/src/registry.cairo:9 - Interface ID updated without documentation Summary: 0 CRITICAL, 1 HIGH, 1 MEDIUM, 1 LOW, 0 INFO Critical Finding: The most serious issue is the potential XSS vulnerability in SVG generation. While the PR description mentions switching from base64 to URL encoding for gas optimization, this change removes a layer of protection against script injection. The Game Versioning Implementation: The version field addition is correctly implemented throughout the codebase - properly threaded through all registration functions, stored in GameMetadata struct, and included in all tests. The interface ID constant was appropriately updated to reflect the signature change. Test Coverage: The test suite has been comprehensively updated with the new version field added to all registration calls. Test assertions correctly expect the new |
Codex Review - General Engineering ReviewNo issues found. |
Codex Review - Cairo/Starknet Contract Review[MEDIUM] packages/utilities/src/renderer/svg.cairo:856 - The SVG data URI uses [LOW] packages/embeddable_game_standard/src/registry/tests/test_registry_component.cairo:295 - New [INFO] packages/utilities/src/renderer/tests/test_renderer.cairo:256 - Summary: 0 CRITICAL, 0 HIGH, 1 MEDIUM, 1 LOW, 1 INFO |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @starknetdev's task —— View job Claude Review - General Engineering Reviewrun=22712360091 attempt=1 sha=1491e14c64f5b59fd493e9a42125e5b68981d612 scope=general Todo List
Review FindingsThis PR modifies only No actionable findings for files within my review scope. Summary: 0 CRITICAL, 0 HIGH, 0 MEDIUM, 0 LOW, 0 INFO The Cairo contract code changes in |
Codex Review - General Engineering ReviewNo issues found. |
2 participants
Summary
Renderer refactoring
svg.cairointo newmetadata.cairosubmodulefont.cairo(VT323 WOFF2 font)AGENTS.mddocumentation for renderer moduleSVG gas optimization
<symbol>icon definitions at their use sites, eliminating unused definitions from outputicon_check,icon_x,icon_target,icon_flag) for multi-use icons#→%23URL encoding fordata:image/svg+xml,URIs, avoiding ~33% base64 size inflationico-usersymbolGame versioning
version: u64field toGameMetadatastructversionthroughregister_game,MinigameComponent, andMinigameRegistryComponentIMINIGAME_REGISTRY_IDinterface ID constantTest updates
versionfieldTest plan
scarb buildcompilessnforge test renderer— all tests pass (test assertions need updating fromdata:image/svg+xml;base64,todata:image/svg+xml,)snforge test registry— all tests passsnforge test minigame— all tests passsnforge test test_stress— compare SVG gas vs baseline (~999M)🤖 Generated with Claude Code