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

blockchain: Introduce BlockchainMap and BlockchainKind #2755

Merged
merged 1 commit into from
Aug 31, 2021

Conversation

leoyvens
Copy link
Collaborator

This introduces the BlockchainMap in which Blockchain instances are stored, keyed by their BlockchainKind and network. This is used in the entry points that need to call functions which are generic on C: Blockchain, which are: InstanceManager::start_subgraph, SubgraphRegistrar::create_subgraph and IndexNodeResolver::resolve_subgraph_features.

Part of #2732.

@leoyvens leoyvens force-pushed the leo/blockchain-map branch from 968193b to 7c944d3 Compare August 30, 2021 16:05
.ok_or(SubgraphRegistrarError::NetworkNotSupported(
network_name.clone(),
))?
.cheap_clone();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This block of code was moved to the create_subgraph_version free function.

let validation_warnings = import_errors
.into_iter()
.map(SubgraphManifestValidationWarning::SchemaValidationWarning)
.collect();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These import_errors are not used anyways, so they are now ignored.

@@ -921,40 +917,12 @@ impl<C: Blockchain> UnvalidatedSubgraphManifest<C> {

impl<C: Blockchain> SubgraphManifest<C> {
/// Entry point for resolving a subgraph definition.
/// Right now the only supported links are of the form:
/// `/ipfs/QmUmg7BZC1YP1ca66rRtWKxpXp77WgVHrnv263JtDuvs2k`
pub async fn resolve(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed because resolve_from_raw is now used in all cases.

// `blockchain_map`.
let mut blockchain_map = BlockchainMap::new();
let ethereum_chains = networks_as_chains(
&mut blockchain_map,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This hack is because I preferred to not refactor the block ingestor to use BlockchainMap since it is unlikely to be used for new chains, so ethereum_chains continues to exists.

@@ -1024,6 +992,7 @@ impl<C: Blockchain> UnresolvedSubgraphManifest<C> {
self,
resolver: &impl LinkResolver,
logger: &Logger,
max_spec_version: semver::Version,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adding this is actually unrelated to this PR, it's just improving the tests to no need the GRAPH_MAX_SPEC_VERSION env var set which was introduced in #2682.

@tilacog tilacog self-requested a review August 31, 2021 00:14
Comment on lines +316 to +339
pub enum BlockchainKind {
/// Ethereum itself or chains that are compatible.
Ethereum,
}

impl fmt::Display for BlockchainKind {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
let value = match self {
BlockchainKind::Ethereum => "ethereum",
};
write!(f, "{}", value)
}
}

impl FromStr for BlockchainKind {
type Err = Error;

fn from_str(s: &str) -> Result<Self, Self::Err> {
match s {
"ethereum" => Ok(BlockchainKind::Ethereum),
_ => Err(anyhow!("unknown blockchain kind {}", s)),
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you feel about delegating this to serde::plain, just like we did for subgraph features?

While the current implementation works flawlessly, if we use serde here we wouldn't need to write any more code when introducing new variants.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't have much of a preference so I'll keep it as it is.

Comment on lines +399 to +401
(None, "subgraphFeatures") => {
graph::block_on(self.resolve_subgraph_features(arguments))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Way better! 👏

@leoyvens leoyvens merged commit 7f1bcbf into master Aug 31, 2021
@leoyvens leoyvens deleted the leo/blockchain-map branch August 31, 2021 15:29
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