Skip to content

refactor: Introduce new type CompoundCycles#9520

Draft
dsarlis wants to merge 1 commit intodfinity:masterfrom
dsarlis:dimitris/separate-nominal-cycles
Draft

refactor: Introduce new type CompoundCycles#9520
dsarlis wants to merge 1 commit intodfinity:masterfrom
dsarlis:dimitris/separate-nominal-cycles

Conversation

@dsarlis
Copy link
Contributor

@dsarlis dsarlis commented Mar 20, 2026

This PR is an alternative to the original approach in #9063.

The original PR, while achieving the goal of having cycles for metrics be always updated irrespective of the subnet's cycles cost schedule, it achieved so by pushing the decision making to all callers that need to deal with cycles charges or metric updates. Specifically, the APIs in the CyclesAccountManager were changed to not require a cost_schedule parameter, so each caller would then need to have some logic like:

match cost_schedule {
   CanisterCyclesCostSchedule::Free => { ... },
   CanisterCyclesCostSchedule::Normal => { ... },
}

This was not ideal as the decision points were spread across the codebase making it also easier for mistakes to be introduced if new use cases were added as every caller would need to have all the knowledge of how to handle things.

The current PR proposes another approach which attempts to encapsulate better the logic around calculating the right amounts for cycles balance and metrics updates. A new type called CompoundCycles is introduced that given an original amount of Cycles, the use case and cost schedule, calculates the "real" and "nominal" amounts that can be used to update the cycles balance and metrics respectively.

This is an improvement over the original approach as now the callers would have access to a CompoundCycles object and when they need to update Cycles amounts in the ReplicatedState they can use compound_cycles.real() while they can use compound_cycles.nominal() for NominalCycles amounts. This way the type system helps guide them to use the right amounts without having to do calculations themselves.

However, making the new type capture more information than before introduces a new risk, namely that one could mix amounts for different use cases or even cost schedule. Imagine the following update in one of the CyclesAccountManager APIs:

pub fn prepay_execution_cycles(instructions: NumInstructions, ...) -> Result<CompoundCycles, CanisterOutOfCyclesError>;

pub fn refund_unused_execution_cycles(instructions: NumInstructions, prepaid_execution_cycles: CompoundCycles, ...);

in the above there's no guarantee that the user of the API does not use CompoundCycles for Memory instead of Instructions that they should use and pass an incorrect amount in refund_unused_execution_cycles without noticing. Additionally, in order to calculate the refund we should implement arithmetic operations on CompoundCycles but it also doesn't make sense to allow adding up compound cycles for different use cases.

One solution to the above issue would be to make the operations fallible but then there would have to be a lot of error handling sprinkled all over the codebase. The PR takes another path which is to introduce generic arguments for CompoundCycles to capture the combinations of use cases. This ensures that operations can only happen between "apples" and "apples" and it also provides a nice way to clarify even more the APIs in the CyclesAccountManager like the following:

pub fn prepay_execution_cycles(instructions: NumInstructions, ...) -> Result<CompoundCycles<Instructions>, CanisterOutOfCyclesError>;

pub fn refund_unused_execution_cycles(instructions: NumInstructions, prepaid_execution_cycles: CompoundCycles<Instructions>, ...);

The changes included can be summarized as follows:

  1. The new type is introduced along with its own arithmetic.
  2. Empty structs that act as tags are added for each use case variant to lift them up to the type level and be able to implement a trait for them to bound the generic argument used in the new type.
  3. The APIs in the CyclesAccountManager and ReplicatedState are transformed to require or return the new type when they would need to deal with both the balance and metrics updates eventually. APIs that are only concerned with the balance of the canister or only with metrics are still using the existing Cycles and NominalCycles types.
  4. A lot of boilerplate changes follow after the above, with a big chunk of them being to update tests.

PS1: The idea has been implemented for the use case argument but not for the cost schedule yet. I suppose it'd have to be implemented for that too but then you'd probably end up with something like:

pub fn prepay_execution_cycles<S>(instructions: NumInstructions, ...) -> Result<CompoundCycles<Instructions, S>, CanisterOutOfCyclesError>;

PS2: The current PR performs only a refactoring to introduce the new type and update APIs but is still using the same value for "real" and "nominal" parts of CompoundCycles. If the approach is agreed, then a follow up to perform the functional changes would be rather small -- a trivial change in the constructor of CompoundCycles as well as some test updates to fix assertions (and/or maybe adding some more tests as I don't think all cases are covered).

@dsarlis dsarlis changed the title refactor: New type CompoundCycles to decouple real from nominal cycles updates refactor: Introduce new type CompoundCycles Mar 20, 2026
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.

1 participant