Skip to content

Commit 4594f13

Browse files
mraszykclaude
andcommitted
refactor: decode Cycles from little-endian bytes via TryFrom
`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>
1 parent eebffee commit 4594f13

7 files changed

Lines changed: 100 additions & 31 deletions

File tree

rs/embedders/tests/system_api.rs

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1523,7 +1523,7 @@ fn cycles_burn128_clamps_to_available_cycles() {
15231523
let mut heap = vec![0; 16];
15241524
api.ic0_canister_liquid_cycle_balance128(0, &mut heap)
15251525
.unwrap();
1526-
let liquid_cycles = Cycles::from(&heap);
1526+
let liquid_cycles = Cycles::try_from(heap.as_slice()).unwrap();
15271527
// Sanity check.
15281528
assert!(liquid_cycles < INITIAL_CYCLES);
15291529
let freeze_limit = INITIAL_CYCLES - liquid_cycles;
@@ -1534,7 +1534,7 @@ fn cycles_burn128_clamps_to_available_cycles() {
15341534
.unwrap();
15351535

15361536
// Only the available cycle balance was burned.
1537-
assert_eq!(liquid_cycles, Cycles::from(&heap));
1537+
assert_eq!(liquid_cycles, Cycles::try_from(heap.as_slice()).unwrap());
15381538

15391539
// The balance is equal to the freeze limit.
15401540
let system_state_modifications = api.take_system_state_modifications();
@@ -1911,19 +1911,22 @@ fn test_ic0_cycles_burn() {
19111911
for _ in 0..2 {
19121912
let mut heap = vec![0; 16];
19131913
api.ic0_cycles_burn128(removed, 0, &mut heap).unwrap();
1914-
assert_eq!(removed, Cycles::from(&heap));
1914+
assert_eq!(removed, Cycles::try_from(heap.as_slice()).unwrap());
19151915
}
19161916

19171917
let mut heap = vec![0; 16];
19181918
api.ic0_cycles_burn128(removed, 0, &mut heap).unwrap();
19191919
// The remaining balance is lower than the amount requested to be burned,
19201920
// hence the system will remove as many cycles as it can.
1921-
assert_eq!(Cycles::new(1_000_000_000_000), Cycles::from(&heap));
1921+
assert_eq!(
1922+
Cycles::new(1_000_000_000_000),
1923+
Cycles::try_from(heap.as_slice()).unwrap()
1924+
);
19221925

19231926
let mut heap = vec![0; 16];
19241927
api.ic0_cycles_burn128(removed, 0, &mut heap).unwrap();
19251928
// There are no more cycles that can be burned.
1926-
assert_eq!(Cycles::new(0), Cycles::from(&heap));
1929+
assert_eq!(Cycles::new(0), Cycles::try_from(heap.as_slice()).unwrap());
19271930
}
19281931

19291932
#[test]

rs/execution_environment/tests/hypervisor.rs

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9322,7 +9322,7 @@ fn invoke_cost_call() {
93229322
let Ok(WasmResult::Reply(bytes)) = res else {
93239323
panic!("Expected reply, got {res:?}");
93249324
};
9325-
let actual_cost = Cycles::from(&bytes);
9325+
let actual_cost = Cycles::try_from(bytes.as_slice()).unwrap();
93269326
assert_eq!(actual_cost, expected_cost,);
93279327
}
93289328

@@ -9342,7 +9342,7 @@ fn invoke_cost_create_canister() {
93429342
let Ok(WasmResult::Reply(bytes)) = res else {
93439343
panic!("Expected reply, got {res:?}");
93449344
};
9345-
let actual_cost = Cycles::from(&bytes);
9345+
let actual_cost = Cycles::try_from(bytes.as_slice()).unwrap();
93469346
assert_eq!(actual_cost, expected_cost.real());
93479347
}
93489348

@@ -9366,7 +9366,7 @@ fn invoke_cost_http_request() {
93669366
let Ok(WasmResult::Reply(bytes)) = res else {
93679367
panic!("Expected reply, got {res:?}");
93689368
};
9369-
let actual_cost = Cycles::from(&bytes);
9369+
let actual_cost = Cycles::try_from(bytes.as_slice()).unwrap();
93709370
assert_eq!(actual_cost, expected_cost.real());
93719371
}
93729372

@@ -9411,7 +9411,7 @@ fn invoke_cost_http_request_v2() {
94119411
test.get_own_subnet_cycles_config(),
94129412
);
94139413
let bytes = get_reply(res);
9414-
let actual_cost = Cycles::from(&bytes);
9414+
let actual_cost = Cycles::try_from(bytes.as_slice()).unwrap();
94159415
assert_eq!(actual_cost, expected_cost.real());
94169416
}
94179417

@@ -9479,7 +9479,7 @@ fn invoke_cost_sign_with_ecdsa() {
94799479
let Ok(WasmResult::Reply(bytes)) = res else {
94809480
panic!("Expected reply, got {res:?}");
94819481
};
9482-
let actual_cost = Cycles::from(&bytes);
9482+
let actual_cost = Cycles::try_from(bytes.as_slice()).unwrap();
94839483
assert_eq!(actual_cost, expected_cost.real());
94849484
}
94859485

@@ -9593,7 +9593,7 @@ fn invoke_cost_sign_with_schnorr() {
95939593
let Ok(WasmResult::Reply(bytes)) = res else {
95949594
panic!("Expected reply, got {res:?}");
95959595
};
9596-
let actual_cost = Cycles::from(&bytes);
9596+
let actual_cost = Cycles::try_from(bytes.as_slice()).unwrap();
95979597
assert_eq!(actual_cost, expected_cost.real());
95989598
}
95999599

@@ -9672,7 +9672,7 @@ fn invoke_cost_vetkd_derive_key() {
96729672
let Ok(WasmResult::Reply(bytes)) = res else {
96739673
panic!("Expected reply, got {res:?}");
96749674
};
9675-
let actual_cost = Cycles::from(&bytes);
9675+
let actual_cost = Cycles::try_from(bytes.as_slice()).unwrap();
96769676
assert_eq!(actual_cost, expected_cost.real());
96779677
}
96789678

rs/state_layout/src/state_layout/proto.rs

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -121,12 +121,14 @@ impl TryFrom<pb_canister_state_bits::CanisterStateBits> for CanisterStateBits {
121121

122122
let cycles_debit = value
123123
.cycles_debit
124-
.map(|c| c.into())
124+
.map(Cycles::try_from)
125+
.transpose()?
125126
.unwrap_or_else(Cycles::zero);
126127

127128
let reserved_balance = value
128129
.reserved_balance
129-
.map(|c| c.into())
130+
.map(Cycles::try_from)
131+
.transpose()?
130132
.unwrap_or_else(Cycles::zero);
131133

132134
let mut consumed_cycles_by_use_cases = BTreeMap::new();
@@ -178,10 +180,14 @@ impl TryFrom<pb_canister_state_bits::CanisterStateBits> for CanisterStateBits {
178180
cycles_balance,
179181
cycles_debit,
180182
reserved_balance,
181-
reserved_balance_limit: value.reserved_balance_limit.map(|v| v.into()),
183+
reserved_balance_limit: value
184+
.reserved_balance_limit
185+
.map(Cycles::try_from)
186+
.transpose()?,
182187
minimum_incoming_canister_call_cycles: value
183188
.minimum_incoming_canister_call_cycles
184-
.map(|v| v.into())
189+
.map(Cycles::try_from)
190+
.transpose()?
185191
.unwrap_or_default(),
186192
status: try_from_option_field(
187193
value.canister_status,

rs/types/cycles/src/compound_cycles.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,7 @@ impl<T: CyclesUseCaseKind> TryFrom<PbCompoundCycles> for CompoundCycles<T> {
215215
.ok_or(ProxyDecodeError::MissingField("CompoundCycles::nominal"))?,
216216
)?;
217217
Ok(CompoundCycles {
218-
real: Cycles::from(real),
218+
real: Cycles::try_from(real)?,
219219
nominal,
220220
_cycles_use_case_marker: PhantomData,
221221
})

rs/types/cycles/src/cycles.rs

Lines changed: 67 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
use candid::{CandidType, Nat};
22
use ic_heap_bytes::DeterministicHeapBytes;
3+
use ic_protobuf::proxy::ProxyDecodeError;
34
use ic_protobuf::state::canister_state_bits::v1::CyclesAccount as pbCyclesAccount;
45
use ic_protobuf::state::queues::v1::Cycles as PbCycles;
56
use serde::{Deserialize, Serialize};
6-
use std::convert::TryInto;
7+
use std::array::TryFromSliceError;
78
use std::iter::Sum;
89
use std::{
910
fmt,
@@ -92,9 +93,13 @@ impl From<u64> for Cycles {
9293
}
9394
}
9495

95-
impl From<&Vec<u8>> for Cycles {
96-
fn from(bytes: &Vec<u8>) -> Self {
97-
Self::new(u128::from_le_bytes(bytes.as_slice().try_into().unwrap()))
96+
/// Decodes `Cycles` from their little-endian representation, as produced by
97+
/// `From<Cycles> for Vec<u8>`. Fails if `bytes` is not exactly 16 bytes long.
98+
impl TryFrom<&[u8]> for Cycles {
99+
type Error = TryFromSliceError;
100+
101+
fn try_from(bytes: &[u8]) -> Result<Self, Self::Error> {
102+
Ok(Self::new(u128::from_le_bytes(bytes.try_into()?)))
98103
}
99104
}
100105

@@ -212,9 +217,11 @@ impl From<Cycles> for PbCycles {
212217
}
213218
}
214219

215-
impl From<PbCycles> for Cycles {
216-
fn from(item: PbCycles) -> Self {
217-
Self::from(&item.raw_cycles)
220+
impl TryFrom<PbCycles> for Cycles {
221+
type Error = ProxyDecodeError;
222+
223+
fn try_from(item: PbCycles) -> Result<Self, Self::Error> {
224+
try_from_le_bytes(&item.raw_cycles)
218225
}
219226
}
220227

@@ -226,12 +233,23 @@ impl From<Cycles> for pbCyclesAccount {
226233
}
227234
}
228235

229-
impl From<pbCyclesAccount> for Cycles {
230-
fn from(value: pbCyclesAccount) -> Self {
231-
Self::from(&value.cycles_balance)
236+
impl TryFrom<pbCyclesAccount> for Cycles {
237+
type Error = ProxyDecodeError;
238+
239+
fn try_from(value: pbCyclesAccount) -> Result<Self, Self::Error> {
240+
try_from_le_bytes(&value.cycles_balance)
232241
}
233242
}
234243

244+
/// Decodes `Cycles` from the little-endian representation used by the protobuf
245+
/// encodings above, mapping a length mismatch onto a `ProxyDecodeError`.
246+
fn try_from_le_bytes(bytes: &[u8]) -> Result<Cycles, ProxyDecodeError> {
247+
Cycles::try_from(bytes).map_err(|_| ProxyDecodeError::ValueOutOfRange {
248+
typ: "Cycles",
249+
err: format!("expected 16 bytes, got {}", bytes.len()),
250+
})
251+
}
252+
235253
#[cfg(test)]
236254
mod test {
237255
use super::*;
@@ -373,4 +391,43 @@ mod test {
373391
"Cycles(340282366920938463463374607431768211455)"
374392
);
375393
}
394+
395+
#[test]
396+
fn test_le_bytes_roundtrip() {
397+
for cycles in [Cycles::zero(), Cycles::new(1), Cycles::new(u128::MAX)] {
398+
let bytes: Vec<u8> = cycles.into();
399+
assert_eq!(bytes.len(), 16);
400+
assert_eq!(Cycles::try_from(bytes.as_slice()).unwrap(), cycles);
401+
}
402+
}
403+
404+
#[test]
405+
fn test_try_from_le_bytes_of_wrong_length_fails() {
406+
for len in [0, 15, 17] {
407+
assert!(Cycles::try_from(vec![0; len].as_slice()).is_err());
408+
}
409+
}
410+
411+
#[test]
412+
fn test_try_from_proto_with_wrong_length_fails() {
413+
for len in [0, 15, 17] {
414+
let err = Cycles::try_from(PbCycles {
415+
raw_cycles: vec![0; len],
416+
})
417+
.unwrap_err();
418+
assert!(
419+
matches!(err, ProxyDecodeError::ValueOutOfRange { typ: "Cycles", .. }),
420+
"unexpected error: {err:?}"
421+
);
422+
423+
let err = Cycles::try_from(pbCyclesAccount {
424+
cycles_balance: vec![0; len],
425+
})
426+
.unwrap_err();
427+
assert!(
428+
matches!(err, ProxyDecodeError::ValueOutOfRange { typ: "Cycles", .. }),
429+
"unexpected error: {err:?}"
430+
);
431+
}
432+
}
376433
}

rs/types/types/src/canister_http.rs

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -380,15 +380,18 @@ impl TryFrom<pb_metadata::CanisterHttpRequestContext> for CanisterHttpRequestCon
380380
Some(refund_status) => RefundStatus {
381381
refundable_cycles: refund_status
382382
.refundable_cycles
383-
.map(Into::into)
383+
.map(Cycles::try_from)
384+
.transpose()?
384385
.unwrap_or_default(),
385386
per_replica_allowance: refund_status
386387
.per_replica_allowance
387-
.map(Into::into)
388+
.map(Cycles::try_from)
389+
.transpose()?
388390
.unwrap_or_default(),
389391
refunded_cycles: refund_status
390392
.refunded_cycles
391-
.map(Into::into)
393+
.map(Cycles::try_from)
394+
.transpose()?
392395
.unwrap_or_default(),
393396
refunding_nodes: refund_status
394397
.refunding_nodes

rs/types/types/src/methods.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -381,7 +381,7 @@ impl TryFrom<pb::Callback> for Callback {
381381
Ok(Self {
382382
call_context_id: CallContextId::from(value.call_context_id),
383383
respondent: try_from_option_field(value.respondent, "Callback::respondent")?,
384-
cycles_sent: Cycles::from(cycles_sent),
384+
cycles_sent: Cycles::try_from(cycles_sent)?,
385385
prepayment_for_response_execution,
386386
prepayment_for_response_transmission,
387387
prepayment_for_call_transmission,

0 commit comments

Comments
 (0)