refactor: remove Option<BlockHeight> and use new enum where applicable
#2033
Conversation
crates/fuel-core/src/schema/tx.rs
Outdated
| @@ -132,7 +132,7 @@ impl TxQuery { | |||
| |start: &Option<SortedTxCursor>, direction| { | |||
| let start = *start; | |||
| let block_id = start.map(|sorted| sorted.block_height); | |||
| let all_block_ids = query.compressed_blocks(block_id, direction); | |||
| let all_block_ids = query.compressed_blocks(block_id.into(), direction); | |||
There was a problem hiding this comment.
Why is this block_id? Obviously this pre-existed your PR :)
There was a problem hiding this comment.
it's the cursor to a paginated query
| } | ||
| (true, IterDirection::Forward) => self | ||
| .on_chain | ||
| .blocks(BlockHeightQuery::Specific(height), direction), |
There was a problem hiding this comment.
This seems like a smell. We are matching on the BlockHeightQuery then we reconstruct the BlockHeightQuery. I think blocks and old_blocks can probably just take Option<BlockHeight> and we can get rid of the From impl into Option./F
There was a problem hiding this comment.
The only issue with this is that blocks is a port so it can either take Option<BlockHeight> or BlockHeightQuery so then this blocks function in database.rs would have to take a Option<BlockHeight>. What's the difference between reconstructing BlockHeightQuery and reconstructing the Option like the previous implementation?
There was a problem hiding this comment.
What's the difference between reconstructing BlockHeightQuery and reconstructing the Option like the previous implementation?
There's no difference. I think it was a smell in the old code too.
How about
if let BlockHeightQuery::Specific(inner) = height {
match (inner >= self.genesis_height, direction) {
(true, IterDirection::Forward) => self
.on_chain
.blocks(height, direction),
It would be nice if we had some type guarantee since the inner >= self.genesis_height is kinda redundant.
| @@ -508,7 +508,7 @@ where | |||
| off_chain_height.map(|height| BlockHeight::new(height.saturating_add(1))); | |||
|
|
|||
| let import_result = | |||
| import_result_provider.block_event_at_height(next_block_height)?; | |||
| import_result_provider.block_event_at_height(next_block_height.into())?; | |||
There was a problem hiding this comment.
Is it possible to make on_chain_database.latest_height() return BlockHeightQuery?
This makes me think that the name should be something different.
There was a problem hiding this comment.
We could change latest_height() to return BlockHeightQuery. I didn't do this because, IIUC, a database should be able to return None if it can't find the requested key. Is the genesis block stored in the DB? I agree the name BlockHeightQuery isn't quite right. The only other name I can think of is SpecificBlockOrGenesis
| @@ -292,7 +295,7 @@ impl BlockQuery { | |||
| crate::schema::query_pagination(after, before, first, last, |start, direction| { | |||
| Ok(blocks_query( | |||
| query.as_ref(), | |||
| start.map(Into::into), | |||
| start.map(Into::<BlockHeight>::into).into(), | |||
There was a problem hiding this comment.
It doesn't look like we can avoid the Option here, so this is where we are giving the business-level context to a None I guess.
|
@matt-user / @MitchTurner can I just regen the state transition bytecode, run tests again and then merge? |
|
Ah dang. Sorry @matt-user I thought this got merged :o |
|
Hello @matt-user , Sorry for letting your PR became stale like this. Are you ok to resolve the conflicts so that we can gett this merge soonish ? If you prefer we can take care of the conflicts for you, let me know :) |
Yeah sorry, I completely forgot about this. I will resolve the conflicts |
closes #2005
I replaced
Option<BlockHeight>withBlockHeightQuerywhere applicable. IIUC there areOption<BlockHeight>which should remain.Checklist
Before requesting review
After merging, notify other teams
[Add or remove entries as needed]