Conversation
cmwhited
left a comment
cmwhited
left a comment
There was a problem hiding this comment.
Pretty mega code change. I wouldn't say "no" to more tests. But LGTM. Didn't say anything that I thought needed changes. Good work, as always
graph-gateway/src/topology.rs
Outdated
| // https://github.com/graphprotocol/graph-node/blob/master/docs/subgraph-manifest.md | ||
| #[derive(Deserialize)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct ManifestSrc { |
There was a problem hiding this comment.
rust question here. does marking these as pub mean they are available outside of this module (file)? Or not because they are defined within the scope of this cat_manifest fn? Also, what is the reason for putting them in here, other than constraining them to this scope?
There was a problem hiding this comment.
Good question. As far as I know, putting pub on these structs & fields is completely nonsensical. And my reason for putting them in function scope is just because they're only used in this function, and just describe how to deserialize the response body.
This is an important step for facilitating many upcoming features. Changes include: - Ensuring that all manifests get at least one chance to cat from IPFS before accepting client queries. This should reduce the occasional hiccups on gateway restarts. - Simplify the data pipeline between the network subgraph & other sources into the gateway's view of the Graph network. - Only load network parameters once on startup, to reduce unnecessary network subgraph queries. - Allow the ISA to select over potentially multiple deployments (not used yet).
2 participants
This is an important step for facilitating many upcoming features. Changes include:
Important files to review:
topology.rs&network_subgraph.rs