Skip to content

refactor: decode Cycles from little-endian bytes via TryFrom - #11102

Merged
mraszyk merged 2 commits into
masterfrom
mraszyk/cycles-try-from-le-bytes
Aug 12, 2026
Merged

refactor: decode Cycles from little-endian bytes via TryFrom#11102
mraszyk merged 2 commits into
masterfrom
mraszyk/cycles-try-from-le-bytes

Conversation

@mraszyk

@mraszyk mraszyk commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Cycles was decoded from its little-endian byte representation through an infallible From<&Vec<u8>> impl, which left it with no way to report a length mismatch to its caller. Replace it with TryFrom<&Vec<u8>>.

The two protobuf conversions built on it, state.queues.v1.Cycles and canister_state_bits.v1.CyclesAccount, become TryFrom impls returning ProxyDecodeError::ValueOutOfRange, so a length mismatch is now reported like any other proto decoding error. This matches how NominalCycles and CompoundCycles already decode in the same crate.

The try_from_option_field call sites (Request::cycles_payment, Response::cycles_refund, CallContext::available_cycles, CanisterStateBits::cycles_balance, Refund::amount) need no change: they already required a TryFrom whose error converts into ProxyDecodeError and were satisfied through the blanket impl. The callers that used the infallible impls directly now propagate the error instead: CompoundCycles::real, Callback::cycles_sent, the three RefundStatus fields, and four CanisterStateBits fields in state_layout.

The encoding is unchanged, so this only affects the decoding failure path.

`Cycles` was decoded from its little-endian byte representation through an
infallible `From<&Vec<u8>>` impl, which left it with no way to report a
length mismatch to its caller. Replace it with `TryFrom<&[u8]>`.

The two protobuf conversions built on it, `state.queues.v1.Cycles` and
`canister_state_bits.v1.CyclesAccount`, become `TryFrom` impls returning
`ProxyDecodeError::ValueOutOfRange`, so a length mismatch is now reported
like any other proto decoding error. This matches how `NominalCycles` and
`CompoundCycles` already decode in the same crate.

The `try_from_option_field` call sites (`Request::cycles_payment`,
`Response::cycles_refund`, `CallContext::available_cycles`,
`CanisterStateBits::cycles_balance`, `Refund::amount`) need no change: they
already required a `TryFrom` whose error converts into `ProxyDecodeError`
and were satisfied through the blanket impl. The callers that used the
infallible impls directly now propagate the error instead:
`CompoundCycles::real`, `Callback::cycles_sent`, the three `RefundStatus`
fields, and four `CanisterStateBits` fields in `state_layout`.

The encoding is unchanged, so this only affects the decoding failure path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors Cycles decoding to report invalid byte lengths instead of panicking.

Changes:

  • Adds fallible byte-slice and protobuf conversions.
  • Propagates decode errors through state deserialization.
  • Updates tests and direct decoding call sites.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
rs/types/types/src/methods.rs Propagates callback cycle decode errors.
rs/types/types/src/canister_http.rs Validates refund cycle fields.
rs/types/cycles/src/cycles.rs Implements fallible decoding and tests.
rs/types/cycles/src/compound_cycles.rs Propagates real-cycle decode errors.
rs/state_layout/src/state_layout/proto.rs Validates persisted cycle fields.
rs/execution_environment/tests/hypervisor.rs Updates test decoding calls.
rs/embedders/tests/system_api.rs Updates system API test decoding.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@mraszyk
mraszyk marked this pull request as ready for review August 11, 2026 15:18
@mraszyk
mraszyk requested a review from a team as a code owner August 11, 2026 15:18
@zeropath-ai

zeropath-ai Bot commented Aug 11, 2026

Copy link
Copy Markdown

No security or compliance issues detected. Reviewed everything up to dbb9ec6.

Security Overview
Detected Code Changes
Change Type Relevant files
Enhancement ► rs/embedders/tests/system_api.rs
    Use Cycles::try_from(&heap) instead of Cycles::from(&heap) where decoding from bytes is performed
► rs/execution_environment/tests/hypervisor.rs
    Switch to Cycles::try_from(&bytes).unwrap() for cost decoding
► rs/state_layout/src/state_layout/proto.rs
    Use Cycles::try_from with transpose for optional fields (cycles_debit, reserved_balance, reserved_balance_limit, minimum_incoming_canister_call_cycles)
Enhancement ► rs/types/cycles/src/compound_cycles.rs
    Decode real field with Cycles::try_from(real) instead of Cycles::from(real)
Enhancement ► rs/types/cycles/src/cycles.rs
    Introduce TryFrom<&Vec> for Cycles using try_from on LE bytes
    Change PbCycles -> Cycles and pbCyclesAccount -> Cycles implementations to TryFrom with proper error handling
    Add helper try_from_le_bytes to map length errors to ProxyDecodeError
    Add tests for roundtrip, wrong length handling, and proto mismatch cases
Enhancement ► rs/types/types/src/canister_http.rs
    In CanisterHttpRequestContext, convert Cycles fields using Cycles::try_from with proper transpose handling
Enhancement ► rs/types/types/src/methods.rs
    In Callback, convert cycles_sent using Cycles::try_from instead of Cycles::from

Comment thread rs/types/cycles/src/cycles.rs Outdated
Every caller of the little-endian decoding impl holds a `Vec<u8>`, so a
`TryFrom<&[u8]>` impl forced all of them to spell out an `as_slice()`.
Take a `&Vec<u8>` instead and drop the 13 `as_slice()` calls.

A blanket `impl<T: AsRef<[u8]>> TryFrom<T> for Cycles`, which would have
covered both, is not possible: it conflicts with the
`impl<T, U: Into<T>> TryFrom<U> for T` in `core` (E0119). The doc comment
on the impl records this.

The private `try_from_le_bytes` helper used by the two protobuf
conversions now takes its `Vec<u8>` by value, which its callers already
own; a `&Vec<u8>` parameter would trip `clippy::ptr_arg`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mraszyk
mraszyk added this pull request to the merge queue Aug 12, 2026
Merged via the queue into master with commit b5c69fc Aug 12, 2026
41 checks passed
@mraszyk
mraszyk deleted the mraszyk/cycles-try-from-le-bytes branch August 12, 2026 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants