feat: gov cache #2851
feat: gov cache #2851
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
📦 Next.js Bundle Analysis for aave-uiThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
|
📦 Next.js Bundle Analysis for aave-uiThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7e11e7354
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (USE_GOVERNANCE_CACHE) { | ||
| const yaeVotes = cacheForData?.pages.flatMap((p) => p.votes.map(adaptCacheVote)) || []; | ||
| const nayVotes = cacheAgainstData?.pages.flatMap((p) => p.votes.map(adaptCacheVote)) || []; | ||
| const combinedVotes = [...yaeVotes, ...nayVotes].sort( | ||
| (a, b) => parseFloat(b.votingPower) - parseFloat(a.votingPower) |
There was a problem hiding this comment.
When NEXT_PUBLIC_USE_GOVERNANCE_CACHE is enabled, useGovernanceVotersSplit returns yaeVotes/nayVotes from the first page only; there’s no exposed fetchNextPage or other trigger to pull subsequent pages. As a result, proposals with more than 50 votes per side will show truncated voter lists (and “View all votes” will still be incomplete) because the hook never loads the remaining pages. Consider either fetching all pages before returning or returning pagination controls so the UI can request more.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds support for reading Governance V3 proposal data from a dedicated “governance cache” GraphQL service (toggleable via env), and refactors proposal list/detail UI to consume a unified, data-source-agnostic shape.
Changes:
- Introduces
GovernanceCacheService+ cache-specific hooks and a unifieduseGovernance*hook layer to switch between subgraph and cache at runtime. - Adds adapters and new canonical governance UI types to normalize proposal/vote data across sources.
- Updates proposal list/detail pages and components to use the unified types; adds a cache-specific lifecycle UI.
Reviewed changes
Copilot reviewed 18 out of 22 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/GovernanceCacheService.ts | New GraphQL client/service for fetching proposals, votes, vote counts, and payloads from the cache API. |
| src/hooks/governance/useGovernanceProposals.ts | New unified hooks that switch between subgraph and cache implementations based on env. |
| src/hooks/governance/useProposalDetailCache.ts | Cache-specific hooks for proposal detail/votes/payloads. |
| src/modules/governance/adapters.ts | New adapters to normalize graph/cache data into shared display types. |
| src/modules/governance/types.ts | New shared governance UI types for list/detail/votes. |
| src/modules/governance/ProposalsV3List.tsx | Refactors list rendering to use unified hooks/types and a new row component. |
| src/modules/governance/ProposalV3ListItem.tsx | Removes the old list item component in favor of the new row implementation. |
| pages/governance/v3/proposal/index.governance.tsx | Switches proposal detail page to unified hooks and conditionally renders graph vs cache lifecycle UI. |
| src/modules/governance/proposal/ProposalOverview.tsx | Migrates overview rendering to unified proposal detail display type. |
| src/modules/governance/proposal/VotingResults.tsx | Migrates voting results rendering to unified types and new voter props. |
| src/modules/governance/proposal/VotersListContainer.tsx | Refactors voter list container to accept unified voter split type. |
| src/modules/governance/proposal/VotersListModal.tsx | Refactors modal to use unified vote/voter types. |
| src/modules/governance/proposal/VotersList.tsx | Updates voter list to use normalized vote display type. |
| src/modules/governance/proposal/VotersListItem.tsx | Updates voter list item to use normalized vote display type. |
| src/modules/governance/proposal/ProposalLifecycleCache.tsx | New cache-specific lifecycle timeline for proposals/payloads. |
| pages/governance/ipfs-preview.governance.tsx | Updates IPFS preview rendering to use unified proposal display type. |
| src/locales/en/messages.po | Updates Lingui catalog references for new/changed governance components. |
| src/locales/en/messages.js | Regenerated compiled English messages bundle. |
| src/locales/es/messages.js | Regenerated compiled Spanish messages bundle. |
| .env.example | Adds cache feature flag + cache GraphQL endpoint env vars. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** Adapt cache vote (votingPower in wei/18 decimals) to normalized VoteDisplay */ | ||
| export function adaptCacheVote(vote: ProposalVote): VoteDisplay { | ||
| return { | ||
| voter: vote.voter, | ||
| support: vote.support, | ||
| votingPower: (parseFloat(vote.votingPower) / 1e18).toString(), |
There was a problem hiding this comment.
adaptCacheVote converts votingPower using parseFloat(...)/1e18, which can lose precision and may produce scientific-notation strings for large/small values. Consider using a big-number based normalization (same approach as the subgraph path) to produce stable, accurate values for sorting and display.
| /** Adapt cache vote (votingPower in wei/18 decimals) to normalized VoteDisplay */ | |
| export function adaptCacheVote(vote: ProposalVote): VoteDisplay { | |
| return { | |
| voter: vote.voter, | |
| support: vote.support, | |
| votingPower: (parseFloat(vote.votingPower) / 1e18).toString(), | |
| /** | |
| * Normalize a wei-denominated value (as a decimal string) into a human-readable | |
| * decimal string with 18 decimals, without using floating-point arithmetic. | |
| * | |
| * Example: | |
| * "1000000000000000000" -> "1" | |
| * "1500000000000000000" -> "1.5" | |
| */ | |
| function normalizeWeiToDecimal(value: string): string { | |
| const base = 10n ** 18n; | |
| const wei = BigInt(value); | |
| const integerPart = wei / base; | |
| const fractionalPart = wei % base; | |
| if (fractionalPart === 0n) { | |
| return integerPart.toString(); | |
| } | |
| let fractionalStr = fractionalPart.toString().padStart(18, '0'); | |
| // Trim trailing zeros from the fractional part | |
| fractionalStr = fractionalStr.replace(/0+$/, ''); | |
| return `${integerPart.toString()}.${fractionalStr}`; | |
| } | |
| /** Adapt cache vote (votingPower in wei/18 decimals) to normalized VoteDisplay */ | |
| export function adaptCacheVote(vote: ProposalVote): VoteDisplay { | |
| return { | |
| voter: vote.voter, | |
| support: vote.support, | |
| votingPower: normalizeWeiToDecimal(vote.votingPower), |
| component={Link} | ||
| href={ROUTES.dynamicRenderedProposal(+proposal.id)} | ||
| > |
There was a problem hiding this comment.
The proposal list row no longer emits the existing analytics event for opening a proposal (previously GOVERNANCE_PAGE.VIEW_AIP). If analytics are still required, re-introduce an onClick handler that calls trackEvent before navigation (while keeping the new data-source-agnostic rendering).
| href={ROUTES.dynamicRenderedProposal(+proposal.id)} | ||
| > |
There was a problem hiding this comment.
proposal.id is typed as string (data-source-agnostic), but it’s coerced with +proposal.id for routing. If an id is ever non-numeric, this will generate NaN and break navigation. Consider typing ids as number at the adapter layer, or validate/parse safely before calling ROUTES.dynamicRenderedProposal.
| NEXT_PUBLIC_USE_GOVERNANCE_CACHE=true | ||
| NEXT_PUBLIC_GOVERNANCE_CACHE_URL=https://governance-cache-api.aave.com/graphql |
There was a problem hiding this comment.
New governance cache env vars were added here, but the CI build action currently hard-codes NEXT_PUBLIC_* env values and does not include these (see .github/actions/build/action.yml). If CI/e2e builds should exercise the cache path, please plumb NEXT_PUBLIC_USE_GOVERNANCE_CACHE / NEXT_PUBLIC_GOVERNANCE_CACHE_URL into the relevant GitHub Actions/workflows as well.
There was a problem hiding this comment.
these done on vercel
| throw new Error(`GraphQL request failed: ${response.status}`); | ||
| } | ||
|
|
||
| return response.json(); |
There was a problem hiding this comment.
graphqlRequest only checks response.ok, but GraphQL servers often return HTTP 200 with an errors array. This will lead to hard-to-debug runtime failures when callers assume data exists. Consider parsing the JSON, checking for errors, and throwing a descriptive error (including errors[0].message / extensions) before returning the data.
| return response.json(); | |
| const body = (await response.json()) as { | |
| data?: unknown; | |
| errors?: { message?: string; extensions?: unknown }[]; | |
| }; | |
| if (Array.isArray(body.errors) && body.errors.length > 0) { | |
| const firstError = body.errors[0] || {}; | |
| const message = firstError.message || 'Unknown GraphQL error'; | |
| const extensions = | |
| firstError.extensions !== undefined | |
| ? ` | extensions: ${JSON.stringify(firstError.extensions)}` | |
| : ''; | |
| throw new Error(`GraphQL request returned errors: ${message}${extensions}`); | |
| } | |
| return body as T; |
| const stateCondition = stateFilter ? `state: { equalTo: "${stateFilter}" }` : ''; | ||
| const filterClause = stateCondition ? `filter: { ${stateCondition} }` : ''; | ||
|
|
There was a problem hiding this comment.
stateFilter is interpolated directly into the GraphQL query string. If it ever contains quotes or unexpected characters, this can break the query and potentially allow query injection. Prefer passing the state filter via GraphQL variables (or strictly whitelisting allowed state values before interpolation).
| export async function getProposalByIdFromCache(id: string): Promise<SimplifiedProposal | null> { | ||
| const query = ` | ||
| query GetProposal($id: String!) { | ||
| allProposalsViews(filter: { proposalId: { equalTo: $id } }) { | ||
| nodes { |
There was a problem hiding this comment.
getProposalByIdFromCache declares $id as String! and compares it to proposalId, but other cache queries treat proposal ids as BigFloat and pass a numeric value. If the schema expects a numeric type here, this query will fail at runtime. Align the variable type with the schema/other functions (e.g., BigFloat!) and pass a numeric value consistently.
| /** Convert 18-decimal vote count string to human-readable number */ | ||
| function formatVotes(votes: string): number { | ||
| const raw = parseFloat(votes) || 0; | ||
| return raw / 1e18; | ||
| } |
There was a problem hiding this comment.
parseFloat on 18-decimal on-chain amounts will lose precision for large vote totals (values exceed JS safe integer range). This can lead to incorrect displayed vote counts/percentages. Consider using a big-number based conversion (e.g., normalizeBN / BigNumber division) and only convert to number at the final formatting step (or keep as string).
General Changes
NEXT_PUBLIC_GOVERNANCE_CACHE_URLandNEXT_PUBLIC_USE_GOVERNANCE_CACHEDeveloper Notes
Add any notes here that may be helpful for reviewers.
Reviewer Checklist
Please ensure you, as the reviewer(s), have gone through this checklist to ensure that the code changes are ready to ship safely and to help mitigate any downstream issues that may occur.
.env.examplefile as well as the pertinant.github/actions/*files