Skip to content

fix(schema): lossy utf8 decoding for bytearrays - #3358

Merged
Larkooo merged 5 commits into
dojoengine:mainfrom
Larkooo:bytearray-unchecked
Oct 9, 2025
Merged

fix(schema): lossy utf8 decoding for bytearrays#3358
Larkooo merged 5 commits into
dojoengine:mainfrom
Larkooo:bytearray-unchecked

Conversation

@Larkooo

@Larkooo Larkooo commented Oct 9, 2025

Copy link
Copy Markdown
Collaborator

Summary by CodeRabbit

  • Bug Fixes

    • Improved byte array decoding to handle non-UTF-8 content gracefully, using tolerant UTF-8 conversion to prevent errors and ensure consistent string output.
    • Resolved edge cases where byte arrays could produce incorrect or failing string results.
  • Chores

    • Updated dependencies to the latest patch versions for cainome and cainome-cairo-serde, bringing minor fixes and compatibility improvements.

@coderabbitai

coderabbitai Bot commented Oct 9, 2025

Copy link
Copy Markdown
Contributor

Caution

Review failed

The pull request is closed.

Ohayo, sensei!

Walkthrough

Updates ByteArray deserialization in crates/dojo/types/src/schema.rs to assign a lossy UTF-8 string after draining consumed felts. Also bumps cainome and cainome-cairo-serde versions in Cargo.toml. No public API changes.

Changes

Cohort / File(s) Summary of Changes
ByteArray deserialization update
crates/dojo/types/src/schema.rs
Adjusts Ty::deserialize ByteArray branch: binds to a String variable and assigns from Cairo-deserialized ByteArray using to_string_lossy() after draining consumed felts. No public signature changes.
Dependency bumps
Cargo.toml
Bumps cainome 0.10.0 → 0.10.1 (with abigen-rs) and cainome-cairo-serde 0.4.0 → 0.4.1. No other config changes.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller
  participant Ty as Ty::deserialize
  participant CS as Cairo Serde
  participant BA as ByteArray

  Caller->>Ty: deserialize(ByteArray, felts)
  Ty->>CS: deserialize(ByteArray from felts)
  CS-->>Ty: ByteArray value + consumed count
  Ty->>BA: drain consumed felts
  Ty->>Ty: string = ByteArray.to_string_lossy()
  Ty-->>Caller: Ty::ByteArray(string)
  note over Ty,Caller: Public type remains ByteArray(String)
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title follows conventional commit style by including the type ‘fix’ and scope ‘schema’ while succinctly summarizing the main change of introducing lossy UTF-8 decoding for bytearrays. It aligns directly with the modifications in the ByteArray handling branch and clearly describes the key update. The phrasing is concise and specific enough for a teammate scanning the history to understand the primary purpose of the changes.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bd07a93 and 94a65d2.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (2)
  • Cargo.toml (1 hunks)
  • crates/dojo/types/src/schema.rs (1 hunks)

Comment @coderabbitai help to get the list of available commands and usage tips.

@Larkooo
Larkooo enabled auto-merge (squash) October 9, 2025 17:04
@Larkooo
Larkooo merged commit 711cb72 into dojoengine:main Oct 9, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants