refactor: Restructure cycles account manager crate#9165
Conversation
|
@basvandijk I see some timeouts in the tests, can you please restart the job? I am quite certain the code changes here are irrelevant. |
|
The I restarted the job but I would expect it to fail again. Maybe it's an issue of using two different clock domains like the system time and the StateMachine time in the same test. |
I'm not sure what this is about but I can say with a lot of confidence my changes did not produce it. For reference, I've seen this issue in some local runs when working on registry changes but not on CI (so I kinda looked away and not in detail). |
Prompting Claude Opus 4.6:
Caused it to use our claude/skills/fix-flaky-tests/SKILL.md and it created: #9172. |
The AI did its job here and found out a fix given other patterns in existing tests. However, with my knowledge of the IC internals and additionally state machine tests internals, I found this explanation superficial and not really convincing. I know that in state machine tests time progresses typically via executing messages. The test also executes messages in a loop and in those messages we will "tick" (state machine nomenclature), meaning we will execute a batch with the requested message, which should be advancing time. So something doesn't add up. Looking more closely, it seems that the tick is artificially set to 1ns always regardless of what kind of processing happened. So this explains I think why (i) you only see these failures some times, you need things to be somewhat overloaded so that you drift much further than this 1ns implies and (ii) why a fix is to simply reset time in each loop iteration. Given all the above, I would argue the "right" solution is to fix "ticking" in state machine tests to be more realistically representing how much time was spent processing a batch and not have the user "magically" reset the time to "fix" something. This way we would represent reality closer where you will get a new batch at a specific time that is related to when the previous state was produced (the faster execution is, the shorter that interval would be and vice versa up to some limit but that's all details you can skip in state machine tests and just advance time more realistically). |
|
@michael-weigelt Can you please merge this? |
This PR refactors the cycles account manager crate to make it more manageable. The code was all in `lib.rs` which had grown quite big. Following similar structure to other crates, the main data structure is moved to its own module with a sub-module `types.rs` to hold some of the related data structures. Unit tests are moved in their own `tests.rs` as well. Along the way some minor cleanups are performed (noticed during the refactoring and that's why they are bundled): `IngressInductionError` is removed as it was not used anywhere and a free function is moved to be a method on the `CyclesAccountManager` and there is no need to export it separately (the place where it was used had access to `CyclesAccountManager` already). This will set things up for further changes that will move `NominalCycles` in the `CyclesAccountManager` which comes with its own boilerplate implementation of arithmetic operations and so having things more modular will make it easier to navigate.
This PR refactors the cycles account manager crate to make it more manageable. The code was all in
lib.rswhich had grown quite big. Following similar structure to other crates, the main data structure is moved to its own module with a sub-moduletypes.rsto hold some of the related data structures. Unit tests are moved in their owntests.rsas well.Along the way some minor cleanups are performed (noticed during the refactoring and that's why they are bundled):
IngressInductionErroris removed as it was not used anywhere and a free function is moved to be a method on theCyclesAccountManagerand there is no need to export it separately (the place where it was used had access toCyclesAccountManageralready).This will set things up for further changes that will move
NominalCyclesin theCyclesAccountManagerwhich comes with its own boilerplate implementation of arithmetic operations and so having things more modular will make it easier to navigate.