Add codec toolkit and fixed-point helpers#253
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a deterministic binary codec (Reader/Writer, Encode/Decode, errors, helpers) and Q32.32 fixed-point types across two crates, plus unit tests and public module exports to expose the new functionality. Changes are purely additive. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb68e4149d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub fn fx_from_i64(n: i64) -> i64 { | ||
| n << 32 |
There was a problem hiding this comment.
fx_from_i64 shifts n left by 32 bits without any range check. For inputs outside the Q32.32 integer range (roughly [-2^31, 2^31-1]), this overflows: in debug builds it panics, while in release it wraps, yielding incorrect fixed-point values and behavior that depends on build profile. If callers can pass large i64 values, this will corrupt coordinates or crash; consider clamping, returning a Result, or documenting/enforcing the valid range.
Useful? React with 👍 / 👎.
| /// Construct from an integer value (n << 32). | ||
| #[must_use] | ||
| pub fn from_i64(n: i64) -> Self { | ||
| Self(n << 32) |
There was a problem hiding this comment.
Fx32::from_i64 performs n << 32 directly, which overflows for any i64 outside the Q32.32 representable integer range. That overflow panics in debug and wraps in release, so the same input can crash in tests but silently produce invalid fixed-point values in production. If callers can pass large integers, this is a correctness bug; add range checks or return a fallible conversion.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@crates/echo-wasm-abi/src/codec.rs`:
- Around line 128-135: The take method performs unchecked addition with
self.offset + len which can overflow; replace that with a checked addition:
compute end = self.offset.checked_add(len).ok_or(CodecError::OutOfBounds)? and
then verify end <= self.bytes.len() before slicing, return
Err(CodecError::OutOfBounds) on overflow or if end > self.bytes.len(), set
self.offset = end and return the slice; update the function named take to use
checked_add on self.offset and len and check end against self.bytes.len().
In `@crates/echo-wasm-abi/src/lib.rs`:
- Line 18: The public module `codec` declared as `pub mod codec;` is missing
rustdoc; add a concise doc comment above the declaration describing the module's
purpose and public API surface (e.g., what encoding/decoding utilities or types
it exposes) so it satisfies the project's public‑API documentation requirement
and tools like cargo doc; update the comment near `pub mod codec` in lib.rs to a
short, clear rustdoc paragraph.
In `@crates/echo-wasm-abi/tests/codec.rs`:
- Line 1: Add the required SPDX/license and copyright header at the very top of
the file before the existing import line (the line starting with use
echo_wasm_abi::codec::{Reader, Writer, fx_from_i64, fx_from_f32,
vec3_fx_from_i64, vec3_fx_from_f32};) so the file begins with the mandated
SPDX/license block followed by the copyright notice, then keep the existing use
statement unchanged.
- Around line 23-26: The test has two issues: the expected value in the
vec3_fx_from_f32 assertion suffers from Rust operator-precedence (the intended
(-2i64 << 32) - (1i64 << 31) is parsed incorrectly), and the file is missing the
required SPDX license header. Fix by adding the standard SPDX header as the
first line of the file, and update the assertion that references
vec3_fx_from_f32 to use explicit parentheses so the negative left-shift is
grouped and the subtraction applies after the shift (i.e., make the middle
element computed as ((-2i64 << 32) - (1i64 << 31))) while keeping the other
fx_from_i64 and fx_from_f32 assertions intact; use the function names
fx_from_i64, fx_from_f32, and vec3_fx_from_f32 to locate the assertions.
In `@crates/warp-core/src/fixed.rs`:
- Around line 31-35: The public struct Vec3Fx is missing rustdoc for its public
fields; add short doc comments for the fields x, y, and z on the Vec3Fx struct
(the Fx32-typed components) describing that they represent the fixed-point X, Y,
and Z coordinates respectively. Update the Vec3Fx declaration so each field (x,
y, z) has a one-line /// comment explaining its role and units/format (e.g.,
fixed-point value) to satisfy CI and the public API docs requirement.
♻️ Duplicate comments (2)
crates/warp-core/src/fixed.rs (1)
10-14: GuardFx32::from_i64against Q32.32 overflow/wrap.Line 12 shifts left without a range guard; out‑of‑range integers wrap and silently corrupt fixed‑point values. Clamp or make the conversion fallible and document the valid range.
🐛 Suggested fix (clamp to representable range)
pub fn from_i64(n: i64) -> Self { - Self(n << 32) + const MIN: i64 = i64::MIN >> 32; + const MAX: i64 = i64::MAX >> 32; + let clamped = n.clamp(MIN, MAX); + Self(clamped << 32) }crates/echo-wasm-abi/src/codec.rs (1)
182-187:fx_from_i64silently wraps for out‑of‑range inputs.Line 186 shifts left without guarding the Q32.32 range, so large integers yield incorrect fixed‑point values. Clamp or make the conversion fallible and document the range.
🐛 Suggested fix (clamp to representable range)
pub fn fx_from_i64(n: i64) -> i64 { - n << 32 + const MIN: i64 = i64::MIN >> 32; + const MAX: i64 = i64::MAX >> 32; + let clamped = n.clamp(MIN, MAX); + clamped << 32 }
# Conflicts: # crates/warp-core/src/lib.rs
- Add overflow protection to fx_from_i64 and Fx32::from_i64 (saturate to i64::MIN/MAX for inputs outside i32 range) - Use checked_add in Reader::take to prevent integer overflow - Add rustdoc for codec module in lib.rs - Update CHANGELOG with codec and fixed module additions Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/echo-wasm-abi/src/codec.rs`:
- Around line 39-50: The current decode_from_bytes function allows trailing
bytes to be ignored; update decode_from_bytes (and Reader usage) to enforce full
consumption by checking the reader after T::decode(&mut reader) and returning a
new CodecError variant (e.g., CodecError::TrailingBytes or similar) if any bytes
remain; add that variant to the CodecError enum and return Err when
reader.has_remaining()/remaining() != 0 (or !reader.is_eof()) so decoding is
strict and rejects inputs with extra data.
| /// Encode a value into a fresh Vec. | ||
| pub fn encode_to_vec<T: Encode>(value: &T) -> Result<Vec<u8>, CodecError> { | ||
| let mut writer = Writer::default(); | ||
| value.encode(&mut writer)?; | ||
| Ok(writer.into_vec()) | ||
| } | ||
|
|
||
| /// Decode a value from a byte slice. | ||
| pub fn decode_from_bytes<T: Decode>(bytes: &[u8]) -> Result<T, CodecError> { | ||
| let mut reader = Reader::new(bytes); | ||
| T::decode(&mut reader) | ||
| } |
There was a problem hiding this comment.
Reject trailing bytes to keep decoding canonical.
decode_from_bytes decodes a prefix and silently ignores any remaining bytes. That permits multiple encodings for the same value and can hide appended data. For a deterministic codec, this is a correctness and integrity risk. Enforce full consumption or provide a strict variant.
🛠️ Proposed fix (add TrailingBytes + strict check)
pub enum CodecError {
@@
/// Enum decoding failed.
#[error("invalid enum value")]
InvalidEnum,
+ /// Decoded value but leftover bytes remained.
+ #[error("trailing bytes")]
+ TrailingBytes,
} pub fn decode_from_bytes<T: Decode>(bytes: &[u8]) -> Result<T, CodecError> {
let mut reader = Reader::new(bytes);
- T::decode(&mut reader)
+ let value = T::decode(&mut reader)?;
+ if reader.offset != bytes.len() {
+ return Err(CodecError::TrailingBytes);
+ }
+ Ok(value)
}📝 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.
| /// Encode a value into a fresh Vec. | |
| pub fn encode_to_vec<T: Encode>(value: &T) -> Result<Vec<u8>, CodecError> { | |
| let mut writer = Writer::default(); | |
| value.encode(&mut writer)?; | |
| Ok(writer.into_vec()) | |
| } | |
| /// Decode a value from a byte slice. | |
| pub fn decode_from_bytes<T: Decode>(bytes: &[u8]) -> Result<T, CodecError> { | |
| let mut reader = Reader::new(bytes); | |
| T::decode(&mut reader) | |
| } | |
| /// Encode a value into a fresh Vec. | |
| pub fn encode_to_vec<T: Encode>(value: &T) -> Result<Vec<u8>, CodecError> { | |
| let mut writer = Writer::default(); | |
| value.encode(&mut writer)?; | |
| Ok(writer.into_vec()) | |
| } | |
| /// Decode a value from a byte slice. | |
| pub fn decode_from_bytes<T: Decode>(bytes: &[u8]) -> Result<T, CodecError> { | |
| let mut reader = Reader::new(bytes); | |
| let value = T::decode(&mut reader)?; | |
| if reader.offset != bytes.len() { | |
| return Err(CodecError::TrailingBytes); | |
| } | |
| Ok(value) | |
| } |
🤖 Prompt for AI Agents
In `@crates/echo-wasm-abi/src/codec.rs` around lines 39 - 50, The current
decode_from_bytes function allows trailing bytes to be ignored; update
decode_from_bytes (and Reader usage) to enforce full consumption by checking the
reader after T::decode(&mut reader) and returning a new CodecError variant
(e.g., CodecError::TrailingBytes or similar) if any bytes remain; add that
variant to the CodecError enum and return Err when
reader.has_remaining()/remaining() != 0 (or !reader.is_eof()) so decoding is
strict and rejects inputs with extra data.
1 participant
Adds codec toolkit to help make writing encode/decode easier. Eventually Wesley will generate this stuff automatically.