refactor(morpho-sdk): split public/wallet client roles#602
Conversation
MorphoClient now takes a viem public client typed as Client<Transport, Chain> (chain required, account never read). Entity-layer validation drops validateUserAddress and only verifies chainId. Permit / permit2 signing moves the address+chain check to Requirement.sign(walletClient, userAddress), typed as Client<Transport, Chain, Account>, via the new validateWalletClient helper. Permit2 signing now also verifies chainId. Docs (root + client/entities/actions AGENTS.md, README, BUNDLER3) updated to state the two-client contract explicitly. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace inline Client<Transport, Chain> / Client<Transport, Chain, Account>
with viem-style aliases exported from morpho-sdk:
type PublicClient = Client<Transport, Chain>
type WalletClient = Client<Transport, Chain, Account>
Used:
- PublicClient on MorphoClient, MorphoClientType, getRequirements,
getMorphoAuthorizationRequirement, getRequirementsPermit, encodeErc20Permit.
- WalletClient on Requirement.sign / ERC20PermitAction.sign and the
encodeErc20Permit{,2} sign callbacks.
Drop the validateWalletClient wrapper: sign callbacks now call
validateChainId(client.chain.id, chainId) and
validateUserAddress(client.account.address, userAddress) explicitly.
Inline checks elsewhere (getRequirements, getMorphoAuthorizationRequirement,
encodeErc20Permit) replaced by validateChainId.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…/ WalletClientWithChain Renamed to disambiguate from viem's PublicClient / WalletClient and signal that the chain (and account, for the wallet variant) is mandatory. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…undled builders Add a one-line callout in JSDoc of every builder where mismatching userAddress and msg.sender silently misroutes funds: - marketV1: supplyCollateral, repay, repayWithdrawCollateral, supplyCollateralBorrow - vaultV1: deposit, migrateToV2 - vaultV2: deposit Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…curacy
- types/client.ts: rename type parameters to PascalCase T-prefix and reorder
WalletClientWithChain params to match viem's <TTransport, TChain, TAccount>
- encodeErc20Permit{,2}: drop the now-unreachable @throws MissingClientPropertyError
from sign() (account is type-required); add @throws ChainIdMismatchError from
sign() to reflect the new validateChainId(client.chain.id, chainId) check;
switch encodeErc20Permit example to createPublicClient
- getRequirements{,Permit}, getMorphoAuthorizationRequirement: JSDoc references
PublicClientWithChain instead of generic 'viem Client'; example switched to
createPublicClient
- BUNDLER3.md: clarify that Requirement.sign userAddress check only fires on
permit/permit2 signing paths — integrator owns the invariant elsewhere
Skipped (intentional design choices, not bugs):
- runtime null-checks before chain.id / account.address — TS narrowing on
PublicClientWithChain / WalletClientWithChain is the contract; defensive
checks would defeat the purpose
- constructor runtime shape validation on MorphoClient — same rationale
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@codex review |
…less clients Restore optional chaining on every client property access before validateChainId / validateUserAddress: - viemClient.chain?.id (was .chain.id) in entities and action helpers - client.chain?.id and client.account?.address (was direct deref) in sign callbacks The PublicClientWithChain / WalletClientWithChain types narrow the happy path, but viem still allows chain/account undefined at runtime, and TS escape-hatches (wagmi useClient, framework boundaries, generic types) bypass the type guard. Without the ?., a chainless or accountless client throws a raw TypeError instead of the SDK's typed errors (ChainIdMismatchError / MissingClientPropertyError). validateChainId and validateUserAddress already handle undefined inputs by throwing the typed error. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… accountless clients" This reverts commit 861e4b1.
0xbulma
left a comment
There was a problem hiding this comment.
https://github.com/morpho-org/sdks/pull/602/changes#r3188919027
To be agreed on before merging
BREAKING: `MorphoClient` no longer wraps a viem `Client`. Constructor now accepts
a `MorphoConfig`:
new MorphoClient({
transports: { 1: http(rpcUrl), 8453: http(baseRpcUrl) },
supportSignature: true,
});
The SDK builds a fresh viem `Client` per entity via the new public method
`MorphoClient.getViemClient(chainId)`, throwing `UnsupportedChainError` when
no transport is configured for the requested chain.
A single `MorphoClient` instance now serves multiple chains in parallel —
useful for indexers, dashboards, and backends that operate across networks.
Other changes:
- drop `PublicClientWithChain` / `WalletClientWithChain` type aliases. Action
helpers now accept plain `Client<Transport>`; sign callbacks accept
`Client<Transport, Chain | undefined, Account>`.
- drop `morphoViemExtension` (no longer meaningful — SDK doesn't wrap a client).
- new error: `UnsupportedChainError`.
- entities build their own viem client at construction; remove all per-method
`validateChainId` calls (chain id is fixed at entity creation).
- test helper `morphoFromTestClient` bridges Anvil test clients to `MorphoConfig`.
- docs: AGENTS.md (root + client + entities + actions), README updated.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace Client<Transport, Chain | undefined, Account> with viem's bare WalletClient (chain optional, account optional via default generics) on: - Requirement.sign / ERC20PermitAction.sign type signatures - encodeErc20Permit / encodeErc20Permit2 sign callbacks Sign callbacks now narrow account explicitly via an early throw of MissingClientPropertyError when client.account is undefined, before calling validateUserAddress and signTypedData. validateChainId remains a runtime check via client.chain?.id. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…notations `Client<Transport>` resolves to the same type as bare `Client` since viem's `Client` defaults its first generic parameter to `Transport`. Use bare `Client` everywhere for consistency. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…lient
`MorphoClient.getViemClient(chainId)` now returns a viem `PublicClient`
(built via `createPublicClient({ transport })`) instead of a bare `Client`.
Entities store `viemClient: PublicClient` and benefit from the public actions
(readContract, getBlock, multicall, etc.) without an explicit `.extend(publicActions)`.
`getMorphoAuthorizationRequirement` parameter narrowed to `PublicClient` and
its internal `viemClient.extend(publicActions)` removed — `viemClient.readContract(...)`
is called directly.
Public action helpers (`getRequirements`, `getRequirementsPermit`, `encodeErc20Permit`)
keep accepting bare `Client` for integrator flexibility. The SDK's PublicClient
is structurally compatible.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ng a viem client
`getRequirements`, `getRequirementsPermit`, and `encodeErc20Permit` now type
`viemClient` as `PublicClient<Transport, Chain | undefined, Account | undefined>`
instead of bare `Client`. Sign callbacks keep `WalletClient`.
The explicit account/chain generics keep the signature compatible with both
account-less public clients (the SDK's internal client built via
`createPublicClient({ transport })`) and account-bound test/wallet clients.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…where `getRequirements`, `getRequirementsPermit`, and `encodeErc20Permit` now type `viemClient` as bare `PublicClient` (account undefined). Tests that called these helpers directly with the AnvilTestClient now wrap it in a fresh public client via the new `publicFromTestClient` helper. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The two AGENTS.md files described `Requirement.sign` as taking `Client<Transport, Chain | undefined, Account>` but the actual signature is now bare `WalletClient` (chain and account both optional at the type level, runtime-validated in the callback). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Cleaner than the dense one-paragraph version: three runtime checks listed explicitly with the typed error each one throws. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Reverts the MorphoConfig refactor and downstream commits (a35731c..55f48b3). MorphoClient is back to taking an injected viem PublicClient with chain mandatory at the type level (PublicClientWithChain alias). Chain id checks restored everywhere: - entity layer: validateChainId on every read/build - action helpers (getRequirements, encodeErc20Permit*, getMorphoAuthorizationRequirement): validateChainId at top - sign callbacks: validateChainId(client.chain.id, chainId) + validateUserAddress(client.account.address, userAddress) Multi-chain integrators continue to instantiate one MorphoClient per chain. Removed: - MorphoConfig, MorphoClient.getViemClient, UnsupportedChainError - Test helpers morphoFromTestClient / publicFromTestClient Restored: - morphoViemExtension - PublicClientWithChain / WalletClientWithChain type aliases Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The PublicClientWithChain / WalletClientWithChain aliases were dropped in c1012f9; the SDK uses viem's bare PublicClient / WalletClient. Update the three AGENTS.md files that still describe the aliases as exported types so the docs match the source. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…t casts Add a `publicClient: PublicClient` fixture to test/setup.ts built via `createPublicClient` over the Anvil transport, so test call sites pass `publicClient` directly instead of casting the AnvilTestClient with `as unknown as PublicClient`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
Parallel PR Review (Claude) + multirun for validation
Reviewed commit: 8be803a4
| Severity | Count |
|---|---|
| Critical | 2 |
| High | 12 |
| Medium | 15 |
| Low | 15 |
Automated parallel review (7 agents + a validation pass + an independent gap pass). Some findings (below) cannot be anchored to a changed line and live in this body; the rest are inline.
Findings without an anchorable diff line
[HIGH] No changeset. The PR retypes 3 public surfaces (MorphoClient.viemClient: PublicClient, Requirement.sign / ERC20PermitAction.sign: WalletClient, morphoViemExtension's <TClient extends PublicClient>) and removes the entity-side validateUserAddress invariant — at least minor and arguably major per AGENTS.md §2 #10 / §7. Add a changeset describing the WalletClient/PublicClient split, the new sign-time validations, and the loss of the entity-build-time userAddress check.
[HIGH] packages/morpho-sdk/src/actions/requirements/encode/encodeErc20Permit.test.ts — no test for sign-time ChainIdMismatchError. encodeErc20Permit.sign() (line 92) now calls validateChainId(client.chain?.id, chainId) but the test file never asserts this throws.
[HIGH] packages/morpho-sdk/src/actions/requirements/encode/encodeErc20Permit.test.ts — no test for sign-time MissingClientPropertyError. The new path at lines 93-94 has no coverage.
[CRITICAL] packages/morpho-sdk/src/actions/requirements/encode/encodeErc20Permit2.test.ts — no regression test for the newly-added validateChainId (PR's stated fix). Without it the chain-binding fix can be silently regressed. (File was not modified in this PR — add the missing tests.)
[HIGH] packages/morpho-sdk/src/actions/requirements/encode/encodeErc20Permit2.test.ts — no test for sign-time MissingClientPropertyError.
[LOW] packages/morpho-sdk/src/actions/requirements/encode/encodeErc20Permit2.test.ts:53 — rejects.toThrow(new AddressMismatchError(...)) matches by class+message. Use rejects.toBeInstanceOf(AddressMismatchError) per AGENTS.md §5.
[MEDIUM] packages/morpho-sdk/src/entities/vaultV1/vaultV1.test.ts — no test verifies ChainIdMismatchError propagates through any V1 entity method. The PR description's test plan listed this as unchecked.
[MEDIUM] packages/morpho-sdk/src/entities/vaultV2/vaultV2.test.ts — no ChainIdMismatchError propagation test on V2 entity methods.
[LOW] packages/morpho-sdk/src/actions/requirements/encode/encodeErc20Permit.test.ts — no edge-case bigint coverage (zero, MAX_UINT256, MAX_UINT160). AGENTS.md §5 recommends property-based tests on calldata encoders.
Sentinel: REVIEW_DONE_PR — PR #602, 44 findings, mode=LocalPR, commit=8be803a4
- Drop inline `MissingClientPropertyError("client.account")` in
`encodeErc20Permit{,2}.sign` — route through `validateUserAddress` for a
single canonical error message.
- Convert `validateUserAddress` to a TypeScript assertion function so call
sites no longer need a non-null assertion on `client.account`.
- Add MUST-broadcast warning to `MarketV1Actions.borrow` JSDoc — the
`setAuthorization` requirement silently grants GA1 control of the
broadcaster's Morpho account on mismatch.
- Add full JSDoc on `Requirement.sign` (params, returns, 4 typed throws).
- Mark `Requirement` / `RequirementSignature` / `PermitArgs` / `Permit2Args`
fields `readonly`.
- Remove dead `ERC20PermitAction` interface (return type contradicted live
`Requirement.sign`).
- Add per-field JSDoc on `MorphoClientType`.
- Fix `morphoViemExtension` `@example` (used `createWalletClient` which
doesn't satisfy `<TClient extends PublicClient>`).
- `MorphoClient` JSDoc: clarify `account` is ignored on the public client.
- Drop redundant `viemClient.extend(publicActions)` in
`getMorphoAuthorizationRequirement`.
- Fix misleading "verifies it against the connected account" copy in
`encodeErc20Permit` JSDoc.
- Sync `actions/AGENTS.md` with the simpler sign-callback body.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@Foulks-Plb Please all comments in english |
0xbulma
left a comment
There was a problem hiding this comment.
Parallel PR Review (Claude)
Reviewed commit: a5967306 (re-review after refactor(morpho-sdk): apply review feedback from #602)
| Severity | Count |
|---|---|
| Critical | 2 |
| High | 5 |
| Medium | 6 |
| Low | 4 |
Automated parallel re-review (7 agents). The commit addressed many earlier findings (changeset added, JSDoc on Requirement.sign, readonly on Permit args, dead ERC20PermitAction removed, morphoViemExtension example fixed, MUST-broadcast warning added to borrow, message inconsistency removed). New issues introduced + remaining open items below.
What was fixed in a5967306
- Inline
MissingClientPropertyError("client.account")removed;validateUserAddress(client.account?.address, …)is now the single canonical path. validateUserAddressis a TypeScript assertion function.- Changeset
.changeset/morpho-sdk-public-wallet-client-split.mdadded. Requirement.signgot JSDoc with all four@throws.MorphoClient/MorphoClientTypeJSDoc now usesPublicClientconsistently; per-field JSDoc added onMorphoClientType.morphoViemExtension@examplenow usescreatePublicClient.Requirement/RequirementSignature/PermitArgs/Permit2Argsfields arereadonly.ERC20PermitActiondead/inconsistent interface removed.marketV1.borrowMUST-broadcast warning added.- Stale "verifies against connected account" JSDoc fixed.
getMorphoAuthorizationRequirement.tsdropped redundant.extend(publicActions).
Findings without an anchorable diff line
[HIGH] encodeErc20Permit2.test.ts (file unchanged) — no regression test for the chainId fix. PR description still flags Permit2 chainId binding as the security fix; removing the new validateChainId line at L90 of encodeErc20Permit2.ts would silently regress without any test failing. Add a permit.sign(walletClientOnWrongChain, userAddress) test asserting ChainIdMismatchError.
[HIGH] encodeErc20Permit.test.ts — no test for sign-time MissingClientPropertyError. encodeErc20Permit.ts:94 flow throws via validateUserAddress(undefined, ...); not exercised through sign().
[HIGH] encodeErc20Permit2.test.ts — no test for sign-time MissingClientPropertyError either.
[MEDIUM] vaultV1.test.ts / vaultV2.test.ts — no test verifies ChainIdMismatchError propagates through any V1/V2 entity method. The PR description's test plan listed this as unchecked. Wire a chain-mismatched PublicClient through MorphoClient and assert the error class on at least one read and one build path on each entity.
Sentinel: REVIEW_DONE_PR — PR #602, 17 findings, mode=LocalPR, commit=a596730
…variant Drop the duplicated (and diverging) Entities & Actions table that followed the getRequirements flow; keep the canonical one and add the missing migrateToV2 row. Restore a README callout for the userAddress = signer/initiator invariant — the SDK now only enforces it on permit/permit2 paths, so integrators need to see it in the README rather than only in BUNDLER3.md.
|
Replies to the four findings without an anchorable diff line in your re-review: [HIGH] No Permit2 sign-time [HIGH] No sign-time [HIGH] Same gap on [MEDIUM] No |
- Reshape `validateUserAddress` to assert on `Account | undefined` directly so call sites read `client.account` without a non-null assertion. - Drop unreachable `expiration` default in `encodeErc20Permit2`. - Add sign-time `ChainIdMismatchError` and `MissingClientPropertyError` tests on both `encodeErc20Permit` and `encodeErc20Permit2`. - Bump changeset to `major` and itemize retypes / removals / behavior changes. - Add MUST-broadcast symmetry note on `marketV1.withdrawCollateral` JSDoc. - Fix stale `// Create wallet client` comment in `examples/example.ts`.
`vaultV1.deposit`, `vaultV1.migrateToV2`, and `vaultV2.deposit` were touched in
this PR to add the MUST-broadcast warning. AGENTS.md §9 requires adopting the
convention on touched code: strip inline `@param {Type}` annotations, add a
`@throws` line per typed error class, fix the duplicate `@returns` on
`vaultV2.deposit`, and add a runnable `@example` block per method.
Why
The viem
Clienttype is too permissive: chain and account are both optional. Two roles in the SDK have different needs and were both typed asClient:MorphoClientwraps a client used only for reads / tx building — it never signs.Requirement.sign/ERC20PermitAction.signneed a client that can sign — chain and account are required at runtime.Reviewers also flagged that the
userAddress = signer = initiatorinvariant for bundled MarketV1 / VaultV1 / VaultV2 paths (which mixonBehalf=userAddresswithmsg.sendersemantics) was a fund-misroute footgun if mishandled, and was under-documented per builder.What changed
Two viem roles, two viem types
The SDK now uses viem's existing
PublicClientandWalletClientdirectly — no custom aliases.MorphoClient.viemClient: PublicClient— passed tonew MorphoClient(viemClient). Used for reads and tx building. The SDK validateschain?.id === expected chainIdat every read / build viavalidateChainId.Requirement.sign(client: WalletClient, userAddress)/ERC20PermitAction.sign(client: WalletClient, userAddress)— passed by the integrator at sign time.Both viem types keep
chain/accountas optional (Chain | undefined,Account | undefined) at the type level, so the SDK still does runtime validation in the sign callbacks (see below).Sign callbacks own their validation
Sign callbacks (
encodeErc20Permit{,2}.sign) now run their own checks at the top:validateChainId(client.chain?.id, chainId)→ChainIdMismatchErrorif (!client.account) throw new MissingClientPropertyError("client.account")validateUserAddress(client.account.address, userAddress)→AddressMismatchErrorThen produce + verify the EIP-712 signature.
The entity layer drops
validateUserAddress(the public client has no account to compare against anyway). It keepsvalidateChainId(client.viemClient.chain?.id, chainId)on every read / build.Permit2 chainId binding (small fix)
encodeErc20Permit2.signnow also runsvalidateChainId(client.chain?.id, chainId)— it was missing. Previously a wallet client connected to chain X could sign a Permit2 message whose typed-data baked in chain Y; the EIP-712 digest the user saw in their wallet would differ from what the SDK encoded.Per-builder JSDoc warnings
Every bundled builder where mismatching
userAddressandmsg.sendersilently misroutes funds now carries a one-line callout in its JSDoc:marketV1.{supplyCollateral, repay, repayWithdrawCollateral, supplyCollateralBorrow},vaultV1.{deposit, migrateToV2},vaultV2.deposit. The integrator owns the invariant on the no-permit path — the SDK only enforces it on permit-signing paths.DevEx — unchanged
One
MorphoClientper chain (unchanged from main).Breaking
MorphoClient.viemClientnarrowed fromClientto viem'sPublicClient. Callers passing a genericClientmay need to widen to public actions.Requirement.sign/ERC20PermitAction.signparameter narrowed to viem'sWalletClient. Callers passing aPublicClientfail at compile time.MissingClientPropertyError/AddressMismatchErrorforuserAddress≠ connected account at build time. The check fires only at sign time. TheuserAddress = signer = initiatorinvariant is the integrator's responsibility on the no-permit path — see BUNDLER3.md and the per-builder JSDoc warnings.Test plan
tsc --noEmitpassesMAINNET_RPC_URL) — CIPublicClientthrowsChainIdMismatchErrorfrom entity reads / buildsrequirement.sign(walletClient, userAddress)throws on chain mismatch + missing account + account mismatchView Slack thread