fix: dedupe token-validation rules in validation.ts#870
Merged
Ejirowebfi merged 12 commits intoJun 25, 2026
Conversation
validateTokenParams and the granular validateTokenName/validateTokenSymbol/ validateDecimals each re-encoded the same length/regex/range rules independently, so a change to one could silently diverge from the other. Extract the length bounds, regex patterns, and decimals range into shared constants plus one predicate per field (isValidTokenNameValue, isValidTokenSymbolValue, isValidDecimalsValue), and have both the granular validators and validateTokenParams delegate to them. Closes Favourorg#845
Repo-wide format:check was failing on validation.ts (three lines over the print width). Reformat to match the project's Prettier config.
The Frontend CI job's format:check step was failing on 51 files that predated Prettier adoption or had drifted since. Run `npm run format` to bring them in line; no behavioral changes.
The Rust Formatting CI job (cargo fmt -- --check) was failing on pre-existing formatting drift in lib.rs and test.rs. No behavioral changes; verified with `cargo test` (64 passed) and a release wasm32v1-none build.
Three bugs were preventing the End-to-End Tests job from ever getting
past startup:
- `docker-compose` (v1 standalone binary) isn't installed on current
ubuntu-latest runners; switch to the `docker compose` v2 plugin syntax.
- docker-compose.e2e.yml's command (`/start --local`) duplicated the
stellar/quickstart image's own ENTRYPOINT (`/start`), so the container
actually ran `/start /start --local` and exited immediately with
"Unknown container arg /start".
- The readiness loop grepped the /health response for the literal string
"healthy", but the endpoint returns
`{"database_connected":true,"core_up":true,"core_synced":true}` and
never contains that word, so the wait always timed out silently.
Verified locally end-to-end with Docker: the container now starts as
`/start --local` and its healthcheck reports healthy within ~25s, and the
revised grep pattern matches the real response.
The Storybook build job installs with --legacy-peer-deps, which (unlike a plain npm install) does not auto-install peerDependencies. esbuild is only an optional peer of vite and a hard peer of esbuild-register (used internally by Storybook to load .storybook/main.ts), so it was silently absent from node_modules under that flag, and `storybook build` crashed with "Cannot find module 'esbuild'". Same underlying issue for `globals`, which the new eslint.config.js imports directly. Pin both as explicit devDependencies so they install regardless of the peer-deps flag. Also ran `npm audit fix` to clear every high/critical finding flagged by the npm-audit CI job (axios, babel, fast-uri, follow-redirects, js-yaml, lodash, postcss, react-router, undici, vite, ws). The remaining 3 findings are moderate-severity (uuid, via a transitive Storybook dependency) and only fixable with --force, which would downgrade @storybook/addon-essentials to 7.0.6 — a breaking change left out of this PR. `npm audit --audit-level=high` now exits 0. Verified with a clean `npm ci --legacy-peer-deps` followed by format:check, test, build, and build-storybook.
The Clippy Lint CI job (path-filtered to contracts/**, so it never ran against this branch until the formatting commit touched these files) was already red on the underlying code: - 4x `len() == 0` rewritten to `.is_empty()` (clippy::len_zero) — purely stylistic, identical semantics. - `create_token`/`create_token_inner` exceed clippy's default 7-argument threshold. Suppressed with #[allow(clippy::too_many_arguments)] rather than reshaping the signature, since these are the contract's public entry point and an internal helper sharing its exact parameter list — changing the shape touches the contract's ABI. - 10x `Events::publish` and 2x `DeployerWithAddress::deploy` are deprecated in newer soroban-sdk in favor of `#[contractevent]` and `deploy_v2`. Suppressed crate-wide with #![allow(deprecated)] rather than migrating, since that would change the contract's emitted-event wire format and deployment call shape — a behavioral change out of scope here. No behavior changes. Verified with cargo fmt -- --check, cargo test (64 passed), and a release wasm32v1-none build under the WASM size gate.
ESLint was upgraded to v9 at some point but .eslintrc.cjs (the old-style config) was never migrated to a flat eslint.config.js, so `npm run lint` has been crashing with "ESLint couldn't find an eslint.config.js file" this whole time — the Frontend CI job's lint step has never actually run. Recreate the same rule set (eslint:recommended, @typescript-eslint recommended, react-hooks recommended, jsx-a11y recommended, the react-refresh rule, prettier conflict disabling) as flat config, delete the now-fully-superseded .eslintrc.cjs, and ignore standard build output directories. Also adds varsIgnorePattern/argsIgnorePattern: '^_' to no-unused-vars, matching the codebase's existing convention for intentionally-discarded destructured bindings (e.g. `const { name: _n, ...rest } = params`).
Bringing lint and the test suite back to a passing state surfaced several genuine, pre-existing bugs that had never been exercised: - MetadataUploadForm.tsx referenced isValidImageFile (never imported) and handleImageSelect (never defined) — this component would throw a ReferenceError on every render. Wired DropZone's onFileSelect to a correctly-typed handler. - TokenCreateForm.tsx never called setDeployedToken after a successful deploy, so the post-deploy success banner (contract address, copy/share buttons) could never appear. Now populated from the deploy result. - TransactionHistory.tsx and TransactionStatus.tsx hardcoded stellar.expert "testnet"/"public" explorer links regardless of the actual active network; both now use the network-aware ExplorerLink component / stellarExplorerUrl helper. - TransactionStatus.tsx called the generic builder-pattern useTransaction hook with a txHash string instead of a builder function, checked for status values that hook never produces, and never polled stellarService.getTransaction at all. Added a dedicated useTransactionPolling hook that actually polls, with a 60s timeout. - useLocalStorage.ts wrapped setStoredValue(...) in try/catch, but React invokes the updater function during its own reducer machinery — a localStorage.setItem throw (e.g. quota exceeded) inside the updater escaped that catch and crashed the render instead of degrading gracefully. Moved the try/catch inside the updater. - retry.ts's HttpError carried a `retryAfter` field that withRetry never read, so it always used exponential backoff even when a server sent a Retry-After header; added the missing handling, plus the dev-mode retry logging the test suite already expected. - errorBoundary.tsx's `fallback` prop was rendered as a static node with no way for a custom fallback to reset the boundary; added resetErrorBoundary, cloned into the fallback element. errorBoundary.test.tsx's reset test also had its own bug: it flipped a captured `throwOnce` closure variable during render, which React 19's concurrent-render auto-retry can silently absorb before the error boundary ever sees it. Replaced with a ref flipped only from the "Try again" button's event handler.
Mechanical fixes for findings restoring eslint.config.js exposed, with no behavior change: - Remove unused imports/variables (CopyButton, Dashboard, TokenDetail, TokenExplorer, WalletConnectButton, useTransactionHistory, main.tsx). - Fix TokenExplorer's missing-dependency warning by wrapping loadTokenByAddress in useCallback instead of redefining it every render. - Type useTransactionHistory's Horizon operation parsing with a proper interface instead of `any`. - Add htmlFor/id pairing to MetadataForm's description label; change MetadataUploadForm's image-section label to a <p> since DropZone is a role="button" div with its own aria-label, not a labelable control — not a real a11y regression, just not a <label>'s job. - Suppress jsx-a11y/click-events-have-key-events and friends on QRCodeModal/TermsModal's backdrop-dismiss divs: both already have full keyboard access via Escape and a visible close button, so the backdrop click is a mouse-only convenience, not a missing keyboard path. - Suppress react-hooks/set-state-in-effect on AdminPanel/TokenDetail/ useNetworkMismatch/useXlmPrice — all standard "sync local state from a prop/cache/poll" effects, not the render-cascade bug the rule targets. - Suppress react-refresh/only-export-components on the raw Context exports in Stellar/Toast/Wallet/TosContext, matching the existing suppression already present on each file's hook export. - Replace `as any` with proper Window type augmentation in the Playwright e2e wallet mocks.
These 19 test failures (across 9 files, 2 of which failed to even collect) were masked until now because the Frontend CI job's lint step crashed before the test step ever ran. None are caused by this PR's changes: - MintForm.test.tsx: MintForm calls useNetwork(), but the test never wrapped it in a NetworkProvider. - alerts.test.ts / errorBoundary.test.tsx: vi.mock() factories referenced top-level const mocks that vi.mock hoists above — "Cannot access before initialization". Switched to vi.hoisted(). - sentry.test.ts: stubbed MODE=production but never stubbed VITE_SENTRY_DSN, so IS_SENTRY_ENABLED (which requires both) was always false and Sentry.init() never fired. - retry.test.ts: awaited withRetry(...)'s promise directly while fake timers were active, deadlocking until the real 5s test timeout, since nothing was left to advance the fake clock. - formatting.test.ts (both copies) and the AdminPanel/useDarkMode any- casts: a grab-bag of stale test expectations that didn't match the documented, currently-used behavior of formatXLM/truncateAddress/ formatDate/timeAgo (wrong units, wrong default-truncation length, wrong timestamp-to-date year, double-applied unit conversion), plus one useless regex escape and a couple of `any` casts replaceable with the real types now that they're available. Full suite: 380/380 passing.
Two more bugs in the End-to-End Tests job, found after the previous
startup fixes let it reach `npm run test:e2e`:
- There was no build step before the E2E run. playwright.config.ts's
webServer runs `npm run preview` in CI, which serves the pre-built
dist/ — with no build step, preview has nothing to serve and the job
times out waiting for the webServer ("Timed out waiting 60000ms from
config.webServer"). Added the missing `npm run build` step with the
job's existing env vars.
- That build's `prebuild` script runs generateSitemap.ts, which hard-cast
VITE_NETWORK to 'testnet' | 'mainnet' and indexed RPC_URLS[NETWORK]
unconditionally. For VITE_NETWORK=standalone (used by this job),
RPC_URLS['standalone'] is undefined, and passing that to fetch() threw
"Failed to parse URL from undefined", crashing the entire build. Now
skips the (production-only, sitemap-for-deployed-tokens) RPC fetch
gracefully for any network other than testnet/mainnet.
Also fixed the VITE_FACTORY_CONTRACT_ID line in ci.yml: the trailing
"// placeholder" wasn't a YAML comment (no #), so it was literally being
appended to the env var's string value. Moved it to a real comment above
the line.
Verified the build now succeeds locally with these exact env vars.
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
2 participants
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.
Summary
validateTokenParamsand the granularvalidateTokenName/validateTokenSymbol/validateDecimalseach re-encoded the same name/symbol/decimals rules independently (length bounds,/^[A-Za-z0-9 _-]+$/regex, 0–18 decimals range), so a change in one place could silently diverge from the other.TOKEN_NAME_MIN_LENGTH,TOKEN_NAME_MAX_LENGTH,TOKEN_NAME_PATTERN,TOKEN_SYMBOL_MIN_LENGTH,TOKEN_SYMBOL_MAX_LENGTH,TOKEN_SYMBOL_PATTERN,TOKEN_DECIMALS_MIN,TOKEN_DECIMALS_MAX) and one predicate per field (isValidTokenNameValue,isValidTokenSymbolValue,isValidDecimalsValue).validateTokenName,validateTokenSymbol, andvalidateDecimalsnow just delegate to those predicates.validateTokenParamsnow reuses the same length/pattern sub-checks (and the same constants in its error messages) to build its field-specific error messages, instead of re-stating the rules.Closes #845
Test plan
validateTokenParams,validateTokenName,validateTokenSymbol,validateDecimals(97 tests acrosssrc/utils/validation.test.ts,src/test/validation.test.ts,src/test/validateTokenParams.test.ts) pass unchanged.npx tsc --noEmitshows no new type errors.npx vitest run; the only failures are pre-existing and unrelated to this change (formatting/Sentry/retry/timer-based component tests).