Rename xdai to gnosis, mainnet to ethereum (where possible). - #114
Rename xdai to gnosis, mainnet to ethereum (where possible).#114pclaesen wants to merge 1 commit into
Conversation
Viem only supports gnosis and mainnet as chain name, 'ethereum' is not a supported chain name. Fixes RequestNetwork#34
|
Hello @pclaesen, thank you for submitting your first pull request to the request-scan repository. We value your contribution and encourage you to review our contribution guidelines to ensure your submission meets our standards. Please note that every merged PR is automatically enrolled in our Best PR Initiative, offering a chance to win $500 each quarter. Our team is available via GitHub Discussions or Discord if you have any questions. Welcome aboard! |
WalkthroughUpdates blockchain chain identifiers throughout the codebase by replacing XDAI with GNOSIS, removing MAINNET from enums, adding ETHEREUM mappings, and refactoring GraphQL payment network field references across multiple query definitions to reflect new chain source mappings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thank you for your submission! As you prepare for the review process, please ensure that your PR title, description, and any linked issues fully comply with our contribution guidelines. A clear explanation of your changes and their context will help expedite the review process. Every merged PR is automatically entered into our Best PR Initiative, offering a chance to win $500 every quarter. We appreciate your attention to detail and look forward to reviewing your contribution! |
Greptile OverviewGreptile SummaryRenamed chain identifiers from The changes update:
The actual chain values remain correct ( A minor style inconsistency exists in Confidence Score: 4.5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant App as Application
participant Consts as consts.ts
participant Queries as GraphQL Queries
participant GQL as GraphQL Client
participant Subgraph as The Graph Subgraph
participant Viem as Viem Public Clients
Note over Consts: CHAINS.ETHEREUM = "mainnet"<br/>CHAINS.GNOSIS = "gnosis"
Note over Consts: PAYMENT_CHAINS.ETHEREUM = "payment_ethereum"<br/>PAYMENT_CHAINS.GNOSIS = "payment_gnosis"
App->>Queries: fetchPayments()
Queries->>GQL: Execute PAYMENTS_QUERY
Note over Queries: Query uses payment_ethereum<br/>instead of payment_mainnet
GQL->>Subgraph: payment_ethereum { payments(...) }
GQL->>Subgraph: payment_gnosis { payments(...) }
Subgraph-->>GQL: Payment data
GQL-->>Queries: Formatted results
Queries-->>App: Payment[]
App->>Consts: Access PUBLIC_CLIENTS[CHAINS.ETHEREUM]
Consts->>Viem: createPublicClient({ chain: mainnet })
Note over Viem: Viem expects "mainnet" and "gnosis"<br/>as valid chain identifiers
Viem-->>Consts: Public client instance
Consts-->>App: Configured client
App->>Consts: Access CHAIN_SCAN_URLS[CHAINS.ETHEREUM]
Consts-->>App: "https://etherscan.io"
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| payment_ethereum { | ||
| payments( | ||
| where: { reference: $reference } | ||
| orderBy: timestamp | ||
| orderDirection: desc | ||
| where: { reference: $reference } |
There was a problem hiding this comment.
Inconsistent parameter ordering: where comes after orderBy in payment_ethereum block, while all other blocks have where first.
| payment_ethereum { | |
| payments( | |
| where: { reference: $reference } | |
| orderBy: timestamp | |
| orderDirection: desc | |
| where: { reference: $reference } | |
| payment_ethereum { | |
| payments( | |
| where: { reference: $reference } | |
| orderBy: timestamp | |
| orderDirection: desc |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/queries/request-payments.ts
Line: 59:63
Comment:
Inconsistent parameter ordering: `where` comes after `orderBy` in `payment_ethereum` block, while all other blocks have `where` first.
```suggestion
payment_ethereum {
payments(
where: { reference: $reference }
orderBy: timestamp
orderDirection: desc
```
How can I resolve this? If you propose a fix, please make it concise.| payment_arbitrum_one { | ||
| payments( | ||
| where: { reference: $reference } |
There was a problem hiding this comment.
Inconsistent parameter ordering: where comes before orderBy in payment_arbitrum_one block, while the old payment_mainnet had it after. Consider keeping same order as other blocks.
| payment_arbitrum_one { | |
| payments( | |
| where: { reference: $reference } | |
| payment_arbitrum_one { | |
| payments( | |
| where: { reference: $reference } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/queries/request-payments.ts
Line: 14:16
Comment:
Inconsistent parameter ordering: `where` comes before `orderBy` in `payment_arbitrum_one` block, while the old `payment_mainnet` had it after. Consider keeping same order as other blocks.
```suggestion
payment_arbitrum_one {
payments(
where: { reference: $reference }
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
|
Heads-up before this merges — I think it would break all payment data in production, and I'd rather flag it than let it land quietly. CI passing doesn't cover this, because nothing here actually queries Hasura. The problemThe diff doesn't just rename TypeScript identifiers, it renames the Hasura remote-schema names inside the GraphQL documents, in five files: - payment_mainnet { - payment_xdai {
+ payment_ethereum { + payment_gnosis {Those are wire names, not labels. If Hasura still exposes the remotes as Evidence that the remotes have not been renamedFrom the production console on scan.request.network today, the SRF query fails with: The query text in that same error contains An inconsistency that suggests the same conclusion
ETHEREUM: "mainnet", // key renamed, value preserved ✅But ETHEREUM = "payment_ethereum", // ⚠️ was "payment_mainnet"
GNOSIS = "payment_gnosis", // ⚠️ was "payment_xdai"
Suggested shapeThe readability win is real and worth having — it just needs to stop at the identifiers:
That gives you Context on overlap#117 restructures these same five query files (one request per chain via |
|
Fine for me, thanks @rodrigopavezi ! |
One unavailable subgraph currently destroys **all** payment data. Stacked on #116. ## On #114 — do not merge it first This touches the same six files as #114 (`xdai → gnosis`, `mainnet → ethereum`). I originally suggested merging #114 first to avoid conflicts. **Having read it, I withdraw that.** #114 renames the Hasura remote-schema names *inside the GraphQL documents* (`payment_xdai` → `payment_gnosis`, `payment_mainnet` → `payment_ethereum`). Production evidence says those remotes are still `payment_xdai`/`payment_mainnet`: the SRF validation error in the console today lists both in the query text and Hasura rejects only `payment_zksyncera`'s inner field — and Hasura validates the whole document, so an unknown top-level field would have been reported too. So merging #114 as-is would take payment data from "broken on one chain" to "broken on every chain". Details and a suggested identifier-only fix are in a comment on #114. This PR does not need to wait for it. ## The problem, live in production right now The payment and SRF queries were single GraphQL documents aliasing 11-14 chain remotes at once. Hasura returns `data: null` when any one remote errors, so a single unavailable chain destroyed the entire result: ``` fetchRequestPayments Error: subgraph not found: no allocations field 'singleRequestProxyDeployments' not found in type: 'payment_zksynceraQuery' ``` Confirmed by loading the site: **`/payments` renders completely empty**, "Recent payments" shows "No data", and **every request page reports `Balance: 0`** no matter what was actually paid. Thirteen healthy chains were being discarded because of one. For actual users this is worse than the 500 in #116 — an explorer that reports every request as unpaid. ## The fix Each fan-out is now one request per chain via `Promise.allSettled`, merged through the existing unchanged formatters. A rejected chain is skipped and logged with its name; healthy chains still return data. Only a **total** failure of every chain throws — a full outage must not be indistinguishable from "no results", and #116's error states surface it. Output is byte-identical when all chains are healthy. **One thing worth knowing, preserved rather than "fixed":** today's behaviour is already per-chain top-N, not global top-N — `first: 10` across 14 chains can return 140 rows, because the formatters merge and sort but never apply a global slice. Changing that would alter visible row counts and pagination, so it deserves its own decision rather than riding along here. ## Also here - **Dropped `payment_zksyncera` from the SRF queries only** — that subgraph genuinely lacks the `singleRequestProxyDeployments` entity, so querying it could only ever error. Expressed as data in `consts.ts` via the pre-existing but unused `PAYMENT_CHAINS` enum. Note `SRF_CHAIN_REMOTES` is *not* "all chains minus zksyncera": the SRF queries never covered fantom, fuse or moonbeam, so an explicit list was the only way to preserve today's coverage exactly. - **`hooks/payments.ts` was a byte-for-byte duplicate** of `queries/payments.ts` whose `fetchPayments` swallowed errors and returned `[]`, making failure indistinguishable from empty at the query layer. Nothing imports it; it is now a thin re-export rather than a second copy of the same bug. - **A CI smoke check** that actually serves the app and requests `/request/<id>`, failing on 5xx. This is the signal that was missing for #116's bug: `next build` only compiles that route, `npm run check` is formatting only, and there are no tests. Dummy env, no secrets, polls for readiness rather than sleeping. ## Trade-off, accepted deliberately Up to **14 parallel HTTP round-trips** per logical query instead of 1. `revalidate: 60` and Hasura's `@cached` blunt it, and no batching layer or dependency was added. Worth watching Hasura rate limits. ## Not addressed - **`queries/transactions.ts` and `address-transactions.ts` have the identical fragility** against the two *storage* remotes — if `storage_sepolia` is down, mainnet requests vanish too. Left alone to keep this diff scoped; they need the same treatment. - **Restoring the missing subgraph is upstream indexer work**, not a change in this repo (likely `payments-subgraph` / indexer allocations). This PR stops one dead chain from zeroing the UI; it does **not** bring the data back. `Balance: 0` will persist until the subgraph is served again. ## Verification `npm run build` ✅, `tsc --noEmit` clean, biome clean, built query documents validated through `graphql.parse`. **Not verified:** happy-path merge against live data — the local Hasura URL is a stand-in, so this is verified by construction and by its degradation behaviour, not against production data.
3 participants
Viem only supports gnosis and mainnet as chain name, 'ethereum' is not a supported chain name. Fixes #34
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.