refactor: Introduce new type CompoundCycles#9520
Draft
dsarlis wants to merge 1 commit intodfinity:masterfrom
Draft
refactor: Introduce new type CompoundCycles#9520dsarlis wants to merge 1 commit intodfinity:masterfrom
dsarlis wants to merge 1 commit intodfinity:masterfrom
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
CyclesAccountManagerwere changed to not require acost_scheduleparameter, so each caller would then need to have some logic like: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
CompoundCyclesis introduced that given an original amount ofCycles, 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
CompoundCyclesobject and when they need to updateCyclesamounts in theReplicatedStatethey can usecompound_cycles.real()while they can usecompound_cycles.nominal()forNominalCyclesamounts. 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
CyclesAccountManagerAPIs:in the above there's no guarantee that the user of the API does not use
CompoundCyclesforMemoryinstead ofInstructionsthat they should use and pass an incorrect amount inrefund_unused_execution_cycleswithout noticing. Additionally, in order to calculate the refund we should implement arithmetic operations onCompoundCyclesbut 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
CompoundCyclesto 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 theCyclesAccountManagerlike the following:The changes included can be summarized as follows:
CyclesAccountManagerandReplicatedStateare 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 existingCyclesandNominalCyclestypes.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:
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 ofCompoundCyclesas well as some test updates to fix assertions (and/or maybe adding some more tests as I don't think all cases are covered).