Skip to content

New timestamp field in all structures#345

Merged
sstanculeanu merged 11 commits intorc/barnardfrom
timestamp-ms-all-structuress
Jun 18, 2025
Merged

New timestamp field in all structures#345
sstanculeanu merged 11 commits intorc/barnardfrom
timestamp-ms-all-structuress

Conversation

@miiu96
Copy link
Copy Markdown
Contributor

@miiu96 miiu96 commented Jun 6, 2025

  • Added a new field timestampMs in all structures

@ssd04 ssd04 self-requested a review June 11, 2025 11:23
Size: int64(blockSizeInBytes),
SizeTxs: int64(sizeTxs),
Timestamp: time.Duration(obh.Header.GetTimeStamp()),
TimestampMs: time.Duration(obh.OutportBlock.BlockData.TimestampMs),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
TimestampMs: time.Duration(obh.OutportBlock.BlockData.TimestampMs),
TimestampMs: time.Duration(obh.OutportBlock.BlockData.GetTimestampMs()),

?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

SerializeSCRs(scrs []*data.ScResult, buffSlice *data.BufferSlice, index string, shardID uint32) error
}

// TemplatesAndPoliciesHandler defines the actions that a templates and policies handler should do
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// TemplatesAndPoliciesHandler defines the actions that a templates and policies handler should do
// TemplatesAndPoliciesHandler defines the actions that a templates and policies handler should do

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

"properties": Object{
"timestampMs": Object{
"type": "date",
"format": "epoch_millis",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

what epoch_millis means here? could have been header_millis?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

epoch_millis in Elasticsearch means:
The date is stored and interpreted as the number of milliseconds since the Unix epoch (i.e., since 1970-01-01T00:00:00Z). (it's an Elasticsearch format for date )

Comment on lines +12 to +13

var TokensTimestampMs = Object{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add comments to the other fields in this file

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added


// PrepareRegularAccountsMap will prepare a map of regular accounts
func (ap *accountsProcessor) PrepareRegularAccountsMap(timestamp uint64, accounts []*data.Account, shardID uint32) map[string]*data.AccountInfo {
func (ap *accountsProcessor) PrepareRegularAccountsMap(timestamp uint64, accounts []*data.Account, shardID uint32, timestampMs uint64) map[string]*data.AccountInfo {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i'm thinking if it might work to pass only timestampMs and get old timestamp as timestampMs/1000 when needed, since we care about ms granularity now and we can get seconds granularity from milliseconds

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

in order to avoid passing kind of duplicated variables

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and this can be applied to all other places if suitable

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

refactored

Comment on lines +50 to +51
// GetTimestampMsMappings will return the timestampMs field mappings for all indices
func (tr *templatesAndPolicyReader) GetTimestampMsMappings() ([]templates.ExtraMapping, error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why is this mapping needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this function is needed to add the new timestampMS mappings for already existing indices.

Index: indexer.EventsIndex,
Mappings: indices.TimestampMs.ToBuffer(),
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change

delete empty line

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Copy Markdown
Contributor

@sstanculeanu sstanculeanu left a comment

Choose a reason for hiding this comment

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

no other comment

@andreibancioiu andreibancioiu requested a review from Copilot June 13, 2025 08:04
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new timestampMs field (milliseconds) alongside existing timestamp fields across all data structures and updates related tests and fixtures to use these values.

  • Added timestampMs to JSON test fixtures and data models, replacing time.Duration with uint64 for timestamp
  • Updated integration tests to pass millisecond timestamps to new methods
  • Bumped mx-chain-core-go dependency version

Reviewed Changes

Copilot reviewed 160 out of 160 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
integrationtests/testdata/accountsBalanceWithLowerTimestamp/account-balance-esdt-second-update.json Add timestampMs to fixture
integrationtests/testdata/accountsBalanceWithLowerTimestamp/account-balance-esdt-first-update.json Add timestampMs to fixture
integrationtests/testdata/accountsBalanceNftTransfer/balance-nft-after-transfer.json Add timestampMs to fixture
integrationtests/testdata/accountsBalanceNftTransfer/balance-nft-after-create.json Add timestampMs to fixture
integrationtests/miniblocks_test.go Pass millisecond timestamp to SaveMiniblocks
integrationtests/logsCrossShard_test.go Pass millisecond timestamp to RemoveTransactions
integrationtests/delegators_test.go Pass millisecond timestamp to RemoveTransactions
integrationtests/accountsESDTRollback_test.go Update RemoveAccountsESDT call with ms timestamp
integrationtests/accountsBalanceNftTransfer_test.go Set TimestampMs in block creation helper
go.mod Bump mx-chain-core-go version
data/transaction.go Change Timestamp type; add TimestampMs
data/tokens.go Change Timestamp type; add TimestampMs
data/scresult.go Change Timestamp type; add TimestampMs
data/scDeploy.go Add TimestampMs to ScDeployInfo and Upgrade
data/logs.go Change Timestamp type; add TimestampMs
data/event.go Change Timestamp type; add TimestampMs
data/delegators.go Change Timestamp type; add TimestampMs
data/data.go Change Timestamp type; add TimestampMs
data/block.go Change Timestamp type; add TimestampMs
data/account.go Change Timestamp type; add TimestampMs
Comments suppressed due to low confidence (1)

integrationtests/accountsESDTRollback_test.go:94

  • It looks like the arguments to RemoveAccountsESDT have been swapped compared to the original call (timestamp, shardID). Please confirm the new signature and correct the argument order.
err = esProc.RemoveAccountsESDT(2, 5040000)

TxHash string `json:"upgradeTxHash"`
Upgrader string `json:"upgrader"`
Timestamp uint64 `json:"timestamp"`
TimestampMs uint64 `json:"timestampMs"`
Copy link

Copilot AI Jun 13, 2025

Choose a reason for hiding this comment

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

[nitpick] The TimestampMs field in Upgrade lacks the omitempty JSON tag, which causes zero values to always be serialized. Consider adding omitempty for consistency with other timestamp fields.

Suggested change
TimestampMs uint64 `json:"timestampMs"`
TimestampMs uint64 `json:"timestampMs,omitempty"`

Copilot uses AI. Check for mistakes.
},
}
err = esProc.SaveMiniblocks(header, miniBlocks)
err = esProc.SaveMiniblocks(header, miniBlocks, 1234000)
Copy link

Copilot AI Jun 13, 2025

Choose a reason for hiding this comment

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

[nitpick] This test uses a hardcoded millisecond value (1234000). To prevent mismatches if header.TimeStamp changes, consider computing it with header.TimeStamp * 1000.

Suggested change
err = esProc.SaveMiniblocks(header, miniBlocks, 1234000)
err = esProc.SaveMiniblocks(header, miniBlocks, header.TimeStamp*1000)

Copilot uses AI. Check for mistakes.
@sstanculeanu sstanculeanu merged commit 50fb80e into rc/barnard Jun 18, 2025
8 checks passed
@sstanculeanu sstanculeanu deleted the timestamp-ms-all-structuress branch June 18, 2025 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants