feat: change subgraph constructor to use full urls

[fredmarques]

Description

This PR fixes #2105

Changes

Subgraph constrictor now uses full urls

How to test

All unit tests still pass

Related Issues

Fixes #2105

[github-actions]

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

[fredmarques]

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

[MartinquaXD]

Thanks for your contribution!
Looks good so far. It's just that we have to pass in 1 additional argument because we need to use 2 different URLs for balancer and uniswap.

[fleupold]

crates/driver/src/infra/config/file/mod.rs also needs to get the url on both the BalancerV2 as well as the Uniswapv3 struct. Can we also make sure the existing integration tests (not part of CI) are still working?

[fredmarques]

Thanks for the the directions @fleupold
All tests are green now.

• ✅ test-db
• ✅ test-driver
• ✅ test-local-node
• ✅ unit-tests

FYI: this is my first time doing some rust after reading the Rust Book so I much appreciate the patience on providing some directions.

[MartinquaXD]

Also thanks for being patient with our feedback. We know that all the config code is not the easiest to maneuver. :)

Only thing left is to move the config arguments to the right location for the driver.
In the end you should be able to configure something like:

[[liquidity.balancer-v2]]
vault = "0x..."
# ...other balancer args
graph-url = "https://..."

instead of:

[liquidity]
balancer-graph-url = "https://..."

[[liquidity.balancer-v2]]
# args without graph-url

[fleupold]

Hi @fredmarques, this PR looks pretty good already, only missing thing seems to be to move uniswap_v3_graph_url into the UniswapV3Config struct (and balancer_v2_graph_url into BalancerV2Config).

Are you still planning to do this change?

[fredmarques]

Oh yes! I was fixing some tests and something else required my attention. Let me finalize it.

[fredmarques]

I'm facing this issue:

error[E0658]: use of unstable library feature 'arc_unwrap_or_clone'
  --> crates/shared/src/price_estimation/trade_finder.rs:58:15
   |
58 |             ..Arc::unwrap_or_clone(self.inner)
   |               ^^^^^^^^^^^^^^^^^^^^
   |
   = note: see issue #93610 <https://github.com/rust-lang/rust/issues/93610> for more information

For more information about this error, try `rustc --explain E0658`.

Are we using nightly rust?

[MartinquaXD]

Are we using nightly rust?
Nope, but this function was stabilized in the most recent Rust version. Try updating your Rust to 1.76.

[fredmarques]

that must be it. I'm on 1.75.
updating...

[fredmarques]

I'm trying to run test-forked-node with

FORK_URL_MAINNET="https://eth.merkle.io" FORK_URL_GNOSIS="https://rpc.gnosischain.com/"  cargo nextest run -p e2e forked_node --nocapture --run-ignored ignored-only

I'm getting this error:

Failed to create genesis: GetAccount(0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266, Server returned an error response: ErrorPayload code -32002, message: "No state available for block 32070725 (0x942186094b13c1670898d24c9449f7bce4f26bd216a64839501fd03a5c8d6142)", contains payload: true

I assume my FORK_URL_GNOSIS is wrong.

[fredmarques]

We should have this documented. If not, with public URLs (I don't know why it's a secret on Git Hub Actions), at least the step-by-step procedure on how to obtain such URLs.

[fleupold]

For forked tests you need an archive node. If you don't have access to one (I don't know of a free version for Gnosis Chain), don't worry we can let the CI run those tests (they are unlikely to fail due to this code change)

[fredmarques]

Great! Now I only see outdated conversationi so can we run the CI one more time? I got all green tests on my local.

[github-actions]

This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed.

[fredmarques]

@fleupold could you do another review?

[fleupold]

Sorry for the delay here. Will re-run the tests. Unfortunately I noticed a configuration issue that I'm not happy with (somewhat silently failing with default values on other chains).

I'm not sure if you still have energy to work through that. Otherwise I'll try to amend this PR sometime this week.

[fleupold]

It's not great that this default now only works for mainnet.
I wonder if it would be better to make this value Option and implement the default based on the current connected chain similar to how we have it currently

[MartinquaXD]

Since the graph is shutting down for good there is no public URL that we could be using for these defaults.
Will make all values optional and only enable the liquidity source if the URLs have been set. Also there are a bunch of old and ignored tests that need the public URLs. I will delete them since they will no longer be functional.

[MartinquaXD]

Hi, since the graph apparently actually shut down the old hosted graphs for good we need this PR merged now ASAP to be able to index balancer and uniswap v3 liquidity again.
I will push your PR over the finish line. Thanks for your contribution anyway. 🚀

changes addressed and this hotfix is urgent