refactor(gastime): Remove hooks imports#5288
Conversation
dd5b380 to
b48c47b
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors SAEVM gas-time accounting to remove gastime’s dependency on the hook package, enabling gastime to be imported from within hook (as described in the PR motivation).
Changes:
- Moved
GasPriceConfigfromvms/saevm/hookintovms/saevm/gastimeand updated hook interfaces/callers accordingly. - Updated
gastime.Time.BeforeBlock/AfterBlockto accept explicit timestamp/config inputs instead of ahook.Points+ header, and rewired call sites in execution and worst-case tracking. - Updated tests and Bazel deps to reflect the new package boundaries.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vms/saevm/worstcase/state_test.go | Updates worst-case tests to call gastime.Time with explicit timestamp/config inputs. |
| vms/saevm/worstcase/state.go | Rewires worst-case block accounting to pass SubSecondBlockTime and (target,cfg) explicitly. |
| vms/saevm/saexec/execution.go | Updates execution-time gas clock updates to use the new BeforeBlock/AfterBlock signatures. |
| vms/saevm/hook/hookstest/stub.go | Switches stub config type to gastime.GasPriceConfig and uses gastime.DefaultGasPriceConfig(). |
| vms/saevm/hook/hookstest/BUILD.bazel | Adds //vms/saevm/gastime dep for hookstest. |
| vms/saevm/hook/hook.go | Changes Points.GasConfigAfter to return gastime.GasPriceConfig; removes old hook.GasPriceConfig. |
| vms/saevm/hook/BUILD.bazel | Adds //vms/saevm/gastime dep for hook. |
| vms/saevm/gastime/gastime_test.go | Updates tests to use gastime.GasPriceConfig and new error expectations. |
| vms/saevm/gastime/gastime.go | Removes hook/types imports, inlines validation, updates SubSecond signature and config handling. |
| vms/saevm/gastime/config.go | Introduces exported gastime.GasPriceConfig with validation and equality helper. |
| vms/saevm/gastime/config.canoto.go | Regenerated canoto bindings for the renamed/exported config type. |
| vms/saevm/gastime/cmpopt.go | Updates cmp options for the new GasPriceConfig type and canoto cache type. |
| vms/saevm/gastime/acp176_test.go | Refactors tests/fuzzing to use the new explicit BeforeBlock/AfterBlock APIs. |
| vms/saevm/gastime/acp176.go | Refactors core ACP-176 transition logic to take explicit inputs (no hooks import). |
| vms/saevm/gastime/BUILD.bazel | Removes hook/libevm types deps now that gastime no longer imports them. |
| vms/saevm/blocks/time.go | Updates gastime.SubSecond call site to pass a time.Duration directly. |
| vms/saevm/blocks/execution_test.go | Updates helper signature to use gastime.GasPriceConfig. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ARR4N
left a comment
There was a problem hiding this comment.
I know that @StephenButtolph is also doing a lot of work in these files right now. I'll DM the two of you to figure out the ordering.
| // DefaultGasPriceConfig returns the default [GasPriceConfig] values. | ||
| func DefaultGasPriceConfig() GasPriceConfig { |
There was a problem hiding this comment.
I know this really isn't related to this PR, but DefaultGasPriceConfig (and the corresponding DefaultTargetToExcessScaling and DefaultMinPrice) feel odd to me.
They aren't actually defaults, we will error if the zero values are provided (rather than defaulting to them).
Not required for this PR, (and someone else could push back on this) but I feel like these should be removed / moved into a test file.
c52797d to
d11bf40
Compare
d11bf40 to
9bcdd8e
Compare
ac57791 to
3302a34
Compare
ARR4N
left a comment
There was a problem hiding this comment.
Assuming my understanding of the deletions of blocks.PreciseTime() and GasTime() are correct then I don't need to re-review how you address my comments.
There was a problem hiding this comment.
Confirming my understanding: PreciseTime() is replaced by hook.Points.BlockTime() and GasTime() is just proxytime.Of(hook.Points.BlockTime())?
There was a problem hiding this comment.
PreciseTime is replaced by the hook 👍.
GasTime was never used.
6227847 to
9cb28cc
Compare
Why this should be merged
It is somewhat nonsensical that gastime has to import hooks. @StephenButtolph may make a conflicting PR, but either is acceptable to me as long as I can import gastime in the hooks package.
How this works
moves any hook import out of the gastime package. The hooks aren't strictly needed by the ACP176 stuff, and there's two gaspriceconfigs for scoping reasons (but that seems insufficient for deduplication).
How this was tested
CI, plus now we import gastime in hooks (which was the goal)
Need to be documented in RELEASES.md?
No