Repository navigation
refactor: reduce production complexity across stack - #380
Conversation
|
@copilot make sure the codecov actually passes before we merge this. |
|
ty-everett
left a comment
ty-everett
left a comment
There was a problem hiding this comment.
Maintainer review completed against exact head 593d7059b6eda6487d027664ca1dcbba2780682b.
I reviewed the refactor boundaries, behavior-preserving validation flow, signature/linkage handling, BTMS untrusted-key storage, public-service compatibility, package-version facts, and source-only release boundary. The focused adverse-path additions cover malformed token/envelope/script/input paths and the shared validators without weakening acceptance of additional signed fields.
The earlier Codecov concern is resolved: the authoritative patch check passes at 91.20198% (736 hits, 23 misses, 48 partials) with the repository's 90% threshold unchanged. Sonar reports 0 new issues and 0 security hotspots; CodeQL, Socket, dependency review, audit, build/lint/typecheck/format, coverage, browser/mobile, mutation-quality, governance, conformance, docs, packaging, and merge gates are green. There are no unresolved review threads and I found no merge-blocking issue.
No packages, tags, releases, images, or deployments are created by this PR.
| if (question.service !== 'ls_ship') { | ||
| throw new Error('Lookup service not supported!') | ||
| } | ||
|
|
There was a problem hiding this comment.
[suggestion] Invalid non-object query handling now throws TypeError instead of Error with the same message. The SLAP path has the same change. Message-based callers and the new tests still work, but any consumer that branches on error.name === 'Error', error.constructor === Error, or serializes error class names will observe a patch-level behavioral change.
Suggestion: Keep throwing Error for full compatibility, or document the change and add an explicit regression test asserting error is a TypeError if that is intentional.
| } | ||
|
|
||
| export function validateOptionalStringArray(value: unknown, path: string): void { | ||
| if (value !== undefined && !Array.isArray(value)) { |
There was a problem hiding this comment.
[suggestion] validateOptionalStringArray only checks Array.isArray(value). Values such as topics: [1, 2] or topics: [null] pass validation even though the error text claims "an array of strings". This matches the previous SHIP inline check, but centralizing the helper is the right moment to close the gap; invalid element types can still reach storage/query layers.
Suggestion: Validate every element with typeof item === 'string' (and optionally reject empty strings if protocol requires it). Extend the discovery lookup rejection tests with a non-string array element case.
| onRejectedOutput: (outputIndex: number, error: unknown) => void | ||
| } | ||
|
|
||
| export async function identifyPushDropOutputs({ |
There was a problem hiding this comment.
[suggestion] Several topic managers (BasketMap, CertMap, ProtoMap, WalletConfig) now depend on this shared admit/reject loop, and BTMS / TokenDemo / UMP / Fractionalize were also restructured. New automated coverage appears only for overlay-discovery-services. A regression in the shared loop (shape assertion, empty-admit throw, previousCoins logging gate) would affect multiple managers without a direct unit test net.
Suggestion: Add focused unit tests for identifyPushDropOutputs / assertTransactionInputsAndOutputs (valid output, rejected output, empty admit with/without previousCoins, invalid beef) and at least one smoke test per refactored high-value path (BTMS retention, UMP v3 field validation).
|
|
||
| export function validatePaginationQuery(query: PaginationQuery): void { | ||
| const { limit, skip, sortOrder } = query | ||
| if (limit !== undefined && (typeof limit !== 'number' || limit < 0)) { |
There was a problem hiding this comment.
[nit] The shared limit check rejects only limit < 0, so 0 is accepted, but the error message still says "must be a positive number". The same wording existed before; consolidating it makes the inaccurate message the single source of truth.
Suggestion: Change the message to "non-negative number" (or reject limit === 0 if positive is truly required). Align the skip message wording the same way.
| return previousCoins !== undefined && previousCoins.length > 0 | ||
| } | ||
|
|
||
| function logSHIPSummary(outputsToAdmit: number[], previousCoins: number[]): void { |
There was a problem hiding this comment.
[nit] hasPreviousCoins and the summary logging helpers are duplicated almost identically in SLAPTopicManager.ts. That reintroduces the kind of copy-paste surface this PR otherwise reduces.
Suggestion: Move shared previous-coin / summary helpers next to isAdmissibleDiscoveryOutput (or a small discoveryTopicLogging.ts) and parameterize the emoji/protocol strings.
| @@ -83,19 +90,17 @@ export class Services implements WalletServices { | |||
| } | |||
There was a problem hiding this comment.
[nit] Extracting configureOptionalProviders dropped the useful comment that tstn intentionally has no WhatsOnChain / block-explorer provider and therefore raw-tx / UTXO / status / history lookups have no provider on that chain. The control flow appears preserved (hasWhatsOnChain = this.chain !== 'tstn'), but future maintainers lose the rationale.
Suggestion: Restore a short comment on hasWhatsOnChain or above initializeReadServices documenting the tstn provider gap.
2 participants
Summary
Advances tracker #324 with a broad production-only complexity and validation wave spanning SDK, Overlay Topics, Overlay Discovery, and Wallet Toolbox.
Mapinstead of prototype-bearing objectsThis is source-only maintenance. It does not publish packages, create tags/releases, or change deployment behavior. Deployable infrastructure remains pinned to currently published package versions.
Validation
Local validation:
pnpm install --frozen-lockfilepnpm audit:security(0 known vulnerabilities)pnpm health:check(64/64; 37 projects, 30 public packages, 0 contract findings, 0 control errors)pnpm check-versionspnpm codegen:checkpnpm docs:examplespnpm license:pack-checkpack:checkpnpm test:governancepnpm conformancegit diff --checkHosted validation on exact head
593d7059b6eda6487d027664ca1dcbba2780682b:Boundaries
The deferred QA/fuzz/manual/browser/performance program remains deferred to the final phase tracked in #324. Sonar administrative configuration is unchanged. Public-service CORS/CSP defaults are unchanged.