feat: Safe multisig badge on governance addresses + oracle router governor row - #796
Conversation
|
🚅 Deployed to the euler-lite-pr-796 environment in euler-lite(dev,PR previews)
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded Safe account detection, cached on-chain lookups, oracle governor discovery, and shared vault address rendering. Vault overviews now display Safe badges and oracle governor addresses when available. ChangesSafe and governor metadata
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review summary
Display-only PR for Safe multisig badges on governance addresses and a live EulerRouter governor() row. I traced the probe → cache → overview address rows path (including how createOnchainLookupCache treats successes vs throws) and cross-checked oracle/governor helpers already in the repo. One warning stands out; nothing here moves funds or bypasses guards.
Reviewed: useSafeAddressInfo, useOracleRouterGovernor, safe-account / onchain-lookup-cache, overview address components, ABIs · Traced beyond the diff: getPublicClient (retryCount 0 + batching), getSpecialAddressLabel, getEulerRouterGovernor / EVault oracle shape, UiHoverPreviewTooltip · Protocol skills consulted: euler-vaults, euler-irm-oracles
Findings
⚠️ Warnings
composables/useSafeAddressInfo.ts / composables/useOracleRouterGovernor.ts — RPC failures cached as definitive negatives for 5 minutes
createOnchainLookupCache correctly skips caching when a probe throws, but both probes swallow failures and return null instead:
- Safe:
Promise.allSettled+resolveSafeAccountInfo(...)→nullwhen any/all reads reject (transport error looks the same as an EOA). - Governor: bare
catch { return null }treats RPC/proxy failures like “not Governable”.
Those nulls are then cached under ${chainId}:${address} for CACHE_TTL_MS (5 min). The shared public client also sets retryCount: 0, so a single 502/timeout from /api/internal/rpc/{chainId} sticks.
Trigger: Open a vault Addresses section while the internal RPC proxy is briefly unhealthy (this environment already sees intermittent upstream 502s). Refreshing within the TTL still serves the cached null and does not re-probe.
Consequence: Real Safe risk managers / fee receivers / Earn roles lose the (threshold/owners) badge for up to five minutes, and EulerRouter governor rows disappear even when governance is live — including across every vault that shares the same router or Safe address, because the cache is module-scoped.
Fix direction: Rethrow transport / HTTP errors from the probes (so the cache’s existing failure path applies), and only return null for definitive on-chain negatives (empty calldata / execution revert / unknown singleton). Optionally treat “all three Safe reads rejected” as a throw rather than null.
Not flagged
- No transaction / geo / screening / approval surface — badges and address rows only.
- Renounced router governance (
0x0→ “None” viagetSpecialAddressLabel) behaves as described; Safe probes correctly skip sentinels. - Reusing
getEulerRouterGovernoris not practical here:EVault.oracleis only{ oracle, name }; lensoracleDetailedInfois not on the vault object the overview receives, so the on-chaingovernor()read is justified. - Hardcoded Safe singleton list matches canonical 1.1.1–1.5.0 deployments; false-positive risk is low given singleton + threshold/owner invariant checks.
Sent by Cursor Automation: Lite PR Reviewer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@composables/useOracleRouterGovernor.ts`:
- Around line 24-26: Update the readContract error handling in
useOracleRouterGovernor so it returns null only when the error chain contains
viem’s ContractFunctionZeroDataError; rethrow transport and request failures so
createOnchainLookupCache does not cache them. Add coverage for zero-data
returning null and for a subsequent successful lookup after a transport failure.
In `@utils/safe-account.ts`:
- Around line 57-59: Update resolveSafeAccountInfo to validate owners before
returning metadata: reject duplicate addresses, the zero address, and the Safe
owner sentinel address, while preserving the existing threshold checks. Add test
cases covering each invalid owner-list condition.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: euler-xyz/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a0a923cf-f1aa-4830-a8d2-979f1e965095
⛔ Files ignored due to path filters (1)
assets/sprite/svg/safe.svgis excluded by!**/*.svg
📒 Files selected for processing (16)
abis/oracle.tsabis/safe.tscomponents/entities/safe/SafeAccountBadge.vuecomponents/entities/vault/overview/SecuritizeVaultOverview.vuecomponents/entities/vault/overview/VaultOverviewAddressValue.vuecomponents/entities/vault/overview/VaultOverviewBlockAddresses.vuecomponents/entities/vault/overview/earn/VaultOverviewEarnBlockAddresses.vuecomponents/entities/vault/overview/earn/VaultOverviewEarnBlockManagement.vuecomposables/useOracleRouterGovernor.tscomposables/useSafeAddressInfo.tstests/composables/useOracleRouterGovernor.test.tstests/composables/useSafeAddressInfo.test.tstests/utils/onchain-lookup-cache.test.tstests/utils/safe-account.test.tsutils/onchain-lookup-cache.tsutils/safe-account.ts
Transport-level RPC failures now rethrow instead of being cached as negative detections for the TTL. Owner lists violating Safe's OwnerManager invariants (zero, sentinel, duplicate owners) are rejected as lookalikes.
|
Review findings addressed in 7bdd0f2:
|
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
There was a problem hiding this comment.
The visual change is clean in the Railway preview, and the earlier transport/owner-invariant findings are fixed on this head. One merge blocker remains: npm run typecheck fails at tests/utils/safe-account.test.ts:58 because toUpperCase().replace(...) widens the second owner from Address to string (TS2322). The focused feature suite passes (26/26), lint has no errors, and git diff --check is clean.
Deployment note, separate from the source blocker: GitHub marks Railway deployment 5832107876 successful for this exact Lite SHA, but the currently served SDK bundle still contains the pre-fix safeAccountService probe (immediate null on rejected masterCopy, without the transport-failure and owner-invariant handling now present at SDK PR #94 head 5ebab4bc). Lite itself does not reference safeAccountService, so the screenshots validate the local Lite implementation and visual delta, not integration with the latest SDK head.
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
There was a problem hiding this comment.
Two non-CI points from the earlier-head review remain on this exact head: one control-boundary wording correction and one accessibility improvement.
|
Re: review 4897002255 — both findings are now fixed:
The SDK-bundle caveat is expected: Lite deliberately uses its local probe until an SDK release ships |
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
There was a problem hiding this comment.
The CI/typecheck blocker is resolved on this exact head: local npm run typecheck, the focused owner-list test, and all three GitHub CI jobs pass. I am not re-approving yet because the separate control-boundary wording finding remains unchanged at SafeAccountBadge.vue:9: Safe modules can execute through execTransactionFromModule without owner confirmations, so the tooltip must describe the configured X-of-Y owner threshold rather than claim every transaction requires those signatures. The accessibility aria-label point remains non-blocking.
…ature requirement
|
Re: review 4897209489 — both points addressed in b5d8cd3:
|
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
There was a problem hiding this comment.
Reviewed exact head b5d8cd38306b99479f0cb0673a91f1af6f715614.
The previous control-boundary blocker is resolved. The tooltip now describes only the configured X-of-Y owner threshold, explicitly avoids claiming that every Safe execution requires owner confirmations, and the badge exposes the same status through a dynamic accessible name.
Validation:
npm run typecheck— passednpx vitest run tests/utils/safe-account.test.ts— 7/7 passednpm run build— passed- GitHub lint, test, and typecheck checks — green on this head
git diff --check— clean
No remaining blockers found.
Mirrors OwnerManager GS203 (owner != address(this)) — a lookalike listing itself as owner no longer passes.
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
There was a problem hiding this comment.
Reviewed exact head 6adf4f999d1059122ab4a8d7164892ffb8f814b9.
The self-ownership parity fix is correct: the local Safe probe now rejects a recognized-singleton lookalike that lists its own address as an owner, matching Safe OwnerManager's GS203 invariant. The regression test covers it alongside zero, sentinel, duplicate-owner, threshold, and malformed-return cases.
Validation:
npx vitest run tests/utils/safe-account.test.ts: 8/8 passednpm run typecheck: passednpm run build: passedgit diff --check origin/development...HEAD: clean- GitHub lint/typecheck/test checks: green
Non-blocking: the internal SafeAccountInfo.threshold JSDoc still says "required to execute a transaction"; it would be more exact to mirror the badge/SDK wording and scope this to owner-authorized transactions, since modules can execute without owner signatures. This does not affect the rendered badge copy.
No blockers.
Use the published package and drop unused deployment fields removed from the SDK surface.
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
There was a problem hiding this comment.
Re-reviewed the delta since the prior approved head and verified current head 0b1bd5015b7119bde6c1b235d030beb4eaaab978.
Local verification:
- clean
npm ci npm run test:run: 1,639 passed, 1 skippednpm run typechecknpm run build- built-server smoke:
GET /returned 200 with the Euler Lite page
No blocking findings.
4 participants
Summary
governor()); renounced governance renders as "None"VaultOverviewAddressValuecomponentImplementation
masterCopy()(special-cased in every Safe proxy since v1.1.1) is validated against the canonical singleton list from safe-deployments (v1.1.1–v1.5.0 incl. eip155 variants), thengetThreshold()/getOwners()provide the badge data; Safe threshold/owner invariants are enforced so lookalikes are rejecteduseSafeAddressInfo/useOracleRouterGovernorcomposables cache perchainId:address(5-min TTL, in-flight dedup, transient RPC failures not cached) via a new reusablecreateOnchainLookupCacheutil; the three probe reads fire concurrently and coalesce into one RPC request via transport batchingcheck-safeprop): risk manager, fee receivers, oracle governor, Earn owner/curator/guardian, Securitize governorsafeAccountService(feat: add safeAccountService for Safe smart-account detection euler-sdks#94); this app-side probe is intentionally self-contained and can swap tosdk.safeAccountService.fetchSafeAccountonce a release ships with itTest plan
tests/utils/safe-account.test.ts)tests/utils/onchain-lookup-cache.test.ts)tests/composables/useSafeAddressInfo.test.ts,tests/composables/useOracleRouterGovernor.test.ts)nuxt typecheckand ESLint cleanSummary by CodeRabbit
New Features
Bug Fixes