Skip to content

Frontend/improve report view#50

Merged
yvesbou merged 12 commits intomainfrom
frontend/improve-report-view
Mar 6, 2026
Merged

Frontend/improve report view#50
yvesbou merged 12 commits intomainfrom
frontend/improve-report-view

Conversation

@yvesbou
Copy link
Collaborator

@yvesbou yvesbou commented Mar 5, 2026

No description provided.

@vercel
Copy link

vercel bot commented Mar 5, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
defiscan-v2-defiscan-frontend Ready Ready Preview, Comment Mar 6, 2026 0:14am
defiscan-v2-docs Ready Ready Preview, Comment Mar 6, 2026 0:14am
defiscan-v2-tool-frontend Ready Ready Preview, Comment Mar 6, 2026 0:14am

Request Review

…rt-view

# Conflicts:
#	packages/defiscan-frontend/public/data/Steakhouse-USDC/compiled-review.json
#	packages/defiscan-frontend/public/data/index.json
#	packages/defiscan-frontend/public/data/liquity-v2/compiled-review.json
#	packages/defiscan-frontend/src/pages/landing/LandingPage.tsx
#	packages/defiscan-frontend/src/pages/review/views/report/AdminCards.tsx
#	packages/defiscan-frontend/src/pages/review/views/report/CodeSection.tsx
#	packages/defiscan-frontend/src/pages/review/views/report/DependencyCards.tsx
#	packages/defiscan-frontend/src/pages/review/views/report/FundCards.tsx
#	packages/defiscan-frontend/src/pages/review/views/report/ReportView.tsx
@claude
Copy link

claude bot commented Mar 5, 2026

test

@claude
Copy link

claude bot commented Mar 5, 2026

Logic error: governance-tagged human admins excluded from admin count

The humanControlled filter uses isHumanType(a) && !a.isGovernance, so a governance-tagged EOA/Multisig/Timelock is excluded from this array but captured by governance.

The narrative sentence (around line 146) uses {humanControlled.length} for the displayed count. If a protocol has only governance-tagged human admins (e.g., a Multisig with isGovernance: true), the text will say "We identified 0 admins" while that admin visibly renders under the Governance heading on the same screen.

Reference:

const humanControlled = sorted.filter(
(a) => isHumanType(a) && !a.isGovernance,
)
const governance = sorted.filter((a) => a.isGovernance)
const noHumanControl =
humanControlled.length === 0 && governance.length === 0

Suggested fix: Either use humanControlled.length + governance.length in the narrative count, or redefine governance as only non-human-type contracts tagged as governance (which is the more common semantic -- a governance contract is a smart contract, not an EOA).


Note: This review was intended as an inline comment on packages/defiscan-frontend/src/pages/review/views/report/AdminCards.tsx at diff position 26.

@claude
Copy link

claude bot commented Mar 5, 2026

Deleting this test comment - posted in error

@claude
Copy link

claude bot commented Mar 5, 2026

debug test

@claude
Copy link

claude bot commented Mar 5, 2026

Code review

1 issue found.

In AdminCards.tsx, the humanControlled filter excludes governance-tagged human admins (isHumanType(a) && !a.isGovernance). If a protocol only has governance-tagged EOAs/Multisigs, the narrative text reads "We identified 0 admins" while simultaneously showing those admins in the Governance section. See inline comment below referencing AdminCards.tsx at diff position 26.


Inline comment for packages/defiscan-frontend/src/pages/review/views/report/AdminCards.tsx (diff position 26):

Logic error: governance-tagged human admins excluded from admin count

The humanControlled filter uses isHumanType(a) && !a.isGovernance, so a governance-tagged EOA/Multisig/Timelock is excluded from this array but captured by governance.

The narrative sentence (around line 146) uses {humanControlled.length} for the displayed count. If a protocol has only governance-tagged human admins (e.g., a Multisig with isGovernance: true), the text will say "We identified 0 admins" while that admin visibly renders under the Governance heading on the same screen.

Reference:

const humanControlled = sorted.filter(
(a) => isHumanType(a) && !a.isGovernance,
)
const governance = sorted.filter((a) => a.isGovernance)
const noHumanControl =
humanControlled.length === 0 && governance.length === 0

Suggested fix: Either use humanControlled.length + governance.length in the narrative count, or redefine governance as only non-human-type contracts tagged as governance (which is the more common semantic — a governance contract is a smart contract, not an EOA).

@claude
Copy link

claude bot commented Mar 5, 2026

Code review

3 issues found:

Bug 1 — total prop unused in DistributionChart bar width (FundCards.tsx line ~199)

DistributionChart destructures total but computes percentages using maxValue (the largest single entry). The largest bar always renders at 100% width, misrepresenting the distribution. total is correctly passed at both call sites. Fix: use total instead of maxValue in the percentage calculation.

<div className="space-y-3">
{entries.map((entry, index) => {
const percentage =
maxValue > 0 ? (entry.value / maxValue) * 100 : 0
const expandKey = `${keyPrefix}-${entry.fund.address}`
const isExpanded = expandedSet.has(expandKey)

Bug 2 — Timelock missing from HUMAN_ADMIN_TYPES in LandingPage.tsx line 13

compile-data.ts and AdminCards.tsx both treat Timelock as a human admin type, but LandingPage.tsx does not. A protocol whose only admin is a Timelock will have its token value counted in totalTokenValueAtRisk globally, but the landing page table will show 0 human admins for that protocol. Fix: add 'Timelock' to the set on line 13.

const HUMAN_ADMIN_TYPES = new Set(['EOA', 'EOAPermissioned', 'Multisig'])
function computeHumanAdminCount(review: CompiledReview): number {

Bug 3 — funds.length count includes token-only entries excluded from Fund Distribution chart (FundCards.tsx line 115)

funds.length counts all fund entries, but totalFunds is computed from balances/positions only — token-only entries (with tokenInfo but balances: null) contribute $0 and are excluded from chartData. For Liquity V1 (added in this PR): 6 entries counted but only 4 have balance data. Fix: replace funds.length with chartData.length.

{formatUsdValue(totalFunds)}
</span>{' '}
across {funds.length} contract{funds.length !== 1 ? 's' : ''}.
{totalTokenValue > 0 && tokenNames.length > 0 && (
<>
{' '}

@claude
Copy link

claude bot commented Mar 5, 2026

Removing test comment

@claude
Copy link

claude bot commented Mar 5, 2026

inline test body

@claude
Copy link

claude bot commented Mar 5, 2026

Code review comment on packages/config/src/projects/liquity-v1/compiled-review.json (line 1): Wrong output location. CLAUDE.md documents compiled-review.json output exclusively to packages/defiscan-frontend/public/data/slug/. The correctly placed copy already exists in this PR at packages/defiscan-frontend/public/data/liquity-v1/compiled-review.json. No other project has compiled-review.json in its config project folder, and reviewCompiler.ts has no code path that writes here. Please remove packages/config/src/projects/liquity-v1/compiled-review.json from this PR.

@claude
Copy link

claude bot commented Mar 5, 2026

Code review comment on packages/defiscan-frontend/src/pages/landing/LandingPage.tsx (line 14): HUMAN_ADMIN_TYPES is missing Timelock. This set is inconsistent with compile-data.ts and AdminCards.tsx which both include Timelock. Timelock-type admins are not counted in the landing page Admins column but are counted as human-controlled in compile-data.ts when computing totalTokenValueAtRisk. A protocol with only Timelock admins would show 0 in the table Admins column while still having its token value counted in global stats. Fix: const HUMAN_ADMIN_TYPES = new Set(['EOA', 'EOAPermissioned', 'Multisig', 'Timelock'])

@claude
Copy link

claude bot commented Mar 5, 2026

Code review comment on packages/defiscan-frontend/src/pages/review/views/report/AdminCards.tsx (line 146): Admin count and capital figure are drawn from inconsistent populations. The intro sentence says We identified N admins controlling X in locked funds where N = humanControlled.length + governance.length (human-type + governance-tagged admins only) but X = totals.totalCapitalAtRisk (capital reachable by ALL admins including Immutable, plain Contract, and Revoked types). For protocols with immutable contracts controlling funds, X will be larger than what the N named admins actually control, making the sentence misleading. The humanTotal and govTotal variables are already computed just above this JSX. Using humanTotal + govTotal would make count and capital consistent.

@yvesbou
Copy link
Collaborator Author

yvesbou commented Mar 6, 2026

@claude review the changes to the explorer view pls

@claude
Copy link

claude bot commented Mar 6, 2026

Claude finished @yvesbou's task in 1m 59s —— View job


Explorer View Code Review

  • Read explorer view changed files
  • Analyze changes and identify issues
  • Post review feedback

Overall the explorer view is well-structured: sortable tables, expandable rows, and the getHumanAdmins utility cleanly shared across all tabs. A few issues worth addressing:

Bug 1 — Double-counting in Admin Type Distribution bar (OverviewTab.tsx line 37–51)

governanceAdmins is built from humanAdmins.filter((a) => a.isGovernance). A Multisig or Timelock with isGovernance: true lands in both its type bucket (Multisig/Timelock) and the Governance bucket. totalAdminCount is the sum of all buckets, so it exceeds humanAdmins.length in that case. The bar segments then use count / totalAdminCount * 100% and their widths won't add up to 100%.

Additionally, revokedAdmins is pulled from all admins (admins.filter(...) at line 26) rather than from humanAdmins. If revoked admins exist alongside human admins, they inflate totalAdminCount further.

Suggested fix: Filter governance to only non-human-type contracts (!isHumanAdminType(a.adminType) && a.isGovernance) so each admin appears in exactly one bucket; exclude Revoked from the breakdown or base it on humanAdmins only. Fix this →

Bug 2 — "positions" mislabeled as "Market Cap" in FundsTab (FundsTab.tsx)

In the fund data model, positions holds DeFi protocol positions (Aave supply, Morpho vault positions, etc.) — not the protocol token's market cap. The actual market cap is tokenInfo.tokenValue. However:

  • Chart tooltip formatter (line ~153): name === 'balances' ? 'Total Value Locked (TVL)' : 'Market Cap' → calls positions "Market Cap"
  • Table column header (line ~220): <SortHeader field="positions" label="Market Cap" ...> → same mislabeling
  • Chart <Bar> name (line ~175): name="Market Cap" for the positions bar

This makes the "Market Cap Token" column (which correctly uses tokenInfo) redundant/confusing — two columns appear to show market cap. Fix this →

Bug 3 — funds.length count includes token-only holders (FundsTab.tsx line 101)

The summary text "N fund holders" uses funds.length, but chartData filters out entries with no balances or positions (token-only holders with only tokenInfo). A holder with market cap data but $0 TVL still increments the count but shows nothing in the bar chart — users see "6 fund holders" but only 4 bars. (Same issue as previously noted for FundCards.tsx.) Fix this →

Observation — CapitalFlowDiagram.tsx: function circles disconnected from fund lines

The center column renders a circle per admin showing the permissioned function count, with a straight arrow line from the admin box to the circle. However the bezier connections paths (lines 201–218) draw directly from the admin's right edge to the fund holder's left edge — they visually pass through the function column without connecting to the circles. This makes the center column appear decorative: the curved lines bypass the circles entirely.

If the intent is to show the admin → function → fund flow, the lines should route through the function circles (i.e., from admin right → circle center → fund left, split into two segments). If the intent is just to annotate function counts per admin, the column label "FUNCTIONS" is misleading. Fix this →

Minor — AdminsTab.tsx footer totals are global, not filtered-admin totals (lines 194–219)

The table footer displays totals.totalCapitalAtRisk under "Direct Funds" and totals.permissionedFunctionCount under "Functions". These are protocol-wide totals from the compiled review, not sums over the displayed humanAdmins rows. If immutable contracts hold funds or non-human admins control functions, the footer will show a higher number than the sum of visible rows — which looks like an addition error. The table could instead sum humanAdmins.reduce((s, a) => s + a.totalDirectCapital, 0) for the Direct Funds total and sum humanAdmins.reduce((s, a) => s + a.functions.length, 0) for the Functions total.


@yvesbou
Copy link
Collaborator Author

yvesbou commented Mar 6, 2026

@claude review again

@claude
Copy link

claude bot commented Mar 6, 2026

Claude finished @yvesbou's task in 3m 45s —— View job


Code Review

  • Read all changed files
  • Analyze report view changes
  • Analyze explorer view changes
  • Analyze other changes
  • Post review

What's been fixed since the last review

  • AdminCards.tsx: Admin count now uses humanControlled.length + governance.length
  • OverviewTab.tsx: Admin type breakdown bar — each admin now lands in exactly one bucket (!a.isGovernance guard on EOA/Multisig/Timelock buckets, so governance-tagged human-types fall only into the Governance bucket). No double-counting. ✓
  • FundsTab.tsx: "Positions" column is no longer mislabeled "Market Cap" ✓
  • FundsTab.tsx: Summary now uses FundsSummaryLabel with separate tvlCount / tokenCount, not funds.length
  • packages/config/src/projects/liquity-v1/compiled-review.json: No longer present in the wrong location ✓

Remaining issues

Bug — DistributionChart bars relative to maxValue, not total (FundCards.tsx line 198–199)

const maxValue = Math.max(...entries.map((e) => e.value), 0)
// ...
const percentage = maxValue > 0 ? (entry.value / maxValue) * 100 : 0

The total prop is passed to DistributionChart but never used for percentages. The largest bar always renders at 100% width, making each bar show "how much relative to the largest entry" rather than "what share of the total." The first bar will always be 100% wide, and a fund holding 10% of TVL next to one holding 9% will look nearly identical. Fix: const percentage = total > 0 ? (entry.value / total) * 100 : 0.

Fix this →


Bug — Timelock missing from HUMAN_ADMIN_TYPES in LandingPage.tsx line 13

const HUMAN_ADMIN_TYPES = new Set(['EOA', 'EOAPermissioned', 'Multisig'])

admins.ts (the shared utility), compile-data.ts, and AdminCards.tsx all treat Timelock as a human admin type. LandingPage.tsx has its own local copy that excludes Timelock. A protocol whose only active admin is a Timelock without isGovernance: true will show 0 in the "Admins" column of the landing page table, even though that same protocol reports a Timelock admin in its detail view. Fix: add 'Timelock' to the set.

Fix this →


Bug — AdminsTab.tsx footer totals don't match visible rows (lines 194–217)

The "Direct Funds" footer cell shows totals.totalCapitalAtRisk (line 196) and "Functions" shows totals.permissionedFunctionCount (line 216). Both are protocol-wide compiled totals. The table only shows humanAdmins rows. If immutable contracts hold funds or non-human admins control functions, the footer will show a larger number than the sum of the visible rows — which looks like an arithmetic error to the reader.

Fix: replace with sums over the displayed humanAdmins array:

humanAdmins.reduce((s, a) => s + a.totalDirectCapital, 0)  // for Direct Funds
humanAdmins.reduce((s, a) => s + a.functions.length, 0)    // for Functions

Fix this →


Minor / Observation

AdminCards.tsx narrative capital vs admin count mismatch (line 154–155)

The intro sentence says "N admins controlling X in locked funds" where X = totals.totalCapitalAtRisk. This total includes funds controlled by all admin types — Immutable, Contract, Revoked — not just the N human+governance admins displayed. The humanTotal + govTotal variables are already computed directly above and would give a consistent number. Low severity since Immutable contracts typically aren't the dangerous case, but could mislead readers of protocols with significant immutable-locked TVL.

CapitalFlowDiagram.tsx — function circles disconnected from fund flow (design observation)

The bezier connections paths draw directly from the admin box's right edge to the fund holder's left edge (M${adminColX + boxW},${adminCenterY} ... ${fundColX},${fundCenterY}). The function count circles in the center column are connected to admins via a short arrow, but have no outgoing connection to fund holders — the curved lines bypass them entirely. The column label "FUNCTIONS" and the circles are decorative under the current rendering. Either route lines through the circles (admin→circle→fund), or remove the center column and label.


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.

1 participant