acp-176/226 refactor to common package #4292
Conversation
ceyonur
changed the title
ceyonur
changed the base branch from
master
to
ceyonur/acp-226-math-package
ceyonur
force-pushed
the
ceyonur/acp-176-226-refactor
branch
from
a9463c2 to
4b1dd83
Compare
There was a problem hiding this comment.
Pull Request Overview
Refactors common functionality from ACP-176 and ACP-226 implementations into a shared common package to reduce code duplication and improve maintainability.
- Introduces
TargetExcessParamsstructure to encapsulate parameters and functions for target excess calculations - Moves exponential calculation logic from gas package to a new math utility function
- Updates both ACP implementations to use the shared common package functions
Reviewed Changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vms/evm/upgrades/common/target_excess.go | New common package with shared target excess calculation functions and parameters structure |
| vms/evm/upgrades/acp226/acp226.go | Refactored to use common package functions, removing duplicate implementation |
| vms/evm/upgrades/acp176/acp176.go | Refactored to use common package functions, removing duplicate implementation |
| vms/components/gas/gas.go | Simplified to use new math utility function for exponential calculations |
| utils/math/exponential.go | New utility function extracted from gas package for exponential calculations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -154,7 +157,7 @@ func (s *State) UpdateTargetExcess(desiredTargetExcess gas.Gas) { | |||
| ) | |||
|
|
|||
| // Ensure the gas capacity does not exceed the maximum capacity. | |||
| newMaxCapacity := mulWithUpperBound(newTargetPerSecond, TargetToMaxCapacity) // C | |||
| newMaxCapacity := gas.Gas(common.MulWithUpperBound(uint64(newTargetPerSecond), TargetToMaxCapacity)) // C | |||
| s.Gas.Capacity = min(s.Gas.Capacity, newMaxCapacity) | |||
There was a problem hiding this comment.
this now uses golang builtin min, would it be a problem? cc @StephenButtolph
michaelkaplan13
left a comment
michaelkaplan13
left a comment
There was a problem hiding this comment.
One nit, and think we should update/remove the "target" terminology from target_excess.go since it's not just applying to the gas target anymore, but otherwise LGTM.
Signed-off-by: Ceyhun Onur <[email protected]>
…alanchego into ceyonur/acp-176-226-refactor
|
This PR has become stale because it has been open for 30 days with no activity. Adding the |
There was a problem hiding this comment.
Can we add tests for this new code?
There was a problem hiding this comment.
Can we add tests for new code?
|
This PR has become stale because it has been open for 30 days with no activity. Adding the |
Why this should be merged
Refactors common parts from acp-176 and acp-226 into
commonpackage.How this works
Uses
TargetExcessParamstructure to define common functions, so that different implementations/parameters can use the same functionsHow this was tested
existing tests
Need to be documented in RELEASES.md?
no