Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(core/types): remove Blocks's Timestamp() method #749

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

qdm12
Copy link
Collaborator

@qdm12 qdm12 commented Jan 16, 2025

Why this should be merged

In core/types, the Block method Timestamp does the same as the method Time and is not present upstream (this causes small difficulties in #750)

How this works

  • core/types.Block method Timestamp() removed
  • core/vm new type PrecompileBlockContext wrapping BlockContext with custom methods, in order to have a Time() method not conflicting the Time field name.
  • The following functions (all non-geth code) were changed to accept a block number + block time instead of the block interface contract.ConfigurationBlockContext:
    • core.ApplyUpgrades
    • core.ApplyPrecompileActivations
    • precompile/modules.Configure
  • precompile/contract.ConfigurationBlockContext interface removed and inlined in BlockContext

How this was tested

Existing CI passing

Need to be documented?

No

Need to update RELEASES.md?

No

@qdm12 qdm12 force-pushed the qdm12/block-remove-timestamp branch from 7f4bf55 to ec26332 Compare January 16, 2025 16:03
@qdm12
Copy link
Collaborator Author

qdm12 commented Jan 17, 2025

@darioush following your PR, I did another commit to try reducing diffs in cce6eab - what do you think? The only issue with re-using an interface was with

blockContext := timestampToTime{BlockGen: b}

Requiring a little wrapper for *BlockGen to convert Timestamp -> Time

Or should we stick to the 'new' simple types and no blockContext interface? I would lean towards this since it's our own code, but thought about preserving it too.

@qdm12 qdm12 changed the title chore(all): remove Timestamp() method on Block types chore(all): remove Timestamp() method on core/types.Block Jan 17, 2025
@qdm12 qdm12 changed the title chore(all): remove Timestamp() method on core/types.Block chore(core/types): remove Blocks's Timestamp() method Jan 17, 2025
@qdm12 qdm12 requested a review from darioush January 21, 2025 10:14
@qdm12 qdm12 force-pushed the qdm12/block-remove-timestamp branch from 37cbc5c to 5d7b0c4 Compare January 21, 2025 10:14
@qdm12 qdm12 marked this pull request as ready for review January 21, 2025 10:15
@qdm12 qdm12 requested review from ceyonur and a team as code owners January 21, 2025 10:15
ceyonur
ceyonur previously approved these changes Jan 21, 2025
darioush
darioush previously approved these changes Jan 21, 2025
core/genesis.go Outdated Show resolved Hide resolved
@qdm12 qdm12 dismissed stale reviews from darioush and ceyonur via 19f0c6b January 21, 2025 15:59
@qdm12 qdm12 force-pushed the qdm12/block-remove-timestamp branch from 5d7b0c4 to 19f0c6b Compare January 21, 2025 15:59
darioush
darioush previously approved these changes Jan 21, 2025
@@ -73,6 +68,7 @@ type Configurator interface {
chainConfig precompileconfig.ChainConfig,
precompileconfig precompileconfig.Config,
state StateDB,
blockContext ConfigurationBlockContext,
blockNumber *big.Int,
blockTime uint64,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this blockTime at the end? It doesn't seem used anywhere in subnet-evm. Waiting for a reply on ava-labs/subnet-evm#1429

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep them for symmetry, previously exposed interfaces, and possible future use, imo.
(same comments here ava-labs/subnet-evm#1429 (comment))

qdm12 and others added 6 commits January 22, 2025 16:59
- `Time()` does the same as `Timestamp()`
- `Time()` is present upstream, not `Timestamp()`
- modify our own custom code: `ApplyPrecompileActivations`, `ApplyUpgrades` to accept a block number + block time and push responsibility of the caller to get the block number and time and inject them
@qdm12 qdm12 force-pushed the qdm12/block-remove-timestamp branch from 940bb2a to 85ec7d0 Compare January 23, 2025 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants