refactor(thegraph)!: simpler getTheGraphClient interface #1621
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## Walkthrough
The changes centralize and extend the logic for determining TheGraph subgraph client URLs by introducing a new `getTheGraphClientUrl` function, allowing an explicit `url` override in options. Related imports, exports, and test cases were updated for consistency. Documentation was also revised for minor textual and formatting improvements.
## Changes
| File(s) | Change Summary |
|------------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `packages/payment-detection/README.md` | Updated textual and formatting issues: capitalized "TypeScript," corrected "subgraphes schema" to "subgraphs schema," and standardized shell code block tags to `sh`. No functional or code example changes. |
| `packages/payment-detection/src/index.ts` | Added `getTheGraphClientUrl` to imports and exports from `./thegraph`. |
| `packages/payment-detection/src/payment-network-factory.ts` | Changed import from `defaultGetTheGraphClient` to `getTheGraphClient` and updated usage accordingly. No logic changes. |
| `packages/payment-detection/src/thegraph/client.ts` | - Added optional `url` property to `TheGraphClientOptions`. <br> - Renamed and replaced `defaultGetTheGraphClientUrl` with `getTheGraphClientUrl` that prioritizes `url` option. <br> - Changed `getTheGraphClient` signature to accept only network and options, deriving URL internally. <br> - Removed `defaultGetTheGraphClient`. <br> - Centralized and simplified client creation and URL resolution. |
| `packages/payment-detection/test/thegraph/client.test.ts` | Replaced all references to `defaultGetTheGraphClientUrl` with `getTheGraphClientUrl` in imports and test cases. Added a new test verifying that an explicitly passed URL option is returned directly. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Caller
participant PaymentNetworkFactory
participant TheGraphClient
participant getTheGraphClientUrl
Caller->>PaymentNetworkFactory: buildOptions()
PaymentNetworkFactory->>TheGraphClient: getTheGraphClient(network, options)
TheGraphClient->>getTheGraphClientUrl: (network, options)
getTheGraphClientUrl-->>TheGraphClient: subgraph URL (custom or derived)
TheGraphClient-->>PaymentNetworkFactory: TheGraphClient instance
PaymentNetworkFactory-->>Caller: buildOptions resultPossibly related PRs
Suggested reviewers
|
| @@ -57,6 +62,7 @@ export { | |||
| initPaymentDetectionApiKeys, | |||
| getDefaultProvider, | |||
| getTheGraphClient, | |||
| getTheGraphClientUrl, | |||
There was a problem hiding this comment.
Fixed: the export was missing at the package level
alexandre-abrioux
changed the title
alexandre-abrioux
changed the title
github-actions
bot
requested review from
aimensahnoun,
bassgeta,
kevindavee and
leoslr
leoslr
approved these changes
May 15, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
defaultGetTheGraphClientUrlat the package level in feat(payment-detection): option to use TheGraph Network with API key #1610defaultGetTheGraphClientanddefaultGetTheGraphClientUrlis a bit out of place when looking at the convention we have in other packages (seegetDefaultIpfsUrl,getDefaultIpfsTimeout,getDefaultEthereumProvider,getDefaultProvider,getDefaultEthereumBlockConfirmations,getDefaultCurrencyVersion,CurrencyManager.getDefault, etc.)defaultGetTheGraphClientandgetTheGraphClientare redundant:defaultGetTheGraphClientaccepts a network andoptionsgetTheGraphClientaccepts a network, an URL, and the sameoptionsoptionsalready contains connection parameters (e.g.theGraphExplorerApiKey), so it could very well host a new optional "URL" parameter. This would allow merging both methods into one.Changes
I added the missing export and renamed/merged methods listed above. Conclusion: interface for TheGraph clients is simpler/less confusing.
Note
This is a breaking change as the interface for
getTheGraphClientchanges slightly. Regarding the other methods:defaultGetTheGraphClientwas not exported at the package level, OKdefaultGetTheGraphClientUrlwas not exported at the package level, OKSummary by CodeRabbit
New Features
Documentation
Refactor