chore(core/types): remove Blocks's Timestamp() method
#1429
Conversation
qdm12
force-pushed
the
qdm12/block-remove-timestamp
branch
2 times, most recently
from
1926d99 to
1bc820a
Compare
…Number` + `blockTime`
…ber` + `blockTime`
…blockNumber` + `blockTime`
qdm12
force-pushed
the
qdm12/block-remove-timestamp
branch
from
1bc820a to
2cf8ef3
Compare
| @@ -43,10 +44,10 @@ func (*configurator) MakeConfig() precompileconfig.Config { | |||
|
|
|||
| // Configure configures [state] with the given [cfg] precompileconfig. | |||
| // This function is called by the EVM once per precompile contract activation. | |||
| func (c *configurator) Configure(chainConfig precompileconfig.ChainConfig, cfg precompileconfig.Config, state contract.StateDB, blockContext contract.ConfigurationBlockContext) error { | |||
| func (c *configurator) Configure(chainConfig precompileconfig.ChainConfig, cfg precompileconfig.Config, state contract.StateDB, blockNumber *big.Int) error { | |||
There was a problem hiding this comment.
Could we keep both the block number and time here even if the time is unused? As we previously exposed this interface in precompile-evm and it seems it could be used in the future?
There was a problem hiding this comment.
Sure, one of the last commits removes the time argument, I wanted to see your opinion on this 😉
There was a problem hiding this comment.
Actually, similar question: what would be the cost to change the function signature in the future?
There was a problem hiding this comment.
It would require changes to all precompiles, both those that we have developed and any external code bases that may have developed precompiles for use in subnet-evm or precompile-evm.
So I think we should avoid changing it without good reason, but we can still change it to deliver improvements, features, and a better interface.
It seems the code that sets up a precompile may want to have access to both the timestamp and the number of the block where it's activated.
There was a problem hiding this comment.
Interesting, should we then not change the signature, such that the block context interface (now removed) can be later extended?
| @@ -21,8 +21,8 @@ type AllowListConfig struct { | |||
| } | |||
|
|
|||
| // Configure initializes the address space of [precompileAddr] by initializing the role of each of | |||
| // the addresses in [AllowListAdmins]. | |||
| func (c *AllowListConfig) Configure(chainConfig precompileconfig.ChainConfig, precompileAddr common.Address, state contract.StateDB, blockContext contract.ConfigurationBlockContext) error { | |||
| // the addresses in [AllowListConfig]. | |||
There was a problem hiding this comment.
Could we keep the arguments for this method for symmetry with the other methods (even though unused)?
There was a problem hiding this comment.
I'm not sure I understand what symmetry you are thinking about? With the Configurator.Configure??
There was a problem hiding this comment.
That was my thought, even though the AllowList is not a precompile it follows through the phases of Configure along with the precompile.
There was a problem hiding this comment.
Related question: what would be the cost of changing the signature of AllowListConfig.Configure if needed in the future? 🤔
There was a problem hiding this comment.
Less than the other method, as the AllowList is not part of the precompile interface but more a partial implementation that's used by some of the precompiles.
As I mentioned I have a subjective preference for methods that do similar things to all take the same arguments but this is not a strong preference.
qdm12
force-pushed
the
qdm12/block-remove-timestamp
branch
from
2cf8ef3 to
a9b334c
Compare
qdm12
force-pushed
the
qdm12/block-remove-timestamp
branch
from
29c0766 to
d590a04
Compare
| @@ -59,7 +60,7 @@ func (*configurator) MakeConfig() precompileconfig.Config { | |||
| // This function is called by the EVM once per precompile contract activation. | |||
| // You can use this function to set up your precompile contract's initial state, | |||
| // by using the [cfg] config and [state] stateDB. | |||
| func (*configurator) Configure(chainConfig precompileconfig.ChainConfig, cfg precompileconfig.Config, state contract.StateDB, blockContext contract.ConfigurationBlockContext) error { | |||
| func (*configurator) Configure(chainConfig precompileconfig.ChainConfig, cfg precompileconfig.Config, state contract.StateDB, blockNumber *big.Int, blockTime uint64) error { | |||
There was a problem hiding this comment.
this is a breaking change to all precompiles (external and internal precompiles). While having a simple params is better, we should also evaluate this with the future extension possibilities. IMO having a blockContext interface is less cumbersome to extend.
Why this should be merged
See ava-labs/coreth#749
How this works
See commit messages, but TLDR:
blockContext->blockNumber+blockTimePrecompileBlockContextimplementing a now changed BlockContext interface (Timestamp -> Time)How this was tested
Existing CI passing
Need to be documented?
Maybe? The template changed 🤔
Need to update RELEASES.md?