-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add timestamp to meta block #3738
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -361,8 +361,14 @@ pub trait ChainStore: Send + Sync + 'static { | |
/// may purge any other blocks with that number | ||
fn confirm_block_hash(&self, number: BlockNumber, hash: &BlockHash) -> Result<usize, Error>; | ||
|
||
/// Find the block with `block_hash` and return the network name and number | ||
fn block_number(&self, hash: &BlockHash) -> Result<Option<(String, BlockNumber)>, StoreError>; | ||
/// Find the block with `block_hash` and return the network name, number and timestamp if present. | ||
/// Currently, the timestamp is only returned if it's present in the top level block. This format is | ||
/// depends on the chain and the implementation of Blockchain::Block for the specific chain. | ||
/// eg: {"block": { "timestamp": 123123123 } } | ||
fn block_number( | ||
&self, | ||
hash: &BlockHash, | ||
) -> Result<Option<(String, BlockNumber, Option<String>)>, StoreError>; | ||
|
||
/// Tries to retrieve all transactions receipts for a given block. | ||
async fn transaction_receipts_in_block( | ||
|
@@ -409,6 +415,14 @@ pub trait QueryStore: Send + Sync { | |
|
||
fn block_number(&self, block_hash: &BlockHash) -> Result<Option<BlockNumber>, StoreError>; | ||
|
||
/// Returns the blocknumber as well as the timestamp. Timestamp depends on the chain block type | ||
/// and can have multiple formats, it can also not be prevent. For now this is only available | ||
/// for EVM chains both firehose and rpc. | ||
fn block_number_with_timestamp( | ||
&self, | ||
block_hash: &BlockHash, | ||
) -> Result<Option<(BlockNumber, Option<String>)>, StoreError>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's an package internal name, I went with I thought was an accurate description 😛 happy to rename if you have suggestions |
||
|
||
fn wait_stats(&self) -> Result<PoolWaitStats, StoreError>; | ||
|
||
async fn has_deterministic_errors(&self, block: BlockNumber) -> Result<bool, StoreError>; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -589,34 +589,42 @@ mod data { | |
} | ||
} | ||
|
||
/// timestamp's representation depends the blockchain::Block implementation, on | ||
/// ethereum this is a U256 but on different chains it will most likely be different. | ||
pub(super) fn block_number( | ||
&self, | ||
conn: &PgConnection, | ||
hash: &BlockHash, | ||
) -> Result<Option<BlockNumber>, StoreError> { | ||
) -> Result<Option<(BlockNumber, Option<String>)>, StoreError> { | ||
const TIMESTAMP_QUERY: &str = | ||
"coalesce(data->'block'->>'timestamp', data->>'timestamp')"; | ||
|
||
let number = match self { | ||
Storage::Shared => { | ||
use public::ethereum_blocks as b; | ||
|
||
b::table | ||
.select(b::number) | ||
.select((b::number, sql(TIMESTAMP_QUERY))) | ||
.filter(b::hash.eq(format!("{:x}", hash))) | ||
.first::<i64>(conn) | ||
.first::<(i64, Option<String>)>(conn) | ||
.optional()? | ||
} | ||
Storage::Private(Schema { blocks, .. }) => blocks | ||
.table() | ||
.select(blocks.number()) | ||
.select((blocks.number(), sql(TIMESTAMP_QUERY))) | ||
.filter(blocks.hash().eq(hash.as_slice())) | ||
.first::<i64>(conn) | ||
.first::<(i64, Option<String>)>(conn) | ||
.optional()?, | ||
}; | ||
number | ||
.map(|number| { | ||
BlockNumber::try_from(number) | ||
.map_err(|e| StoreError::QueryExecutionError(e.to_string())) | ||
}) | ||
.transpose() | ||
|
||
match number { | ||
None => Ok(None), | ||
Some((number, ts)) => { | ||
let number = BlockNumber::try_from(number) | ||
.map_err(|e| StoreError::QueryExecutionError(e.to_string()))?; | ||
Ok(Some((number, ts))) | ||
} | ||
} | ||
} | ||
|
||
/// Find the first block that is missing from the database needed to | ||
|
@@ -1690,12 +1698,15 @@ impl ChainStoreTrait for ChainStore { | |
.confirm_block_hash(&conn, &self.chain, number, hash) | ||
} | ||
|
||
fn block_number(&self, hash: &BlockHash) -> Result<Option<(String, BlockNumber)>, StoreError> { | ||
fn block_number( | ||
&self, | ||
hash: &BlockHash, | ||
) -> Result<Option<(String, BlockNumber, Option<String>)>, StoreError> { | ||
let conn = self.get_conn()?; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would ideally be asyncified to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this one is also used in a few places so I'd rather take that as a separate PR |
||
Ok(self | ||
.storage | ||
.block_number(&conn, hash)? | ||
.map(|number| (self.chain.clone(), number))) | ||
.map(|(number, timestamp)| (self.chain.clone(), number, timestamp))) | ||
} | ||
|
||
async fn transaction_receipts_in_block( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tuple is getting too complex, seems worth introducing a struct. Also there might be a better name than
block_number
since this also returns other info.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this refactoring should go on the same PR though. I'm happy to make the changes and raise a new PR for it