Skip to content

perf(sdk): memoize history selectors and amount conversion factors (#32) - #413

Open
RaymondAbiola wants to merge 1 commit into
conduit-protocol:mainfrom
RaymondAbiola:detailed-sdk-feature
Open

perf(sdk): memoize history selectors and amount conversion factors (#32)#413
RaymondAbiola wants to merge 1 commit into
conduit-protocol:mainfrom
RaymondAbiola:detailed-sdk-feature

Conversation

@RaymondAbiola

Copy link
Copy Markdown

Closes #366

@drips-wave

drips-wave Bot commented Jul 30, 2026

Copy link
Copy Markdown

@RaymondAbiola Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits.

You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀

Learn more about application limits

@Jaydbrown Jaydbrown left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not mergeable as-is — two real problems, both blocking:

  1. Removes strictness flags from all three tsconfig files repo-wide (tsconfig.json, tsconfig.rollup.json, packages/react/tsconfig.json):

    -    "noImplicitAny":           true,
    -    "useUnknownInCatchVariables": true,

    This is a real regression in the codebase's type-safety guarantees, applying to every file in the SDK and the React package, not just the selector-memoization work this PR is about. This almost certainly exists to let the new code type-check without properly typing something (implicit anys or untyped catch bindings) rather than being an intentional, discussed policy change. Please restore both flags and fix whatever type errors that surfaces in the new code instead.

  2. This branch is stale and would silently revert an already-merged fix. src/dashboard/transaction-history.ts's toIso8601/SPACE_SEPARATED_DATETIME helper (added by PR #406, "normalize space-separated indexer timestamps to ISO 8601 for Safari") is removed by this diff, along with its regression test in src/tests/transaction-history-crash.test.ts. GitHub shows this as a clean, conflict-free diff since the textual regions don't overlap — but merging as-is would reintroduce the Safari timestamp bug #406 just fixed. Please rebase onto current main and drop this reversion; the actual memoization work in transaction-history.ts's selectors doesn't need to touch asTimestamp/toIso8601 at all.

The core selector-memoization approach and the utils.ts amount-conversion caching look reasonable on a skim, but I'd like to see this rebased and with the tsconfig changes reverted before a full re-review.

@RaymondAbiola

Copy link
Copy Markdown
Author

Ok thanks for your review. I will fix it.

@RaymondAbiola
RaymondAbiola force-pushed the detailed-sdk-feature branch from 7af0584 to 14140bf Compare July 30, 2026 15:16
@RaymondAbiola

Copy link
Copy Markdown
Author

@Jaydbrown Thanks for the review — rebased onto current main, both blockers are resolved.

On the tsconfig flags: the branch never actually touched any tsconfig file. noImplicitAny and useUnknownInCatchVariables were added to main after this branch forked (base was faefc1b, main is now 944baf3), so the diff rendered them as removals against a stale base. Same root cause as the toIso8601 reversion you spotted — the rebase fixed both. All three tsconfigs and the #406 Safari timestamp fix now show zero diff against main.

On the rest: the rebase showed the optimizations here had already landed:

I resolved every conflict in favor of main and dropped my implementation. The WeakMap + O(n log k) top-k approach in #408 is better than what I had, and keeping mine would have been a regression. The docs changes were redundant with the "Selector memoisation" section already there.

I also verified the signature string can't collide (the first three positions are enum-constrained and space-free) and pinned that down with a test, since a search term containing spaces is the obvious way it could break.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enhancement: Detailed SDK Feature #32

2 participants