fix(torii-tokens-graphql): returns tokens balances with no metadata #2764
Conversation
glihm
changed the title
WalkthroughOhayo, sensei! This pull request introduces modifications to the Changes
Possibly related PRs
🪧 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: 1
🧹 Outside diff range and nitpick comments (1)
crates/torii/graphql/src/object/erc/token_balance.rs (1)
258-258: Consider extracting image path template to configurationThe image path format
{token_id}/imageappears to be hardcoded. Consider moving this to a configuration value for better maintainability.+ const TOKEN_IMAGE_PATH_TEMPLATE: &str = "{}/image"; + // Later in the code - let image_path = format!("{}/{}", token_id.join("/"), "image"); + let image_path = format!(TOKEN_IMAGE_PATH_TEMPLATE, token_id.join("/"));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
crates/torii/graphql/src/object/erc/token_balance.rs(2 hunks)
🔇 Additional comments (1)
crates/torii/graphql/src/object/erc/token_balance.rs (1)
106-106: Ohayo! The simplified WHERE clause looks good, sensei!
The removal of the metadata length check from the conditions is a positive change as token balances should be returned regardless of metadata presence.
| let metadata: serde_json::Value = | ||
| serde_json::from_str(&metadata_str).expect("metadata is always json"); | ||
| let metadata_name = |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling for metadata parsing, sensei!
The .expect("metadata is always json") could lead to a panic if the metadata is malformed. Consider using a more graceful error handling approach.
- let metadata: serde_json::Value =
- serde_json::from_str(&metadata_str).expect("metadata is always json");
+ let metadata: serde_json::Value = match serde_json::from_str(&metadata_str) {
+ Ok(value) => value,
+ Err(e) => {
+ warn!("Failed to parse metadata JSON: {}", e);
+ return (String::new(), None, None, None, String::new());
+ }
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let metadata: serde_json::Value = | |
| serde_json::from_str(&metadata_str).expect("metadata is always json"); | |
| let metadata_name = | |
| let metadata: serde_json::Value = match serde_json::from_str(&metadata_str) { | |
| Ok(value) => value, | |
| Err(e) => { | |
| warn!("Failed to parse metadata JSON: {}", e); | |
| return (String::new(), None, None, None, String::new()); | |
| } | |
| }; | |
| let metadata_name = |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2764 +/- ##
==========================================
- Coverage 55.50% 55.50% -0.01%
==========================================
Files 434 434
Lines 54295 54307 +12
==========================================
+ Hits 30137 30141 +4
- Misses 24158 24166 +8 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/preview.yml (1)
Line range hint
1-77: Ohayo sensei! Here are some workflow optimization suggestions.
- Consider consolidating the two cache actions into one by using a pattern:
path: /tmp/.buildx-cache/*
- Consider using a more efficient cache key strategy:
key: ${{ runner.os }}-buildx-${{ hashFiles('**/Dockerfile') }}
- Define the platform as an environment variable to avoid duplication.
Would you like me to provide a complete optimized version of the workflow?
.github/workflows/preview.yml
Outdated
| mv /tmp/.buildx-cache-new/prebuild /tmp/.buildx-cache/prebuild | ||
| rm -rf /tmp/.buildx-cache/build | ||
| mv /tmp/.buildx-cache-new/build /tmp/.buildx-cache/build | ||
|
|
There was a problem hiding this comment.
Ohayo sensei! Critical cache management issue detected.
The workflow writes Docker cache to new locations (/tmp/.buildx-cache-new/*) but never moves them to the expected cache locations (/tmp/.buildx-cache/*). This will prevent cache from being restored in subsequent runs, defeating the purpose of caching and significantly increasing build times.
Please add the following steps after each build step:
- name: Move prebuild cache
run: |
rm -rf /tmp/.buildx-cache/prebuild
mv /tmp/.buildx-cache-new/prebuild /tmp/.buildx-cache/prebuild
- name: Move build cache
run: |
rm -rf /tmp/.buildx-cache/build
mv /tmp/.buildx-cache-new/build /tmp/.buildx-cache/build
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/preview.yml (1)
86-86: Add newline at end of file.mv /tmp/.buildx-cache-new/build /tmp/.buildx-cache/build || true +🧰 Tools
🪛 yamllint (1.35.1)
[error] 86-86: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/preview.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/preview.yml
[error] 86-86: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/preview.yml
Outdated
| @@ -81,6 +81,6 @@ jobs: | |||
| name: Move cache | |||
| run: | | |||
| rm -rf /tmp/.buildx-cache/prebuild | |||
| mv /tmp/.buildx-cache-new/prebuild /tmp/.buildx-cache/prebuild | |||
| mv /tmp/.buildx-cache-new/prebuild /tmp/.buildx-cache/prebuild || true | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ohayo sensei! Consider improving error handling for cache operations.
While adding || true prevents workflow failures, it silently ignores potential cache issues. Consider adding error logging:
- mv /tmp/.buildx-cache-new/prebuild /tmp/.buildx-cache/prebuild || true
- mv /tmp/.buildx-cache-new/build /tmp/.buildx-cache/build || true
+ mv /tmp/.buildx-cache-new/prebuild /tmp/.buildx-cache/prebuild || echo "Warning: Failed to move prebuild cache"
+ mv /tmp/.buildx-cache-new/build /tmp/.buildx-cache/build || echo "Warning: Failed to move build cache"Also applies to: 86-86
Summary by CodeRabbit
New Features
Bug Fixes
Chores