feat: add safeAccountService for Safe smart-account detection - #94
Conversation
|
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:
📝 WalkthroughWalkthroughThe SDK adds a ChangesSafe Account Service
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant SafeAccountService
participant ProviderService
participant SafeContract
Caller->>SafeAccountService: fetchSafeAccount(chainId, account)
SafeAccountService->>ProviderService: probe Safe view functions
ProviderService->>SafeContract: read masterCopy, getThreshold, getOwners
SafeContract-->>ProviderService: return probe results
ProviderService-->>SafeAccountService: return singleton, threshold, owners
SafeAccountService-->>Caller: return SafeAccountInfo or null
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@packages/euler-v2-sdk/docs/safe-account-service.md`:
- Line 38: Update the safe-account probe caching described in the service
documentation so rejected masterCopy, getThreshold, and getOwners RPC reads are
not stored as null; distinguish transport failures from expected contract-call
reverts, cache only valid results or expected null outcomes, and ensure each
transient failure retries on the next call.
- Line 30: Update fetchSafeAccount so Safe detection validates proxy bytecode,
the implementation slot, nonzero unique owners, and threshold invariants before
accepting or authorizing the account; otherwise explicitly mark the result
heuristic and prevent authorization use. Also revise the Promise.allSettled
failure path so transient null probe results are not cached, or document the
resolved-null caching behavior in the relevant caching documentation.
In `@packages/euler-v2-sdk/src/sdk/sdk.ts`:
- Line 61: Update EulerSDKOptions.safeAccountService to be optional and, during
EulerSDK construction, default a missing value to new
SafeAccountService(options.providerService). Preserve explicitly supplied
safeAccountService instances and ensure sdk.safeAccountService is always
initialized for direct callers.
In `@packages/euler-v2-sdk/src/services/safeAccountService/safeAccountService.ts`:
- Around line 145-168: Update probeSafeAccount and fetchSafeAccount so transient
provider/RPC rejections from masterCopy, getThreshold, or getOwners are
propagated or otherwise excluded from the five-minute negative cache, while
preserving null for confirmed non-Safe contract failures. Add a test where the
first probe fails at the RPC layer and the second call retries the reads instead
of returning the cached null.
- Around line 163-183: Update the Safe metadata probe around the existing
singleton, threshold, and owners checks to fetch the account runtime bytecode
with getBytecode and validate it against the known SafeProxy runtime bytecode.
Return null when bytecode is missing or does not match a recognized SafeProxy
runtime, and only construct SafeAccountInfo after this validation succeeds.
🪄 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: 54deab59-1089-4633-9232-d4b727d66259
📒 Files selected for processing (13)
packages/euler-v2-sdk/README.mdpackages/euler-v2-sdk/docs/safe-account-service.mdpackages/euler-v2-sdk/docs/services.mdpackages/euler-v2-sdk/src/index.tspackages/euler-v2-sdk/src/sdk/buildSDK.tspackages/euler-v2-sdk/src/sdk/sdk.tspackages/euler-v2-sdk/src/services/safeAccountService/index.tspackages/euler-v2-sdk/src/services/safeAccountService/safeAccountService.tspackages/euler-v2-sdk/src/services/safeAccountService/safeAccountServiceTypes.tspackages/euler-v2-sdk/test/safeAccountService.test.tsskills/euler-sdk/AGENTS.mdskills/euler-sdk/SKILL.mdskills/euler-sdk/rules/sdk-architecture.md
Transport-level RPC failures now reject fetchSafeAccount instead of being cached as null. Owner lists violating Safe's OwnerManager invariants (zero, sentinel, duplicate owners) are rejected. EulerSDKOptions.safeAccountService is optional with a providerService-backed default so direct EulerSDK construction stays source-compatible. Docs state the heuristic nature of detection explicitly.
|
Review findings addressed in 5ebab4b:
|
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
There was a problem hiding this comment.
The earlier transport-cache, owner-invariant, direct-constructor, and heuristic-authorization findings are addressed cleanly on this head. The current SDK suite is green locally (524 tests, lint, build, and typecheck), and the mainnet KPK fixtures also resolve correctly: Safe v1.4.1 at 2/5, fee receiver 5/8, and the router governor matches the displayed Safe.
One API-semantics blocker remains: a non-Safe contract that returns malformed non-empty fallback data causes viem to reject with an ABI-decoding error, and the service currently rethrows it as if it were a transport outage. I reproduced this with viem's public AbiDecodingDataSizeTooSmallError; fetchSafeAccount rejects instead of returning null. That contradicts the public interface/docs for non-Safe contracts and diverges from the Lite implementation, which only rethrows errors classified as transport failures.
The fix can remain narrow: classify ABI decode failures from successful contract responses as definitive non-Safe outcomes (while preserving transport rethrows), and add the malformed-response regression case.
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
There was a problem hiding this comment.
One additional control-boundary wording point survived the earlier-head review and is still present on this exact head.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/euler-v2-sdk/docs/safe-account-service.md (1)
30-30: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winQualify the one-RPC-request claim for custom providers.
ProviderServiceenables viem multicall batching, butIProviderServicedoes not require it andbuildEulerSDK()accepts provider overrides. Qualify the claims atsafe-account-service.md:30andsafe-account-service.md:42and the method comment, or issue the probe throughprovider.multicallexplicitly.🤖 Prompt for 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. In `@packages/euler-v2-sdk/docs/safe-account-service.md` at line 30, Qualify the one-RPC-request and multicall batching claims in safe-account-service.md around the probe description and the referenced method comment to apply only when the provider supports multicall batching. Also update the corresponding method documentation to describe the behavior for custom provider overrides, without asserting batching as a universal guarantee.
🤖 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.
Outside diff comments:
In `@packages/euler-v2-sdk/docs/safe-account-service.md`:
- Line 30: Qualify the one-RPC-request and multicall batching claims in
safe-account-service.md around the probe description and the referenced method
comment to apply only when the provider supports multicall batching. Also update
the corresponding method documentation to describe the behavior for custom
provider overrides, without asserting batching as a universal guarantee.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: euler-xyz/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 712eb5b9-68c9-4f0d-9491-58ca0dfd811e
📒 Files selected for processing (5)
packages/euler-v2-sdk/docs/safe-account-service.mdpackages/euler-v2-sdk/src/sdk/sdk.tspackages/euler-v2-sdk/src/services/safeAccountService/safeAccountService.tspackages/euler-v2-sdk/src/services/safeAccountService/safeAccountServiceTypes.tspackages/euler-v2-sdk/test/safeAccountService.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/euler-v2-sdk/test/safeAccountService.test.ts
- packages/euler-v2-sdk/src/services/safeAccountService/safeAccountService.ts
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
There was a problem hiding this comment.
Reviewed exact head 212e72093e6d9593d546fb904aeb1429a38871c3.
The current commit is formatting-only. It fixes the Biome delta, but the two previously identified semantics/control-boundary findings remain unchanged:
-
Malformed non-Safe contract responses are still treated as transport outages. A non-Safe fallback returning malformed non-empty data produces viem
AbiDecodingDataSizeTooSmallError(wrapped byContractFunctionExecutionError). The classifier does not recognize it, sofetchSafeAccount()rejects rather than resolvingnullas the public interface and docs promise. An exact-head runtime probe rejected twice and performed six reads; a definitive negative should resolvenulland be cached after one three-read probe. Classify ABI response-decoding failures as definitive contract negatives while preserving genuine transport rethrows, and add the malformed-response regression test. -
The exported threshold wording still overstates the control boundary.
SafeAccountInfo.thresholdand the service docs describe the number of signatures required to execute a transaction. Enabled Safe modules can callexecTransactionFromModulewithout owner confirmations, and this heuristic does not inspect modules or guards. Describe this as the owner-signature threshold for owner-authorized Safe transactions / configured owner threshold.
Validation:
- focused
safeAccountService.test.ts— 11/11 passed - full SDK suite — 524/524 passed, no type errors
release:check/ build / typecheck — passed- changed Safe service files — formatted cleanly
- package lint still reports only pre-existing warnings outside this PR's files
git diff --check— clean
The implementation is otherwise well bounded: transient transport failures are not cached, in-flight probes coalesce, owner invariants are enforced, direct constructor compatibility is preserved, and heuristic-only authorization guidance is explicit.
Supplement to the exact-head reviewThe independent fresh-challenge pass found one additional blocker on Reject Safe self-ownership when applying OwnerManager invariants.
Please compare normalized owners against the probed Non-blocking documentation point from the same pass: the "one RPC request" claim is guaranteed only when the injected provider supports/configures batching; |
Malformed non-empty fallback data (viem ABI-decoding errors) now classifies as a definitive non-Safe negative instead of a transport rethrow. Self-ownership is rejected per OwnerManager GS203. Threshold docs describe the owner-signature threshold for owner-authorized transactions only, and the single-RPC-request claim is scoped to batching providers.
|
All second-round findings addressed in aba22c0:
525 tests + typecheck + biome check clean. The same self-ownership fix landed in Lite's local probe (euler-xyz/euler-lite@6adf4f99). |
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
There was a problem hiding this comment.
Reviewed exact head aba22c0f503e4b7441ac516e7587ea96d4479db9.
The self-ownership check, owner-threshold wording, and custom-provider batching qualification are fixed correctly. The new malformed-scalar regression also proves the original 0x01 / AbiDecodingDataSizeTooSmallError case.
One narrowed blocker remains: the implementation and updated docs now classify malformed non-empty ABI responses generally as definitive non-Safe results, but the classifier covers only two decoder error names/message shapes. Real malformed dynamic getOwners() output commonly raises other viem decoder errors. I decoded a truncated address[] result using viem 2.48.8; it raised PositionOutOfBoundsError, wrapped it in ContractFunctionExecutionError as readContract does, and passed that through SafeAccountService. fetchSafeAccount() rejected instead of resolving/caching null. A malicious huge offset similarly raises IntegerOutOfRangeError.
Please classify the contract-response decoding family rather than only the scalar-size subtypes, and add at least one dynamic-array regression. Genuine HTTP/RPC transport failures must continue to reject and remain uncached.
Validation:
- committed Safe service tests: 12/12 passed
- full SDK suite: 525/525 passed, no type errors
- package lint: passed with pre-existing informational warnings outside this PR
- build, typecheck, and release check: passed
git diff --check: clean- adversarial truncated-
getOwners()regression: failed as described; temporary test removed and worktree clean
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
There was a problem hiding this comment.
Reviewed exact head aba22c0f503e4b7441ac516e7587ea96d4479db9.
The self-owner invariant, threshold/module wording, batching-provider wording, and scalar malformed-data regression are fixed correctly. One ABI-decoding boundary remains, so I am keeping this blocked.
Blocker — dynamic-result decoder errors still escape as transport failures.
getOwners() returns a dynamic array. Malformed non-empty returndata can therefore make viem throw decoder errors outside the two newly allowlisted names. I reproduced both through the real service and real viem error wrappers:
truncated array -> PositionOutOfBoundsError
2 calls -> 2 rejected ContractFunctionExecutionError results, 6 reads
huge offset -> IntegerOutOfRangeError
2 calls -> 2 rejected ContractFunctionExecutionError results, 6 reads
Both are definitive malformed contract responses, not RPC/transport failures. Under the documented contract they should return null and cache that negative after the first three reads. The new test only exercises scalar malformed data (AbiDecodingDataSizeTooSmallError), so it does not discriminate malformed dynamic-array results.
Please classify the relevant viem ABI-decoder family for these fixed no-argument reads and add at least one malformed dynamic getOwners() regression asserting null plus one-probe caching.
Validation:
- focused Safe service tests: 12/12 passed
- full SDK suite: 525/525 passed, no type errors
pnpm run release:check: passedgit diff --check origin/main...HEAD: clean- exact-head runtime challenge above: failed as described
Supplement to the exact-head reviewOne non-blocking wording inconsistency remains on
The batching-provider qualification is correct elsewhere. Replacing these two remaining instances with "one probe" (or provider-dependent wording) would make the public explanation consistent. This does not add another merge blocker beyond the dynamic-decoder failure in the formal review. |
…gatives Truncated getOwns arrays and out-of-range dynamic offsets raise viem cursor/encoding errors (PositionOutOfBoundsError, IntegerOutOfRangeError) that were falling through to the transport rethrow. The classifier now covers the AbiDecoding family by name pattern plus the cursor/encoding response-decoding classes; regression tests generate the real errors via decodeFunctionResult so they track viem's actual behavior.
|
Dynamic ABI boundary finding fixed in c0fc64a:
526 tests + typecheck + biome check clean. |
Concurrent callers share one in-flight three-read probe; only batching providers collapse the reads into a single RPC request.
|
Non-blocking wording supplement addressed in fdcfc59: the two remaining "one RPC round-trip" claims ( |
LeonardEulerXYZ
left a comment
LeonardEulerXYZ
left a comment
There was a problem hiding this comment.
Reviewed exact head fdcfc59b35d7a3ab958b02748bf039dea9a00b36.
All published findings are resolved.
The dynamic ABI classifier now handles the real viem PositionOutOfBoundsError and IntegerOutOfRangeError paths, and the regression generates those errors through decodeFunctionResult rather than synthetic names. I independently reran both hostile getOwners() cases twice:
truncated dynamic array -> [null, null], 3 reads
huge dynamic offset -> [null, null], 3 reads
That confirms definitive-negative classification and one-probe caching. Transport failures remain retryable, self-ownership is rejected, threshold wording is scoped to owner-authorized transactions/modules, and the remaining in-flight wording now accurately says one shared three-read probe.
Validation:
- focused Safe service tests: 13/13 passed
- full SDK suite: 526/526 passed, no type errors
- release check/build/typecheck: passed
- exact-head real-viem challenge: passed
git diff --check origin/main...HEAD: clean
No blockers.
|
Docs completeness check against the #93 pattern (a4e7d08): the SDK-side docs were already in place ( Coordination note: #93's branch bumps the skill to 1.3.1. Whichever PR merges second should rebase its changelog/metadata on top of the other (if #93 lands first, this stays 1.4.0; if this lands first, #93's 1.3.1 becomes 1.4.1). |
…-shape guidance
Safe v1.1.1/v1.2.0 permitted self-ownership (the GS203 restriction arrived
in v1.3.0); the probe deliberately applies the strict rule to every
allowlisted version, so a canonical legacy Safe listing itself as an owner
reads as null. Documented in the service docs, the invariant comment, and
a regression test with a legacy positive control.
The skill guidance claimed all fetch* methods return { result, errors };
fetchSafeAccount() returns SafeAccountInfo | null — exception added to
SKILL.md, AGENTS.md, and the sdk-architecture rule.
3 participants
Summary
Adds
safeAccountService— on-chain detection of Safe (ex Gnosis Safe) smart accounts with signer configuration reads. First consumer is the euler-lite governance-address badge (euler-xyz/euler-lite#796); it also serves connected-wallet Safe checks in app transaction flows.API
getSafeSingletonVersion(address)andsafeAccountAbiare exported for callers that need the pieces directly.Implementation
masterCopy()/getThreshold()/getOwners()concurrently; the provider's multicall batching coalesces them into a single RPC request.masterCopy()is special-cased in every Safe proxy's fallback since v1.1.1, so EOAs and non-Safe contracts read as null rather than erroring${chainId}:${account}(5-min default TTL, configurable) with in-flight dedup; RPC failures are not cached so later calls retrybuildSDK/EulerSDKwithservicesOverrides.safeAccountServicesupport; docs page + services map + README index + skills updatedTest plan
test/safeAccountService.test.ts: canonical detection, unknown singleton, EOA/non-Safe, invariant lookalikes, per-key caching, concurrent probe dedup, TTL expiry,buildEulerSDKwiring + overriderelease:check(clean + build + typecheck) green; biome cleanSummary by CodeRabbit
New Features
Documentation
Tests