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

Subgraph Composition: Use Firehose endpoint to load blocks for subgraph triggers #5706

Open
wants to merge 4 commits into
base: krishna/switch-to-using-only-block-ptrs
Choose a base branch
from

Conversation

incrypto32
Copy link
Member

No description provided.

@incrypto32 incrypto32 added this to the Subgraph Composition milestone Nov 18, 2024
@incrypto32 incrypto32 force-pushed the krishna/sgc-use-firehose-get-block branch from c6a7960 to 84f4ccb Compare November 18, 2024 11:56
@incrypto32 incrypto32 force-pushed the krishna/sgc-use-firehose-get-block branch from 84f4ccb to a3891bb Compare November 19, 2024 12:01
@incrypto32 incrypto32 changed the base branch from krishna/cache-firehose-blocks to krishna/switch-to-using-only-block-ptrs November 19, 2024 12:01
/// Fetches blocks from the cache based on block numbers, excluding duplicates
/// (i.e., multiple blocks for the same number), and identifying missing blocks that
/// need to be fetched via RPC/Firehose. Returns a tuple of the found blocks and the missing block numbers.
async fn fetch_unique_blocks_from_cache(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be a good candidate to add some unit test coverage

@@ -763,23 +819,96 @@ impl TriggersAdapterTrait<Chain> for TriggersAdapter {
logger: Logger,
block_numbers: HashSet<BlockNumber>,
) -> Result<Vec<BlockFinality>> {
use graph::futures01::stream::Stream;
// Common function to handle block loading, regardless of source
async fn load_blocks<F, Fut>(
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason this couldn't be a function on triggers adapter instead of inside the function? This is more of a nitpick but this usually means it's code that cannot be tested and personally I think it makes code less readable. I understand the code locality here and I think it does help understand the full context (in this case). Just wary of doing this sort of thing as a norm

.ok_or(anyhow!("unable to get adapter for is_on_main_chain"))?
.is_on_main_chain(&self.logger, ptr.clone())
.await
match &*self.chain_client {
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering if this should be on the ChainClient, since it's implemented already for rpc adapters, perhaps it would make sense to get the block from the adapter (whichever) and then have the logic on the layer above that makes the decision on whether it is on the main chain.

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