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

Eng 61 update subgraph endpoints #2708

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

@adamgall adamgall self-assigned this Feb 3, 2025
Copy link

linear bot commented Feb 3, 2025

Copy link

cloudflare-workers-and-pages bot commented Feb 3, 2025

Deploying decent-interface with  Cloudflare Pages  Cloudflare Pages

Latest commit: 3b3066f
Status: ✅  Deploy successful!
Preview URL: https://517f6b5a.decent-interface.pages.dev
Branch Preview URL: https://eng-61-update-subgraph-endpo.decent-interface.pages.dev

View logs

src/graphql/index.ts Outdated Show resolved Hide resolved
src/components/DaoHierarchy/DaoHierarchyNode.tsx Outdated Show resolved Hide resolved
- Moved GraphQL queries to separate files for better organization
- Updated import paths for GraphQL queries
- Implemented client caching in GraphQL client creation
- Added new query files for Snapshot, Streams, and DAO queries
- Removed deprecated snapshot client creation method
- Renamed GraphQL query constants from *QueryDocument to *Query
- Updated import statements in various files to use new query names
- Affected files include DaoHierarchyNode, DAOQueries, StreamsQueries, and several hooks
- Renamed `createDecentGraphClient` to `createDecentSubgraphClient`
- Renamed `createSablierGraphClient` to `createSablierSubgraphClient`
- Renamed `createSnapshotGraphClient` to `createSnapshotSubgraphClient`
- Updated import statements across multiple files to use new function names
- Renamed `SubgraphConfig` to `TheGraphConfig` in types
- Updated network configs to use `decentSubgraph` instead of `subgraph`
- Modified GraphQL client creation to use the new configuration type
- Removed `version` field from TheGraphConfig type
- Updated GraphQL client creation to use 'version/latest' for dev environments
- Removed version configurations from all network config files
@adamgall adamgall marked this pull request as ready for review February 4, 2025 19:11
- Update `.env` and `vite-env.d.ts` to use `VITE_APP_THEGRAPH_API_KEY`
- Modify GraphQL client creation to use the renamed environment variable
- Consistent naming for TheGraph API key across the application
Comment on lines -68 to -73
"build:cf": "vite build && vite build --mode page-function",
"graphql:build": "graphclient build",
"graphql:dev-server": "graphclient serve-dev",
"test": "vitest --dir=test",
"postinstall": "npm run graphql:build"
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥

Comment on lines -184 to -185
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

👀 sweet if we don't need this anymore. tested Myosin good stress tester for org page.

Comment on lines +14 to +18
// Check if we already have a client for this URL
const cachedClient = clientCache.get(url);
if (cachedClient) {
return cachedClient;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we cache the client? looking at the documentation, and is this instead of create a context provider?

Copy link
Member Author

Choose a reason for hiding this comment

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

Decided to implement this lil caching mechanism (thanks for the suggestion @mudrila), specifically because I've removed the context provider.

Wherever we need to utilize a graphql client, we create (or get from cache) an instance of it on the fly, instead of using the "one" that would be in a Provider context.

Decided to do it like this because of how diverse our graphql clients are. Effectively the "url" property of a urql client instance is the thing that makes it unique, and given that there are 3 dimension in which this "url" can differ (dev vs prod (2), decent vs sablier (2), all the networks (5)), there are 2 * 2 * 5 = 20 different potential endpoints for the urql client. Rather than just include one of those client instances in a Provider context, i said fuggit and we just create one (or pull from cache) as needed.

I didn't really do any profiling to see how expensive it is to create these different clients, but the caching layer here is so light it seemed like a simple thing to implement.

@@ -146,14 +158,15 @@ export const useFractalGovernance = () => {
type,
]);

const proposalsLoadKey = useRef<string>();
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -346,7 +337,7 @@ const getPaymentStreams = async (
});

const streamsWithCurrentWithdrawableAmounts: SablierPayment[] = await Promise.all(
formattedLinearStreams.map(async stream => {
formattedLinearStreams.map(async (stream: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

you mean to leave as type any?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah shit, good point. I'm realizing now that there's no typing for the result of these queries...

sigh, I'll need to get back into this.

Copy link
Member Author

Choose a reason for hiding this comment

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

oke pushed some more code

);
const formattedLinearStreams = lockupLinearStreams.map(lockupLinearStream => {
const formattedLinearStreams = lockupLinearStreams.map((lockupLinearStream: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

you mean to leave as type any?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

- Add type definitions for DAO, Snapshot, and Streams GraphQL queries
- Enhance type safety in GraphQL query calls across multiple hooks and components
- Improve error handling and data validation in GraphQL query processing
- Remove unnecessary fields and simplify query response types
@adamgall adamgall force-pushed the ENG-61_update-subgraph-endpoints branch from b744035 to 406b343 Compare February 5, 2025 03:25
Copy link
Contributor

@Da-Colon Da-Colon left a comment

Choose a reason for hiding this comment

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

Good deal. Send it

@adamgall
Copy link
Member Author

adamgall commented Feb 5, 2025

Definitely need to some more manual testing on Snapshot integration. I think that's the code that had the most actual changes which I'm most unsure about and haven't yet manually tested.

edit: Ok yeah I tested it, seems to work.

@Da-Colon
Copy link
Contributor

Da-Colon commented Feb 7, 2025

This also fixes an issue that was found when doing a subgraph lookup on polygon https://linear.app/decent-labs/issue/ENG-182/deployed-a-polygon-dao-does-not-load

Copy link

linear bot commented Feb 7, 2025

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.

3 participants