introduce utility that settles async functions to multiple#163
introduce utility that settles async functions to multiple#163chimalsky wants to merge 2 commits into
Conversation
networks and emits diagnostic messages to client. converted - getMarkets - getUserBalances - getUserPositions
lyoungblood
left a comment
There was a problem hiding this comment.
Code Review
Summary
Introduces a settleAcrossEnvironments utility in src/common/diagnostics.ts that replaces raw Promise.allSettled patterns across three actions (getMarkets, getUserBalances, getUserPositions). Collects diagnostics on RPC failures and invokes an optional onDiagnostics callback on the MoonwellClient.
Issues
🔴 Critical — Debug console.log statements left in production code
src/common/diagnostics.ts:console.log("settled data", s)andconsole.log("diagnostics", ...)src/actions/core/getUserBalances.ts:console.log("result", result, environmentsTokensBalances)
These dump potentially large data structures on every SDK call.
Remove all console.log statements from src/common/diagnostics.ts and
src/actions/core/getUserBalances.ts.
🟡 Major — Array.map leaks index/array arguments into run callback
src/common/diagnostics.ts: environments.map(run) passes (element, index, array) to run. The ...args: unknown[] rest parameter silently absorbs these.
In src/common/diagnostics.ts, change the run signature to
(env: Environment) => Promise<T> and use
environments.map((env) => run(env)).
🟡 Major — WithDiagnosticsHook added to getMarkets args type but unused
src/actions/core/markets/getMarkets.ts: The type extension is dead code — the hook lives on the client. The other two converted actions don't have it.
In src/actions/core/markets/getMarkets.ts, remove the WithDiagnosticsHook
intersection from the args type and its import.
🟡 Major — Verify onDiagnostics is in MoonwellClient type definition
In src/client/createMoonwellClient.ts, confirm onDiagnostics is in the
MoonwellClient type definition, not just the runtime object.
🔵 Minor — error field typed as string but s.reason is unknown
Will silently store [object Object] when rejection reason isn't a string.
In src/common/diagnostics.ts, convert s.reason explicitly:
error: s.reason instanceof Error ? s.reason.message : String(s.reason)
🔵 Minor — Unnecessary .flatMap identity after removing console.log
Simplify to:
const data = settled.flatMap((s) => s.status === "fulfilled" ? [s.value] : []);
🔵 Minor — supportedChainsIds lookup not null-checked
Positive observations
- Client-level diagnostic hook is a sound design — reduces API surface.
- Centralizes a duplicated
Promise.allSettledpattern (good DRY improvement). - Removing try/catch-swallow-and-return-empty-array is the right call.
Suggestions
- Add unit tests for
settleAcrossEnvironmentscovering all-succeed, partial-failure, and total-failure cases. - Replace
console.warnfallback with the project's logger module or remove it entirely.
context
#154 introduced code that converted most of the
actionsin this sdk to not crash when one/more RPC node(s) were down. However the frontend had no way of knowing which network(s), and which actions were failing.solution
This PR introduces a way for an
actiontoemita diagnostic via a diagnosticHook that the clients can define.Since the client defines the diagnosticHook callback function... and since this callback is an optional param... no frontend clients need to be rewritten to prevent breakage. Frontend clients instead can opt-in to define the callback handlers to improve UX progressively as needed.
design decision
I chose to make diagnosticHook defined at the global client level rather than per
actionto reduce code changes and breakage potential. The actions in this sdk can be changed progressively rather than in one giant PR.notes
converted actions in this PR:
Example of what the frontend might look like when using this dianostic emit utility
example code:
Screen.Recording.2025-09-22.at.7.18.15.PM.mov