Warp gas costs granite #1306
Warp gas costs granite #1306
Conversation
…coreth into gstuart/warp-gas-costs-granite
geoff-vball
requested review from
iansuvak,
StephenButtolph and
michaelkaplan13
| // IsGranite returns true if the time is after Granite. | ||
| IsGranite(time uint64) bool |
There was a problem hiding this comment.
Do we ever use this?
There was a problem hiding this comment.
There was a problem hiding this comment.
I think we can return the rules with the GetRules from env.rules to consolidate these two.
more specifically the interface here and the implemtnation
but it probably requires a bigger refactor that changes other files
| } | ||
|
|
||
| func GetWarpPredicateGasConfig(isGraniteActivated bool) GasConfig { | ||
| if isGraniteActivated { |
There was a problem hiding this comment.
did we add any test for this case?
| return GasConfig{ | ||
| PerWarpSigner: 250, | ||
| PerWarpMessageChunk: 3_200, | ||
| PerSignatureVerification: 100_000, | ||
| } |
There was a problem hiding this comment.
nit: we can just define them as constants just for a better visibility
| return statefulContract | ||
| } | ||
|
|
||
| func GetWarpPredicateGasConfig(isGraniteActivated bool) GasConfig { |
There was a problem hiding this comment.
| func GetWarpPredicateGasConfig(isGraniteActivated bool) GasConfig { | |
| func CurrentGasConfig(rules precompileconfig.Rules) GasConfig { |
Rationale re readability / maintainability:
- The "get" prefix doesn't communicate anything of value; "current" tells the reader that the function changes what it returns over time.
- We're already in the
warppackage sowarp.GetWarp...is jarring when called from outside, and unnecessary when called from inside the package. - "Predicate" could possibly remain, but is unnecessary and may not always be where this is used (e.g. under SAE the gas charge is the same but it's done in a different place).
- Raw booleans lend themselves to hard-to-read call sites, often requiring comments like
config := CurrentGasConfig(false /*isGranite*/)(e.g. your first change toTestGetVerifiedWarpMessageis opaque). Across geth/libevm,Rulesare the point-in-time carriers of these booleans, so accepting them as the argument makes code self-explanatory and less brittle if changed (e.g. in a future fork where there would be two booleans).
Co-authored-by: Michael Kaplan <[email protected]> Signed-off-by: Geoff Stuart <[email protected]>
| } | ||
|
|
||
| func (a accessibleState) GetRules() precompileconfig.Rules { | ||
| extra := GetExtra(a.GetPrecompileEnv().ChainConfig()) |
There was a problem hiding this comment.
nit: can we rename this to chainConfigExtra as there are actual extras package which can be confusing
| return GetExtra(a.env.ChainConfig()) | ||
| } | ||
|
|
||
| func (a accessibleState) GetRules() precompileconfig.Rules { |
There was a problem hiding this comment.
Thanks for refactoring this.
| IsGranite bool | ||
| } | ||
|
|
||
| func (a AvalancheRules) IsGraniteActivated() bool { |
There was a problem hiding this comment.
can we add a comment why this is required (to make it clear this is called from precompile environment etc)
Why this should be merged
Updates warp gas fees for granite upgrade. (Final numbers still TBD).
How this works
How this was tested
Unit tests in
precompile/procompiletestthat use granite-specific gas costs also run for granite now.Need to be documented?
Need to update RELEASES.md?