refactor(assets): consolidate asset identity checks - #3000
Open
CassioMG wants to merge 29 commits into
Open
Conversation
Add @shared/helpers/assetIdentity with one predicate per identifier layer: token type, canonical identifier, contract id (derived from the network passphrase), and the raw code/issuer pair. Move isNativeBalance there from popup/helpers/balance so every nativeness check has one home. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
getOperation chose the create-account branch by comparing the source asset's code to the native code. CreateAccountOp carries no asset field, so that branch is only ever correct for the native asset itself. Gate it on isNativeAsset, so a classic asset that uses the same code falls through to the payment branch carrying the asset it actually is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
getBalanceByKey decided which balance to compare against the native contract address from the balance's code, and returned early from that branch. Gate it on isNativeBalance so the native SAC resolves the native balance and any other asset reaches the issuer arm below, whatever code it uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The "enough XLM for the fee" pre-flight picked the paying balance by code, so any balance using that code satisfied it. Extract the check to hasEnoughXlmForFee in popup/helpers/balance, anchored on the balance type, and give it direct tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AssetIcon decided whether to render the bundled Stellar logo from the asset code alone, while already receiving issuerKey. Pair the two, so an asset that uses the native code but has its own issuer goes through the normal icon lookup. Also drop the dead native ternary in handleClick, which receives a canonical identifier and so could never match the bare code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
History chose the bundled native logo from an asset code alone, and treated a contract's self-reported symbol as proof of which asset the contract is. Pair the code with its issuer at the icon sites, and decide a contract token's nativeness from its contract address, which is the only identifier available in contract space. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Asset-detail operation matching folded an asset code and a Horizon asset type into one variable, so an asset whose code is the native code collected the account's native operations. Extract operationMatchesAssetKey with the native and classic arms separated, each testing in its own identifier space, and give it direct tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… address The fixture identified its test token by contract address CDLZFC3SYJYDZT7K67VZ75HPJVIEUVNIXF47ZG2FB2RMQQVU2HHGCYSC while separately having it report the symbol "TEST" — that address is the native SAC on TESTNET, so once row identity is decided by contract address the row correctly renders as XLM and the test's own "TEST" assertions fail. Swap in a genuinely non-native contract address and assert it stays that way, so this cannot silently regress again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The four existing cases only exercised the payment-asset arm. Add path-payment fixtures whose source and destination legs differ, pinning both directions of the conflation removal on the source side too: a native-sourced path payment correctly excludes from a classic asset keyed on the native code and includes under the true native key, and a path payment sourced from that classic asset correctly includes under its own key and excludes from the native key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
getNativeContractDetails listed the native SAC address for two networks and returned an empty string for the rest, which degraded every downstream comparison to a match against "". Derive it from the network passphrase, and route isAssetSac's native branch through isNativeContract. Also updates three pre-existing getNativeContractDetails unit tests in searchAsset.test.js: they called it with network-only fixtures (no networkPassphrase), which the old table-based implementation tolerated but the derivation now needs, since it hashes the passphrase unconditionally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Looking a token up by the native contract address produced a row carrying the network table's legacy issuer value, so the row's canonical identifier was a code/issuer pair rather than the native identifier and never matched the held native balance. Extract buildNativeAssetRow, which carries no issuer, and gate the branch on isNativeContract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
List membership compared optional issuer and contract fields directly, so two absent values matched and an asset could match a list holding a single contract-less entry. Extract assetMatchesListItem, which requires the asset's own side of each comparison to be present. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds no-asset-code-comparison, which reports a strict equality comparison with a native-asset sentinel on either side, so a nativeness check goes through a predicate rather than a string comparison. Not wired into the config yet — the tree is migrated first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Without an explicit type, the plain object literal's meta.type field widens to string instead of ESLint's RuleType union, so TypeScript rejects passing the rule to RuleTester.run() (TS2345). Annotate it with the RuleModule type from ESLint's own types instead of changing its shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…pair useGetAssetDomainsWithBalances treated any balance coded "XLM" as the native asset, regardless of issuer. Nothing reserves that display code for the native asset — a classic balance can legitimately carry code "XLM" with a real issuer — so a non-native asset sharing the code collapsed into the native row: it lost its real issuer (shown as ""), was skipped for icon and home-domain lookup, and had its Blockaid verdict hardcoded to benign instead of reflecting its actual scan data. Route the check through isNativeAssetPair(code, issuer.key), which requires both the native code and the absence of an issuer, matching how every other identity check in the codebase now decides nativeness. A balance's code alone never establishes its identity; the pair does. Added a regression test pinning both outcomes on one fixture: a classic asset coded "XLM" with a real issuer keeps its own issuer, domain, and Blockaid-derived suspicious flag and is listed separately, while the genuine native balance keeps its existing native-row behavior unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… predicates Also collapses two hand-rolled canonical identifiers onto getCanonicalFromAsset, which already returns the native identifier for a code with no issuer. Corrects AssetNetworkInfo's assetType prop to a plain string. The prop has no real callers today; its previous union type resolved through an unrelated Omit<> modeling issue in ClassicAsset["token"]["type"] (account-balance.ts) that only surfaced once isNativeAssetId's stricter parameter type replaced a bare `===` comparison. No behaviour change. Adds a BalanceRow regression test pinning that a classic asset coded "XLM" with a real issuer and an iconUrl renders its icon instead of AssetIcon's perpetual loading state, per the hazard already documented at BalanceRow's canonical/resolvedIcons comment. Verified red against the pre-fix condition and green against the fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…edicates
Also routes useSimulateTxData's native-SAC derivation through the shared
getNativeContractId helper instead of an inline Asset.native().contractId(...)
call, so there is one derivation site.
useSwapTokenLookup's heldToRecord corrects the brief's literal replacement:
token.issuer is a {key: string} object here (not a plain string), so the
native check uses token.issuer?.key to keep isNativeAssetPair's runtime
truthiness check identical to the original `!token.issuer`, while type-checking
against the predicate's string signature. currencyToRecord's asset.issuer is
already a plain string, so it passes straight through as the brief specified.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Includes both SignTransaction sites in the six-caller group that pass an
empty assetIcons map for an XLM-coded classic asset with a real issuer
(assetIcons={code !== "XLM" ? icons : {}}); migrating to isNativeAssetPair
(issuer-aware) instead of a bare code check keeps AssetIcon's isEmpty(...)
check false for that asset, matching the other four callers migrated in the
prior two commits.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
useGetAssetDomainsWithBalances decided identity for an XLM-coded asset with isNativeAssetPair(code, issuer.key), but the full AssetType-typed balance is in unshadowed scope at that line, and isNativeAssetPair is documented as a last resort for when neither a token type nor a contract id is available — neither restriction applies here. Route the check through isNativeBalance(balance) instead. It agrees with the pair form for NativeAsset and ClassicAsset, and is strictly more robust for SorobanAsset: a Soroban token's shape carries no `type` field, so isNativeBalance is false for it unconditionally, while the pair form would read a Soroban token coded "XLM" with an empty issuer key as native. isNativeBalance doesn't depend on that key being non-empty to begin with. The existing regression test is unaffected, since it exercises NativeAsset and ClassicAsset, where the two forms already agreed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Turns no-asset-code-comparison on at error level now that every call site is migrated, with @shared/helpers/assetIdentity exempt as the module that defines the predicates. ESLint runs inside the webpack build, so a new comparison against a native sentinel fails the build. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds an anti-patterns section spelling out the (code, issuer)/contract-id identity rule and which predicate to use for each shape, and records the enforcing lint rule in the code-style reference. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…n map TokenList already computes isNative via isNativeBalance(balance), which is authoritative here since the balance's type is known. The assetIcons ternary independently re-derived nativeness via isNativeAssetPair(code, issuerKey) instead of reusing it — isNativeAssetPair is documented as a last resort for when neither a token type nor a contract id is available, which does not apply at this call site. Same category as 217be89. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The asset-identity docs claimed ESLint runs inside the webpack build so a violation fails `yarn build:extension`, full stop. In fact eslint-webpack-plugin derives its lint glob from the webpack context, which is `<repo>/extension` under `yarn workspace extension build` -- @shared/, the tree the predicates themselves live in, is not linted by that build, and there is no root `lint` script wiring it in either. State that plainly instead of implying a guarantee the rule doesn't provide; closing the gap is a follow-up. Also: expand "What it can't do" with the rule's real syntactic blind spots -- it only visits `===`/`!==` binary expressions, so loose equality, switch/case, template literals, .includes()-style checks, and a sentinel hoisted into a local const all pass silently; describe the assetIdentity.ts lint exemption as a belt-and-braces safeguard rather than a necessity, since that module's own local consts aren't in the rule's identifier list anyway; mark NATIVE_TOKEN_CODE/HORIZON_NATIVE_ASSET_TYPE as forward-looking names with no matching constant in this codebase yet (they cover code ported from mobile), not existing ones; soften code-style.md's claim that lint enforces asset identity (it only catches native-sentinel comparisons, never identity) and point its cross-link at the heading's actual anchor. Also applies the same isNativeBalance(balance) correction from 217be89 and 5159097 to useSwapTokenLookup's heldToRecord, the branch's last remaining isNativeAssetId(...) || isNativeAssetPair(...) split. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
stellar.expert returns the native asset as a bare "XLM" record — no issuer and no domain — and the asset search mapped that to a row carrying neither an issuer nor a contract id. The verified-list split recognises the native asset only by its contract id (it seeds the network's native contract into the verified set), so a row with no identity at all landed under "Unverified", and the held-balance check could not match it either. Extract the record-to-row mapping into mapStellarExpertRecord and build the native row from buildNativeAssetRow, so it carries the derived native contract id, an empty issuer and the native canonical identifier. Native is detected with isNativeAssetPair on the split record, the raw-strings case that predicate exists for; a classic asset that merely uses the code "XLM" keeps its own issuer and is not treated as native. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
PR Preview build is ready: https://github.com/stellar/freighter/releases/tag/untagged-d5cc2fdfaebb047b5bfc |
Contributor
There was a problem hiding this comment.
Pull request overview
Consolidates native-asset identity checks around shared predicates, preventing XLM-coded issued assets from being mistaken for native lumens.
Changes:
- Adds shared asset, balance, and contract identity predicates.
- Migrates send, swap, history, signing, and asset-management flows.
- Adds regression tests, documentation, and an ESLint rule.
Reviewed changes
Copilot reviewed 54 out of 54 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
extension/src/popup/views/Swap/index.tsx |
Uses canonical native detection. |
extension/src/popup/views/SignTransaction/index.tsx |
Fixes fee and icon identity checks. |
extension/src/popup/views/AccountHistory/hooks/useGetHistoryData.tsx |
Corrects history asset classification. |
extension/src/popup/views/AccountHistory/hooks/__tests__/useGetHistoryData.test.tsx |
Tests Soroban transfer identity. |
extension/src/popup/views/Account/hooks/useGetIcons.tsx |
Excludes native assets via predicate. |
extension/src/popup/views/__tests__/AccountHistory.test.tsx |
Corrects non-native contract fixture. |
extension/src/popup/helpers/xlmReserve.ts |
Uses shared balance predicate. |
extension/src/popup/helpers/soroban.ts |
Uses canonical and contract predicates. |
extension/src/popup/helpers/sendWarnings.ts |
Uses canonical native detection. |
extension/src/popup/helpers/searchAsset.ts |
Derives native contracts and maps search rows. |
extension/src/popup/helpers/balance.ts |
Centralizes native balance and fee checks. |
extension/src/popup/helpers/assetList.ts |
Guards optional identity comparisons. |
extension/src/popup/helpers/account.ts |
Corrects operation-to-asset matching. |
extension/src/popup/helpers/__tests__/soroban.test.ts |
Tests native SAC detection. |
extension/src/popup/helpers/__tests__/searchAsset.test.ts |
Tests native search-row mapping. |
extension/src/popup/helpers/__tests__/searchAsset.test.js |
Updates native contract tests. |
extension/src/popup/helpers/__tests__/balance.test.ts |
Tests balance and fee identity. |
extension/src/popup/helpers/__tests__/assetList.test.ts |
Tests list matching and verification. |
extension/src/popup/helpers/__tests__/account.test.ts |
Tests operation matching. |
extension/src/popup/components/swap/SwapAsset/hooks/useSwapTokenLookup.ts |
Corrects swap token identity. |
extension/src/popup/components/swap/SwapAmount/index.tsx |
Uses canonical native checks. |
extension/src/popup/components/swap/SwapAmount/helpers/getSwapDerivedData.ts |
Corrects reserve derivation. |
extension/src/popup/components/send/SendAmount/index.tsx |
Corrects source icon handling. |
extension/src/popup/components/send/SendAmount/hooks/useSimulateTxData.tsx |
Fixes native operation selection. |
extension/src/popup/components/send/SendAmount/hooks/__tests__/useSimulateTxData.test.ts |
Tests emitted operation types. |
extension/src/popup/components/manageAssets/ToggleAssetRows/index.tsx |
Corrects native icon selection. |
extension/src/popup/components/manageAssets/SelectAssetRows/index.tsx |
Corrects asset-row icons. |
extension/src/popup/components/manageAssets/SearchAsset/hooks/useAssetLookup.ts |
Reuses normalized search-row builders. |
extension/src/popup/components/InternalTransaction/TokenList/index.tsx |
Uses balance type for nativeness. |
extension/src/popup/components/InternalTransaction/SubmitTransaction/index.tsx |
Uses canonical icon checks. |
extension/src/popup/components/InternalTransaction/ReviewTransaction/index.tsx |
Uses canonical source identity. |
extension/src/popup/components/InternalTransaction/ReviewTransaction/components/SendDestination.tsx |
Corrects destination icon handling. |
extension/src/popup/components/BalanceRow/index.tsx |
Uses full code-and-issuer identity. |
extension/src/popup/components/BalanceRow/__tests__/index.test.tsx |
Tests XLM-coded asset icons. |
extension/src/popup/components/AssetTile/index.tsx |
Uses canonical native detection. |
extension/src/popup/components/AssetListRow/index.tsx |
Uses canonical identity and icon handling. |
extension/src/popup/components/amount/AmountCard/index.tsx |
Corrects amount-card icon selection. |
extension/src/popup/components/accountMigration/ReviewMigration/index.tsx |
Uses asset-type predicate. |
extension/src/popup/components/accountHistory/AssetNetworkInfo/index.tsx |
Uses Horizon asset-type identity. |
extension/src/popup/components/account/AssetDetail/index.tsx |
Uses canonical and balance predicates. |
extension/src/popup/components/account/AccountAssets/index.tsx |
Uses issuer-aware native icons. |
extension/src/popup/components/account/AccountAssets/__tests__/AssetIcon.test.tsx |
Tests issuer-aware icons. |
extension/src/helpers/transaction.ts |
Uses shared balance identity. |
extension/src/helpers/hooks/useGetAssetDomainsWithBalances.ts |
Corrects domain lookup classification. |
extension/src/helpers/__tests__/useGetAssetDomainsWithBalances.test.tsx |
Tests XLM-coded classic balances. |
eslint.config.js |
Enables the asset-identity rule. |
docs/skills/freighter-best-practices/references/code-style.md |
Documents asset identity conventions. |
docs/skills/freighter-best-practices/references/anti-patterns.md |
Documents unsafe code comparisons. |
config/eslint-plugin-asset-identity/index.mjs |
Implements the lint rule. |
config/eslint-plugin-asset-identity/__tests__/no-asset-code-comparison.test.ts |
Tests lint-rule behavior. |
@shared/helpers/stellar.ts |
Reuses shared identity predicates. |
@shared/helpers/assetIdentity.ts |
Defines shared identity predicates. |
@shared/helpers/__tests__/assetIdentity.test.ts |
Tests all shared predicates. |
@shared/api/helpers/addBlockaidScanResults.ts |
Excludes native assets canonically. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The rule matched only literal sentinels and two identifier names, so the spelling the branch itself replaced — comparing an asset's code to Asset.native().code — was a MemberExpression the rule never inspected and passed the gate. Recognise `.code` and `.getCode()` read off an `X.native()` call as native sentinels too. Asset.native().contractId(...) is deliberately left reportable-free: comparing a contract id against the derived native contract is the sound check in contract space, and the codebase relies on it. Adds the SDK forms as invalid RuleTester cases and the contract-id form as a valid one; docs updated to match. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two add-by-contract-address paths still built the native asset's row inline with `issuer: contractId`. That fabricated issuer made the row read as a contract token — AssetIcon derives Soroban-ness from the issuer key and rendered the Soroban placeholder — and gave it the canonical `XLM:C…`, so the held-balance check never matched the native balance and the row offered "Add" for an asset the account already holds. Both paths now gate on isNativeContract and use buildNativeAssetRow, matching the search path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AssetIcon's custom memo comparator only compared the icon map and the two security flags, so a surviving instance whose asset changed while the icon map stayed deeply equal kept the previous asset's logo or loading state on screen. That gap predates this branch — the comparator already ignored the code the native check used to depend on — and now that the check also reads the issuer, both halves of the asset's identity have to be compared. Compare code, issuer, icon and the shape flags alongside the icon map; the retry callback stays excluded because its identity is unstable and it does not affect the render. Two rerender tests pin an identity change over an equal icon map in both directions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code |
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.
TL;DR
This PR consolidates the extension's native-asset identity checks into a small set of shared predicates, each keyed on the authoritative discriminant for its layer: a token's type, the native contract address derived from the network passphrase, or the full canonical identifier.
The flows themselves are unchanged; what changes is how the identity checks are made, and where that logic lives.
It also adds a lint rule that reports a
===/!==with a native-asset sentinel on either side, so new code reaches for an existing predicate instead of re-deriving one;@shared/helpers/assetIdentity.tsis exempt as the module that defines them, and the convention is written up indocs/skills/freighter-best-practices/references/anti-patterns.md.Closes https://github.com/stellar/wallet-eng-monorepo/issues/61
Size
Most of the diff is tests — the production surface is small (net +200).
Implementation details (for agents)
The shared predicates.
@shared/helpers/assetIdentity.ts(new) holds one predicate per identifier layer:isNativeBalance/isNativeAsset(type-based, for a balance object or an SDKAsset),isNativeAssetId(the canonical id, a Horizonasset_type, or atoken.type— matches"native"only),getNativeContractId/isNativeContract(contract space, derived viaAsset.native().contractId(passphrase)), andisNativeAssetPair(code, issuer)for the raw-strings layer where nothing better is available.isNativeBalancemoves here frompopup/helpers/balance.ts. Identity for anything other than nativeness — equality, map keys, labels — goes through the existinggetCanonicalFromAsset.Transaction building.
useSimulateTxData'sgetOperationdecides its create-account branch from the asset's type viaisNativeAsset, and is exported so the operation it builds is pinned on parsed operations.Balances and signing.
getBalanceByKeyenters its native-contract branch on the balance's type. The signing screen's fee pre-flight is extracted tohasEnoughXlmForFeeinpopup/helpers/balance.ts, anchored on the native balance.Icons, history and display.
AssetIconresolves the native icon from code and issuer together, and its memo comparator compares every render-affecting prop; the callers that build its icon map use the same pair. History icon selection pairs each code with its issuer, the Soroban transfer row decides nativeness from the contract address, and asset-detail operation matching is extracted tooperationMatchesAssetKeywith separate native and classic arms.Contract space and add-a-token.
getNativeContractDetailsderives the native contract address from the passphrase for every network.isAssetSac's native branch,useAssetLookup,useTokenLookupandAddAssetresolve the native contract throughisNativeContract; the native search row is built bybuildNativeAssetRow, and stellar.expert records are mapped bymapStellarExpertRecordalongside it.getAssetListsForAssetrequires both sides of an identity to be present before comparing them.The lint rule and the convention.
config/eslint-plugin-asset-identity/(new local plugin, wired intoeslint.config.jsat error level) addsno-asset-code-comparison: it reports a===/!==with"XLM","native", the SDK's own native code (Asset.native().code/.getCode()), or the identifier namesNATIVE_TOKEN_CODE/HORIZON_NATIVE_ASSET_TYPEon either side.Asset.native().contractId(...)is not reported: a contract-id comparison is the sound check in contract space. Every site the rule flags was migrated in this branch — 62 comparisons across 31 files, each routed to the predicate matching what its operand holds — so it reports zero hits on the tree. The convention is written up inanti-patterns.md §11with a what-you-hold → which-predicate table, andcode-style.mdrecords the rule.Verification. Full Jest suite green: 1851 passed / 51 skipped across 234 suites.
yarn build:extensionis clean with the rule live (ESLint runs inside the webpack build forextension/), and the rule was confirmed to fire before the tree was declared clean. Each migrated site has tests covering its predicate's discriminating cases — a genuine native control alongside a classic asset that uses the codeXLMwith its own issuer — and derived contract ids are pinned against the published PUBLIC and TESTNET addresses. No user-facing strings were added.Follow-ups / out of scope. Extending the build's lint gate from
extension/to@shared(the docs state the current scope). Issuer rows on the liquidity-pool branch of the trustline approval pane. Worth a smoke test before release: the add-a-token search and icon surfaces, which now resolve the native asset through the shared predicates.🤖 Generated with Claude Code