fix: StrKey checksum validation and screen-visibility-gated polling - #636
Open
omarima-10 wants to merge 2 commits into
Open
fix: StrKey checksum validation and screen-visibility-gated polling#636omarima-10 wants to merge 2 commits into
omarima-10 wants to merge 2 commits into
Conversation
TransactionPanel already had inline format validation and a balance
check (validateStellarAddress + insufficientBalance in
TransactionPanel.tsx were already on main) - the one gap left was that
validateStellarAddress only checked the surface format
(/^G[A-Z2-7]{55}$/), not the actual StrKey checksum. A single mistyped
character in an otherwise well-formed address passes that regex and
would still reach the network, failing with an opaque error instead
of a clear one at input time - the exact problem Sorokit#535 describes.
Implemented the real StrKey decode (base32 -> version byte -> 32-byte
key -> CRC16/XMODEM checksum) as a small pure function rather than
adding @stellar/stellar-sdk as a dependency - this package is
"strictly presentation layer with no blockchain logic inside" per its
own README, and checksum verification is data validation, not chain
interaction, so it stays in scope for that constraint.
Verified the algorithm against a real, publicly known Stellar address
(not a synthetic fixture) before trusting it, and confirmed it
correctly rejects: a single mutated character, a different StrKey
type that also base32-encodes to a 'G' prefix (ED25519_SIGNED_PAYLOAD,
version byte 49 - only version 48/0x30 is ED25519_PUBLIC_KEY),
lowercase input, and out-of-alphabet characters.
14 new tests in utils.test.ts.
…t#533) Dashboard's original bug - all 5 screens pre-mounted at module-eval time via a static SCREENS constant - is already fixed on main, but via a different, deliberate design than the issue describes: screens are lazy-loaded and only mount once actually visited, then stay mounted-but-hidden (not unmounted) on navigating away, to preserve in-progress state like a half-typed Soroban form (see the comment above the `visited` Set in Dashboard.tsx). That's a real, intentional UX improvement over unmount-on-navigate - but it means a screen's useEffect-based polling (FeeEstimator's refreshInterval, ContractEventFeed's pollInterval and its "Last updated" tick) never gets torn down once the screen has been visited once, since `hidden` is CSS-only and never runs cleanup. That's the actual resource-leak Sorokit#533 is about, just via a different mechanism than "all mounted at once." Rather than reverting the mount-once design (which would regress the state-preservation improvement) or threading an `isActive` prop through Dashboard -> every screen -> every component that polls (a public API change), added useIsVisible(): an IntersectionObserver- backed hook that reports whether a component's own DOM node currently has a laid-out box, independent of any prop plumbing. `hidden` sets display:none with no CSS override in this codebase, so a hidden screen's IntersectionObserver entries report non-intersecting immediately - no scrolling involved, this is layout-driven. FeeEstimator and ContractEventFeed both gate their setInterval calls on isVisible in addition to their existing on/off controls (refreshInterval > 0, the Live/Paused toggle) - polling now pauses the instant a screen is hidden and resumes the instant it's shown again, with zero change to either component's public props. 7 new tests: 6 in useIsVisible.test.ts (own suite, mocked IntersectionObserver, including the fail-open-when-unavailable case) plus one pause/resume regression test in each of FeeEstimator.test.tsx and ContractEventFeed.test.tsx.
|
@omarima-10 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! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #535
closes #533
#535 — TransactionPanel address/balance validation
Both of the issue's originally-described gaps — no format validation, no balance check — were already fixed on
mainby the time I picked this up:TransactionPanel.tsxalready hasvalidateStellarAddress(dest)gating the Send button with inline errors, and aninsufficientBalancecheck comparing against the connected wallet's actual XLM balance (minus the 1 XLM reserve).The one real gap left:
validateStellarAddressonly checked the surface format (/^G[A-Z2-7]{55}$/), not the actual StrKey checksum — exactly the caveat the issue's own "Solution" section calls out (StrKey.isValidEd25519PublicKeydoes real verification; a bare regex doesn't). A single mistyped character in an otherwise well-formed address passes that regex and would still reach the network, failing with an opaque error instead of a clear one at input time.Implemented the actual StrKey decode (base32 → version byte → 32-byte key → CRC16/XMODEM checksum) as a small pure function rather than pulling in
@stellar/stellar-sdk— this package is "strictly presentation layer with no blockchain logic inside" per its own README, and checksum verification is data validation, not chain interaction, so it stays in scope for that constraint without adding a dependency.Verified the algorithm against a real, publicly known Stellar address (not a synthetic fixture) before trusting it — confirmed it correctly accepts the real address and rejects a single mutated character. Also confirmed and tested a subtler case: StrKey version bytes 48–55 all base32-encode to a
Gprefix, but only 48 (0x30) isED25519_PUBLIC_KEY— the other 7 are other valid StrKey types (e.g.ED25519_SIGNED_PAYLOAD) that a format-only regex cannot distinguish from a payable account address.14 new tests in
utils.test.ts.#533 — Dashboard screen mounting
The original bug (all 5 screens pre-mounted at module-eval time via a static
SCREENSconstant) is already fixed onmain, but via a different, deliberate design than the issue describes: screens are lazy-loaded and only mount once actually visited, then stay mounted-but-hidden (not unmounted) when navigating away — to preserve in-progress state, like a half-typed Soroban form (see the comment above thevisitedSet inDashboard.tsx).That's a real, intentional UX improvement over strict unmount-on-navigate — but it means a screen's polling (
FeeEstimator'srefreshInterval,ContractEventFeed'spollIntervaland its "Last updated" tick) never gets torn down once a screen has been visited once, since thehiddenattribute is CSS-only and never runs auseEffectcleanup. That's the actual resource-leak concern #533 raises, just via a different mechanism than "all mounted at once."Rather than reverting the mount-once design (which would regress the state-preservation improvement it was built for) or threading an
isActiveprop throughDashboard→ every screen → every component that polls (a public API change across many files), I addeduseIsVisible(): anIntersectionObserver-backed hook that reports whether a component's own DOM node currently has a laid-out box — independent of any prop plumbing.hiddensetsdisplay: nonewith no CSS override anywhere in this codebase, so a hidden screen'sIntersectionObserverentries report non-intersecting immediately (no scrolling involved — this is layout-driven, not viewport-driven).FeeEstimatorandContractEventFeedboth gate theirsetIntervalcalls onisVisible, in addition to their existing on/off controls (refreshInterval > 0, the Live/Paused toggle) — polling now pauses the instant a screen is hidden and resumes the instant it's shown again, with zero change to either component's public props.7 new tests: 6 in
useIsVisible.test.ts(own suite, mockedIntersectionObserver, including the fail-open-when-unavailable case) plus one pause/resume regression test in each ofFeeEstimator.test.tsxandContractEventFeed.test.tsx.I want to flag one thing before merge
The Dashboard/polling fix (#533) is a genuinely different approach than the issue's suggested solution (which was to unmount inactive screens). I think pausing polling while preserving mount-once is the better fix given the deliberate state-preservation design already on
main, but it's a judgment call about priorities (memory/interval hygiene vs. UX continuity) rather than a straightforward bug fix — happy to discuss if a maintainer prefers a different tradeoff.Test plan
npx tsc --noEmit— clean, zero errors.npx eslinton all 8 changed/new files — clean.npx vitest run src/lib/utils.test.ts— 23/23 passing.npx vitest run src/components/FeeEstimator.test.tsx— 19/19 passing.npx vitest run src/components/ContractEventFeed.test.tsx— 34/37 passing; the 3 failures (JSON export tests, issue test: BalanceList search filter and sort modes, TransactionHistory status filter API params, ContractEventFeed export and topic copy #352) are pre-existing — confirmed identical viagit stashagainst unmodifiedmain, unrelated to this PR's changes.npx vitest run src/hooks/useIsVisible.test.ts— 6/6 passing (new suite).npx vitest runfor a final sanity pass; this machine has other unrelated background work competing for resources right now and the full run (1270 tests) didn't complete in a reasonable time even after ~25 minutes across several attempts. Given every individual test file this PR actually touches was independently verified clean above, I didn't think it was worth blocking further on the full-suite run rather than disclosing this plainly. Happy to re-run and report back if useful.