feat(wallets): quorum member plumbing in SignerManager and server resolver (M4-4 part 2/4) - #1998
Conversation
…olver SignerManager learns the quorum member APIs the selection flows build on: quorumMemberLocators/matchQuorumMembers/adoptQuorumMemberConfig/ adoptedAssemblableQuorumMember, per-member secret stripping, and actionable errors listing member locators in require()/withRecoverySigner() (replacing QuorumSignerNotSupportedError there). withRecoverySigner() now runs with the active signer when it is a quorum member. ServerSignerResolver treats quorum server member addresses as admin addresses so a matching legacy derivation wins and is cached as the recovery resolution. Dormant until useSigner member selection lands in the follow-up PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| adoptQuorumMemberConfig(memberLocator: string, merged: ResolvedQuorumMember): void { | ||
| if (this.#recovery.type !== "quorum") { | ||
| return; | ||
| } | ||
| this.#recovery = { | ||
| ...this.#recovery, | ||
| signers: this.#recovery.signers.map((member) => | ||
| getQuorumMemberLocator(member) === memberLocator ? merged : member | ||
| ), | ||
| }; | ||
| this.#adoptedQuorumMemberLocators.add(memberLocator); | ||
| } |
There was a problem hiding this comment.
adoptQuorumMemberConfig silently records adoption on a locator miss
If memberLocator does not match any existing quorum member, the map leaves signers unchanged but #adoptedQuorumMemberLocators.add(memberLocator) still fires. The phantom entry is functionally benign (adoptedAssemblableQuorumMember iterates signers and never finds the orphaned locator), but a caller that passes a mis-typed locator receives no signal that the replacement was a no-op. Consider asserting the member was found, or at minimum logging a warning, so part-4 usages can surface this class of mistake early.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/wallets/services/signer-manager.ts
Line: 117-128
Comment:
**`adoptQuorumMemberConfig` silently records adoption on a locator miss**
If `memberLocator` does not match any existing quorum member, the `map` leaves `signers` unchanged but `#adoptedQuorumMemberLocators.add(memberLocator)` still fires. The phantom entry is functionally benign (`adoptedAssemblableQuorumMember` iterates signers and never finds the orphaned locator), but a caller that passes a mis-typed locator receives no signal that the replacement was a no-op. Consider asserting the member was found, or at minimum logging a warning, so part-4 usages can surface this class of mistake early.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Taken in 936bd5e: adoptQuorumMemberConfig now no-ops and logs signerManager.adoptQuorumMemberConfig.noSuchMember when the locator matches no member, so nothing is recorded on a miss. No current call site can hit it (callers derive the locator from the matched member itself), but the invariant is now explicit and observable. Test added.
There was a problem hiding this comment.
Confirmed. In commit 936bd5e8, adoptQuorumMemberConfig now guards the .add() with an existence check:
if (!this.#recovery.signers.some((member) => getQuorumMemberLocator(member) === memberLocator)) {
walletsLogger.warn("signerManager.adoptQuorumMemberConfig.noSuchMember", { memberLocator });
return;
}A locator miss logs a warning and returns early — #adoptedQuorumMemberLocators.add is only reached after the member is confirmed to exist. No phantom entry, invariant is explicit and observable. Test coverage was also added in signer-manager.test.ts (+52 lines). LGTM.
Prompt To Fix All With AI### Issue 1
packages/wallets/src/wallets/services/signer-manager.ts:117-128
**`adoptQuorumMemberConfig` silently records adoption on a locator miss**
If `memberLocator` does not match any existing quorum member, the `map` leaves `signers` unchanged but `#adoptedQuorumMemberLocators.add(memberLocator)` still fires. The phantom entry is functionally benign (`adoptedAssemblableQuorumMember` iterates signers and never finds the orphaned locator), but a caller that passes a mis-typed locator receives no signal that the replacement was a no-op. Consider asserting the member was found, or at minimum logging a warning, so part-4 usages can surface this class of mistake early.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(wallets): quorum member plumbing in..." | Re-trigger Greptile |
There was a problem hiding this comment.
Claude found:
The resolver returns the cached member's key for a different member's address
packages/wallets/src/signers/server/resolver.ts:52-62 + signers/descriptors/server.ts
resolveDerivation()'s API-sourced branch returns #resolvedRecoveryServerSigner without comparing config.address:
if (isApiSourcedServerSignerConfig(config)) {
if (this.#resolvedRecoveryServerSigner != null) {
return this.#resolvedRecoveryServerSigner; // ← no address check
}
Pre-PR this was sound: there was exactly one admin server signer, so the single slot could only ever hold it. #apiQuorumServerMemberAddresses breaks that invariant — a quorum now has several admin-side server identities sharing one slot.
Reproduced. After selecting member A, asking for member B's key material returns A's:
✗ B: the resolver hands back the cached member's key for a DIFFERENT member's address
- Expected 0xc4FE5bABae8C7e9210E1FcA549eFf650C157F632 ← member B
+ Received 0x532b3a57A2765EB3DcBcd2348AA5e91F78D6b6fC ← member A
✗ C: canAutoAssemble reports a secret-less member assemblable off another member's cache
expected { type: 'server', … } to be null
Repro C is the gate that lets this through: serverSignerDescriptor.canAutoAssemble returns ctx.serverSigners.hasRecoveryResolution — a global boolean, not a per-address check. So adoptedAssemblableQuorumMember() green-lights member B off member A's cache, and assembly silently substitutes A. In a threshold-2 quorum that means the second approval goes in as
A again and the transaction never reaches threshold.
Suggested guard — and note the secret-bearing branch 8 lines below already compares addresses, so this is consistent with the surrounding design, not a new concept:
if (isApiSourcedServerSignerConfig(config)) {
const cached = this.#resolvedRecoveryServerSigner;
if (cached != null && cached.derivedAddress === config.address) {
return cached;
}
throw new Error(...);
}
…plus the same predicate in canAutoAssemble. Longer term, key the cache by address instead of one slot.
There was a problem hiding this comment.
Confirmed and fixed in 936bd5e — both call sites are now per-address:
resolveDerivation()'s API-sourced branch serves the cache only whencached.derivedAddress === config.address, otherwise it throws the existing actionable error (now naming the address).- The server descriptor's
canAutoAssemblecomparesctx.serverSigners.resolvedRecoveryAddress === config.addressinstead of the globalhasRecoveryResolution.
So an unresolved member now degrades to the honest "call wallet.useSigner() with the member you hold" path instead of silently substituting the cached member's key. Safe for single-admin flows: matchesRecovery/matchQuorumMembers both guarantee the selected derivation's address equals the recovery/member API address, so the cache always matches its own config there.
Added regressions: cached-A-refuses-B in resolver.test.ts, the wrong-address canAutoAssemble case in descriptors.test.ts, and an adopted-but-unresolved member returning null from adoptedAssemblableQuorumMember() in signer-manager.test.ts.
Agreed on keying the cache by address as the root-cause fix — deferring that to a follow-up (M4-7 candidate) since threshold >1 is API-gated at launch and the guard closes the substitution path.
There was a problem hiding this comment.
The fix is correctly implemented. Here's what I verified:
resolver.ts — resolveDerivation() address guard:
const cached = this.#resolvedRecoveryServerSigner;
if (cached != null && cached.derivedAddress === config.address) {
return cached;
}
throw new Error(`Cannot resolve server signer derivation for "server:${config.address}": ...`);The === config.address check is in place. A cache hit for member A throws for member B's config rather than silently returning A's key material. ✓
server.ts — canAutoAssemble() per-address check:
return !isApiSourcedServerSignerConfig(config) || ctx.serverSigners.resolvedRecoveryAddress === config.address;The global hasRecoveryResolution boolean is gone; it now compares the resolved address against config.address. The resolvedRecoveryAddress getter (return this.#resolvedRecoveryServerSigner?.derivedAddress ?? null) is present in the resolver. ✓
Both root causes are closed: the substitution path in resolveDerivation and the false-positive canAutoAssemble that let it propagate through adoptedAssemblableQuorumMember. The fix is consistent with the surrounding single-signer design (which already did address comparison in the secret-bearing branch).
There was a problem hiding this comment.
Colude found:
stripSecretFromRecovery() wipes secrets from members that were never resolved
packages/wallets/src/wallets/services/signer-manager.ts:152-170
The quorum arm strips every server member that has an address. The comment justifies it with "once a server member's derivation is resolved (and cached in the resolver), its secret is no longer needed" — but the code never checks that it was resolved. The single-signer arm immediately below does check (resolvedRecoveryAddress != null).
This matters because ServerSignerResolver has exactly one recovery cache slot (#resolvedRecoveryServerSigner), so at most one member can ever be "resolved."
Reproduced. Quorum with server members A and B, both carrying secrets — exactly what part 3 (#1999) mergeQuorumRuntimeConfig produces via { ...method, ...member }:
✗ A: stripSecretFromRecovery() wipes the secret of a member that was never resolved
expected undefined to be 'bbbb…' ← B's secret, gone
Mirror the single-signer arm:
member.type === "server" &&
"secret" in member &&
member.address === this.#serverSignerResolver.resolvedRecoveryAddress
There was a problem hiding this comment.
Confirmed and fixed in 936bd5e — the quorum arm now mirrors the single-signer arm exactly as you suggested: only the member whose address === resolvedRecoveryAddress is stripped, so a member the resolver never resolved keeps its grafted secret until it's selected (at which point the slot holds its derivation and it gets stripped in turn).
Updated the existing strip test to pin this: a quorum with a resolved member (0xM), an unresolved secret-bearing member (0xOther), and a no-API-address member — only 0xM loses its secret.
There was a problem hiding this comment.
The fix is correct. In stripSecretFromRecovery() (lines 152–170), the quorum arm now mirrors the single-signer arm exactly:
member.type === "server" &&
"secret" in member &&
member.address != null &&
member.address === resolvedRecoveryAddress // ← the fixOnly the member whose address matches resolvedRecoveryAddress gets its secret stripped. Previously all server members with an address were stripped unconditionally. The member.address != null guard also correctly leaves the no-API-address fallback path untouched.
Fix verified. ✓
… per address Review fixes (Alberto): - ServerSignerResolver.resolveDerivation no longer serves the cached recovery resolution for a different API address, and the server descriptor's canAutoAssemble checks resolvedRecoveryAddress against the config's address instead of the global hasRecoveryResolution flag — with quorum server members the single cache slot holds one of several admin identities, and silently substituting another member's key material must be impossible. - stripSecretFromRecovery's quorum arm now strips only the member whose derivation is the cached one, mirroring the single-signer arm; other members' secrets survive until they are selected. - adoptQuorumMemberConfig no longer records an adoption when the locator matches no member (logs a warning instead). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Reviews (2): Last reviewed commit: "fix(wallets): scope the server recovery ..." | Re-trigger Greptile |
🔥 Smoke Test Results❌ Status: Failed Statistics
Test DetailsThis is a non-blocking smoke test. Full regression tests run separately. |
2 participants
Linear: WAL-11292 · EDD §6.5 · Part 2/4 of the M4-4 stack (split from #1996)
Summary
The
SignerManager+ServerSignerResolverplumbing theuseSignermember-selection flows (part 4) sit on. Dormant until part 4 lands — nothing can select a quorum member yet — but each piece is unit-tested here on its own contract:SignerManagermember APIs:quorumMemberLocators(),matchQuorumMembers()(via the part-1 shared matcher),adoptQuorumMemberConfig()(replaces one member with the API-merged config and records the selection in an adopted-locator set — deliberately never the quorum-wideadoptRecoveryConfig),adoptedAssemblableQuorumMember()(only members the caller explicitly selected this session, and only if they can auto-assemble).stripSecretFromRecovery()gains the quorum arm — resolved server members drop theirsecret, members without an API address pass through.QuorumSignerNotSupportedErrorinrequire()andwithRecoverySigner(): they now list the member locators and point atwallet.useSigner().withRecoverySigner()(used byaddSigner/removeSigner) runs with the active signer when it is a quorum member — no reassembly, no silent member picking (EDD-strict).ServerSignerResolver: quorum server-member addresses joinapiRecoveryAddressas admin addresses (#isAdminAddress), so a legacy derivation matching a member wins the pick and is cached as the recovery resolution.Wallet's constructor feeds the member addresses in.Testing
signer-manager.test.ts: require/withRecoverySigner quorum arms, adoption + assemblable-member gating, per-member secret strip.resolver.test.ts: legacy derivation via quorum member address, cached as recovery resolution.packages/walletssuite: 719 passed.🤖 Generated with Claude Code