Skip to content

Transform results of getBlockByNumber#59

Merged
masonforest merged 38 commits intomasterfrom
YAS-247/TheGraph/ParseGetBlockResult
Mar 30, 2020
Merged

Transform results of getBlockByNumber#59
masonforest merged 38 commits intomasterfrom
YAS-247/TheGraph/ParseGetBlockResult

Conversation

@masonforest
Copy link
Copy Markdown

Description

getBlockByNumber now returns timestamps and transaction hashes from the perspective of the full node instead of the perspective of our backend node.

Other Changes

  • Moved getTimestamp into utils so it can be used in a static method (create) without having to instantiate the class it was in.

Metadata

Fixes

Contributing Agreement

@masonforest masonforest force-pushed the YAS-247/TheGraph/ParseGetBlockResult branch from 69430f3 to a89f959 Compare March 27, 2020 17:01
Copy link
Copy Markdown
Contributor

@K-Ho K-Ho left a comment

Choose a reason for hiding this comment

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

The main fix required is that the only place in which the timestampIncreaseSeconds is used is when callingevm_getTime , which I also don't think is a method that ever gets used.
We need to add timestampIncreaseSeconds to the current time every time we set a transaction's timestamp and store a block's timestamp.

*/
protected getTimestamp(): number {
return super.getTimestamp() + this.timestampIncreaseSeconds
protected async getTimestamp(): Promise<number> {
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.

This should instead return utils.getTimestamp + this.timestampIncreaseSeconds and should be used to set the timestamp within sendRawTransaction

)

const block = await httpProvider.getBlock('latest', true)
block.timestamp.should.be.gt(timestamp)
Copy link
Copy Markdown
Contributor

@K-Ho K-Ho Mar 30, 2020

Choose a reason for hiding this comment

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

one note is that if we do end up speeding up transactions enough, then we could end up with block.timestamp equaling timestamp in this test

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Ah good call! I changed it to greater than or equal to just incase.

// Construct the raw transaction calldata
const internalCalldata = this.getTransactionCalldata(
this.getTimestamp(),
0,
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 does this need to be 0?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ah good catch! It doesn't 👍 -> Reverted

Copy link
Copy Markdown
Contributor

@K-Ho K-Ho left a comment

Choose a reason for hiding this comment

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

Looks good! just a couple tiny notes

@masonforest masonforest merged commit 42f92d9 into master Mar 30, 2020
@masonforest masonforest deleted the YAS-247/TheGraph/ParseGetBlockResult branch April 7, 2020 11:54
snario pushed a commit that referenced this pull request Apr 14, 2021
* Fix L2XDomainMessenger addr in deploy

* xdomain: generalize config, add deploy overrides

Co-authored-by: Karl Floersch <karl@karlfloersch.com>
blockchaindevsh added a commit to blockchaindevsh/optimism that referenced this pull request Sep 25, 2024
…oxContract

remove UseInboxContract and optimize getTxSucceed
theochap pushed a commit that referenced this pull request Jan 15, 2026
### Description

Simplifies signature definitions.
Closes #59.
theochap pushed a commit that referenced this pull request Jan 21, 2026
### Description

Simplifies signature definitions.
Closes #59.
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.

2 participants