Make subgraph URL's configurable #2126

narcis96 · 2023-12-06T08:01:54Z

Description

This PR aims to fix 2105

Changes

The subgraph base url is read from the environment variables.

How to test

All unit tests still pass

github-actions · 2023-12-06T08:02:08Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

narcis96 · 2023-12-06T08:43:42Z

I have read the CLA Document and I hereby sign the CLA

fleupold

This parameter should become part of the shared arguments we use to configure all our services. Cf. #1911 for a PR that achieves something very similar.

fleupold · 2023-12-06T09:09:17Z

crates/shared/src/subgraph.rs

+        let graph_api_base =
+            std::env::var("GRAPH_API_BASE_URL").unwrap_or(DEFAULT_GRAPH_API_BASE_URL.clone());


Reading global state such as env variables in core domain components is bad practice (it makes it hard to see holistically which arguments are required for the system as a whole).
Instead we use DI to pass in those configuration values from above and configure them in one place (shared::arguments)

fleupold · 2023-12-06T13:36:07Z

crates/driver/src/boundary/liquidity/balancer/v2/mod.rs

    eth: &Ethereum,
    block_stream: CurrentBlockStream,
    block_retriever: Arc<dyn BlockRetrieving>,
    config: &infra::liquidity::config::BalancerV2,
 ) -> Box<dyn LiquidityCollecting> {
    let eth = Arc::new(eth.clone());
+    let graph_api_base_url = Arc::new(graph_api_base_url.clone());


What is the point of using an Arc here, if init_liquidity doesn't take a shared ref?

otherwise it complains that "borrowed data escapes outside of function".
EDIT : this comment is outdated now

fleupold · 2023-12-06T13:37:00Z

crates/driver/src/boundary/liquidity/balancer/v2/mod.rs

@@ -75,19 +76,22 @@ fn to_interaction(
 }

 pub fn collector(
+    graph_api_base_url: &Url,


this field should probably, like other things that configure the different liquidity sources, be part of the config struct (otherwise this method will soon have 100 arguments)

crates/shared/src/sources/balancer_v2/graph_api.rs

MartinquaXD · 2023-12-06T15:53:22Z

crates/driver/src/infra/cli.rs

+    /// The base URL to connect to subgraph clients.
+    #[clap(long, env, default_value = "https://api.thegraph.com/subgraphs/name/")]
+    pub graph_api_base_url: Url,


We should probably not continue requiring the same base url for every use case of the graph and instead pass around the complete url in the univ3 and balancer v2 liquidity section of the config.
Otherwise we are forced to completely move towards self hosting the subgraph on the same base url.

fleupold

I'm not sure @MartinquaXD suggestion of passing in the entire subgraph url (not just the base url) is practical as it would probably require some more involved "default logic" that has to be re-written in the infra repository as well (unless we make it mandatory in which case setting this up for developers to use locally may be a PITA).

If we leave this task to just supply the base URL, I think the right config this value should go on is infra::config::file::LiquidityConfig so it's only set once.

Can you please also test how a driver config with a custom subgraph URL works locally?

fleupold · 2023-12-07T09:19:04Z

crates/driver/src/infra/liquidity/config.rs

+            graph_api_base_url: Url::parse(&DEFAULT_GRAPH_API_BASE_URL)
+                .expect("invalid default Graph API base URL"),


Shouldn't the url be configurable here as well?

fleupold · 2023-12-07T09:20:56Z

crates/driver/src/infra/config/file/mod.rs

+
+        graph_api_base_url: Url,


Preset pools (which are used in prod) should also be able to configure this value

I have actually moved the url to the LiquidityConfig so I think that this comment is outdated now.

fleupold · 2023-12-08T14:52:35Z

crates/driver/src/infra/config/file/mod.rs

+
+        graph_api_base_url: Url,


Can we move the four occurrences here (crates/driver/src/infra/config/file/mod.rs) to a single field on the top level LiquidityConfig object? Since we are not going to configure different base URLs for different liquidity types, this seems the least configuration overhead.

agree that it causes some configuration overhead, so I moved it to LiquidityConfig as suggested

crates/shared/src/sources/balancer_v2/graph_api.rs

fleupold

Looking good. I think we should still only define the string "https://api.thegraph.com/subgraphs/name/" once as a constant somewhere in the codebase (and I'd prefer using a type that implements Default over re-implementing it), but otherwise this looks good to me.

fleupold · 2023-12-08T17:32:40Z

crates/driver/src/infra/config/file/mod.rs

@@ -293,6 +293,24 @@ struct LiquidityConfig {
    /// Liquidity provided by a Balancer V2 compatible contract.
    #[serde(default)]
    balancer_v2: Vec<BalancerV2Config>,
+
+    /// The base URL used to connect to subgraph clients.
+    graph_api_base_url: Url,


nit: maybe slightly nice to use Option<Url>, which implements Default and then unwrap_or with out constant when we convert it to the liquidity specific config object.

fleupold · 2023-12-08T17:32:56Z

crates/driver/src/infra/config/file/mod.rs

+
+impl Default for LiquidityConfig {
+    fn default() -> Self {
+        let default_url = Url::parse("https://api.thegraph.com/subgraphs/name/")


let's keep this a constant somewhere

MartinquaXD

Looks okay to me.
Did you run it locally as a sanity check? I think we might not have e2e tests verifying that we can query the graph liquidity.

MartinquaXD · 2023-12-11T11:05:06Z

crates/driver/src/infra/config/file/mod.rs

 }

+// impl Default for LiquidityConfig {


Left over comments.

oh yeah. removed

narcis96 · 2023-12-11T11:32:05Z

Looks okay to me. Did you run it locally as a sanity check? I think we might not have e2e tests verifying that we can query the graph liquidity.

I don't know how to run in locally. Are you referring to "Running the Services Locally" section from the README ?

narcis96 requested a review from a team as a code owner December 6, 2023 08:01

narcis96 force-pushed the configuration_subgraph branch from 92de641 to 69a2dae Compare December 6, 2023 08:47

github-actions bot added a commit that referenced this pull request Dec 6, 2023

@narcis96 has signed the CLA from Pull Request #2126

c393c4e

narcis96 force-pushed the configuration_subgraph branch from 69a2dae to e759151 Compare December 6, 2023 08:51

fleupold requested changes Dec 6, 2023

View reviewed changes

narcis96 force-pushed the configuration_subgraph branch 2 times, most recently from e86e8c5 to 10a6266 Compare December 6, 2023 13:30

fleupold reviewed Dec 6, 2023

View reviewed changes

narcis96 force-pushed the configuration_subgraph branch from 10a6266 to bfb8186 Compare December 6, 2023 13:51

MartinquaXD reviewed Dec 6, 2023

View reviewed changes

narcis96 force-pushed the configuration_subgraph branch 4 times, most recently from ff7876c to 01c2996 Compare December 6, 2023 16:43

fleupold reviewed Dec 7, 2023

View reviewed changes

narcis96 force-pushed the configuration_subgraph branch from 01c2996 to 605b8a7 Compare December 8, 2023 14:46

fleupold reviewed Dec 8, 2023

View reviewed changes

narcis96 force-pushed the configuration_subgraph branch 4 times, most recently from bf0aeb9 to 1b96d7b Compare December 8, 2023 15:37

fleupold mentioned this pull request Dec 8, 2023

Add Balancer subgraph on Sepolia #2139

Closed

fleupold approved these changes Dec 8, 2023

View reviewed changes

narcis96 force-pushed the configuration_subgraph branch 4 times, most recently from 1bb18f9 to 646dae3 Compare December 11, 2023 10:58

MartinquaXD approved these changes Dec 11, 2023

View reviewed changes

squadgazzz approved these changes Dec 11, 2023

View reviewed changes

narcis96 force-pushed the configuration_subgraph branch 2 times, most recently from 612ca44 to 58dba87 Compare December 11, 2023 11:28

narcis96 force-pushed the configuration_subgraph branch from 90ed087 to 5f27fbd Compare December 11, 2023 11:33

Make subgraph URL's configurable

6834e8b

narcis96 force-pushed the configuration_subgraph branch from fb1d437 to 6834e8b Compare December 11, 2023 16:42

narcis96 merged commit bf1d4a7 into main Dec 11, 2023
8 checks passed

narcis96 deleted the configuration_subgraph branch December 11, 2023 16:59

github-actions bot locked and limited conversation to collaborators Dec 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make subgraph URL's configurable #2126

Make subgraph URL's configurable #2126

narcis96 commented Dec 6, 2023 •

edited

Loading

github-actions bot commented Dec 6, 2023 •

edited

Loading

narcis96 commented Dec 6, 2023

fleupold left a comment

fleupold Dec 6, 2023

narcis96 Dec 6, 2023

fleupold Dec 6, 2023

narcis96 Dec 6, 2023 •

edited

Loading

fleupold Dec 6, 2023

narcis96 Dec 6, 2023

MartinquaXD Dec 6, 2023

fleupold left a comment

fleupold Dec 7, 2023

narcis96 Dec 8, 2023

fleupold Dec 7, 2023

narcis96 Dec 8, 2023 •

edited

Loading

fleupold Dec 8, 2023

narcis96 Dec 8, 2023

fleupold left a comment

fleupold Dec 8, 2023

narcis96 Dec 11, 2023

fleupold Dec 8, 2023

narcis96 Dec 11, 2023

MartinquaXD left a comment

MartinquaXD Dec 11, 2023

narcis96 Dec 11, 2023

narcis96 commented Dec 11, 2023

		let graph_api_base =
		std::env::var("GRAPH_API_BASE_URL").unwrap_or(DEFAULT_GRAPH_API_BASE_URL.clone());

		graph_api_base_url: Url::parse(&DEFAULT_GRAPH_API_BASE_URL)
		.expect("invalid default Graph API base URL"),

Make subgraph URL's configurable #2126

Make subgraph URL's configurable #2126

Conversation

narcis96 commented Dec 6, 2023 • edited Loading

Description

Changes

How to test

github-actions bot commented Dec 6, 2023 • edited Loading

narcis96 commented Dec 6, 2023

fleupold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narcis96 Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fleupold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narcis96 Dec 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fleupold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinquaXD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narcis96 commented Dec 11, 2023

narcis96 commented Dec 6, 2023 •

edited

Loading

github-actions bot commented Dec 6, 2023 •

edited

Loading

narcis96 Dec 6, 2023 •

edited

Loading

narcis96 Dec 8, 2023 •

edited

Loading