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

feat: add timestampMs for transactions #93

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from
Draft

Conversation

yoxira
Copy link
Member

@yoxira yoxira commented Dec 17, 2024

@yoxira yoxira added the feature label Dec 18, 2024
@yoxira yoxira self-assigned this Dec 18, 2024
@@ -476,6 +479,10 @@ Transaction.prototype.verify = function (trs, sender, requester, cb) {
return setImmediate(cb, 'Missing sender');
}

if (typeof trs.timestampMs !== 'number') {
Copy link
Member

Choose a reason for hiding this comment

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

Will previous txs/blocks be valid?
It's important to keep backward compatibility.

@@ -647,10 +654,26 @@ Transaction.prototype.verify = function (trs, sender, requester, cb) {
if (trs.timestamp < INT_32_MIN || trs.timestamp > INT_32_MAX) {
return setImmediate(cb, 'Invalid transaction timestamp. Timestamp is not in the int32 range');
}
if (slots.getSlotNumber(trs.timestamp) > slots.getSlotNumber()) {

if (Math.abs(trs.timestampMs - trs.timestamp * 1000) >= 1000) {
Copy link
Member

Choose a reason for hiding this comment

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

Will previous txs/blocks be valid?

return setImmediate(cb, 'Invalid transaction timestamp. Timestamp and timestampMs delta is greater than 1000ms');
}

const currentTime = slots.getTime();
Copy link
Member

Choose a reason for hiding this comment

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

Returns time in seconds, not in ms, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes

@adamant-al
Copy link
Member

adamant-al commented Dec 18, 2024

Ensure that

A node will include timestampMs in responses, including socket.

Note that tx a timestamp in the past may be still included in the current block. E.g., tx 15:30:00 may be included in a block timed 13:30:10. It’s OK.

When a client posts a new tx, timestampMs is preferred, but optional.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants