Repository navigation
Closes #729 - #782
Closes #729#782mansur-codes wants to merge 4 commits into
Conversation
|
@mansur-codes 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! 🚀 |
|
@mansur-codes is attempting to deploy a commit to the miracle656's projects Team on Vercel. A member of the Team first needs to authorize it. |
Miracle656
left a comment
There was a problem hiding this comment.
The registry data here is genuinely good — and after what the rest of this week's PRs did with addresses, that's worth saying first. All five issuers are valid strkeys and all five resolve on live mainnet Horizon with matching home_domain. No fabrication. The tests are real too, including the lookalike-issuer case asserting 'Unverified: USDY (issuer GFAK…)'. Three things block it.
1. The main acceptance criterion is unwired
#729's AC #2 is: "An asset that is not in it renders as 'Unverified: CODE (issuer GABC…)' everywhere it appears: balance rows, pickers, quotes."
$ git grep -n "formatAssetLabel\|isRegisteredIssuer" -- ':!*__tests__*' ':!scripts/*'
frontend/mobile/lib/assets.ts:121:export function isRegisteredIssuer(
frontend/mobile/lib/assets.ts:125:export function formatAssetLabel(
frontend/wallet/lib/assets.ts:103:export function isRegisteredIssuer(
frontend/wallet/lib/assets.ts:107:export function formatAssetLabel(
Definitions only — zero call sites. No balance row, picker or quote screen is touched, and the existing consumers (app/assets/page.tsx, app/assets.tsx) still call getRegisteredAsset('USDY') with one argument. So nothing in the app renders "Unverified:" today. The functions and their tests are the easy half; the wiring is the half that closes the issue.
2. expo-asset ^57.0.18 is the wrong major, and conflicts with main
package.json: "expo": "~54.0.0",
"expo-asset": "^57.0.18", ← SDK-57 release
main: "expo-asset": "~12.0.13", ← SDK-54 release
npm dist-tags: sdk-54 → 12.0.13, sdk-57 → 57.0.18, latest → 57.0.18. This took latest and pinned an SDK-57 package onto an SDK-54 app; the lockfile then pulled a whole SDK-57 subtree (expo-asset/node_modules/{@expo/env, @expo/image-utils, expo-constants, semver}), which is most of the +89 lines.
That's not hypothetical here — a mismatched expo-constants major is exactly what silently broke the About screen and the update check earlier this week, because the API it read had been removed.
I added expo-asset ~12.0.13 to main in bad9c03 (expo-font needs it but doesn't declare it), so please drop the expo-asset entry entirely and regenerate with plain npm install in frontend/mobile — never --legacy-peer-deps, which corrupts that lockfile. Currently:
CONFLICT (content): frontend/mobile/package-lock.json
CONFLICT (content): frontend/mobile/package.json
3. The CI script fails open, and only checks half the registry
The script does hit the real network and works — but:
- It passes entries it never verified. When the toml can't be fetched it prints
✓ Horizon issuer account verified for domain "circle.com"and returns. Circle's toml wasn't fetched in my run, so USDC and EURC both printed ✓ with no toml check at all — which is the AC ("fails when a registry entry's home domain does not match its issuer'sstellar.toml") going unenforced for 2 of 5 entries. Same fail-open whencurrencies.length === 0. A verifier that can't reach its source should fail, not pass. - It only parses
frontend/wallet/lib/assets.ts. The mobile registry is a hand-duplicated copy and is never verified, so drift between them is undetectable. #729 asked for "One registry module, shared by web and mobile" — the duplication is the thing the script most needs to catch, and can't. - It runs on every push, so CI now depends on horizon.stellar.org, stellar.org, aqua.network and ondo.finance being reachable. Please move it to a scheduled job or mark it
continue-on-error.
Smaller
frontend/wallet/lib/__tests__/feeBump.test.ts gains a TextEncoder/TextDecoder polyfill unrelated to #729 — and it's now redundant, since main fixed that file in 818f539 by stubbing the @veil/sdk barrel. Drop the hunk on rebase.
Please coordinate with #760
Open PR #760 ("flag unverified asset impersonators", closes #730) adds a competing lib/assetRegistry.ts in both apps and edits frontend/mobile/lib/assets.ts — the same file you rewrite. It also does the wiring you've left out (AssetRow.tsx, AssetsList.tsx, dashboard/page.tsx). Merging both would leave two parallel registries, which for a security feature means drift is the vulnerability.
Your registry is the better-sourced one and #760's is the wiring. Worth agreeing which module is canonical before either lands — happy to help broker that if you two want to split it.
Miracle656
left a comment
There was a problem hiding this comment.
Good progress — two of the three blockers are properly closed, and the main one was the one that actually mattered.
Closed:
-
1 — the wiring.
formatAssetLabelandisRegisteredIssuerhave call sites now, which is what #729's AC #2 asked for:frontend/mobile/components/AssetRow.tsx:30 {formatAssetLabel(asset.code, asset.issuer, getNetworkName())} frontend/mobile/components/AssetRow.tsx:39 <Text style={styles.unverified}>Unverified asset</Text> frontend/wallet/app/assets/page.tsx:90 if (!isRegisteredIssuer(line.code, line.issuer)) continue frontend/wallet/app/assets/page.tsx:468 {formatAssetLabel(line.code, line.issuer)}That is the half that closes the issue, and it is done.
-
2 —
expo-asset. Now~12.0.13againstexpo ~54.0.0. Correct major, and the SDK-57 subtree is gone from the lockfile. -
3b — the script parses the mobile registry too, so drift between the two copies is now detectable.
Two things left, and one of them is new.
The CI step now undoes a change that landed yesterday
.github/workflows/ci.yml:29
- run: npm run verify:assetsmain no longer has that line. #892 merged yesterday and moved the live check off the push path entirely: verify:assets:offline (StrKey, SAC derivation, wallet/mobile parity — no network) stayed fast, and the network-dependent half became a scheduled job in .github/workflows/asset-registry-verify.yml. Your branch is current with main, so this line is re-adding what that PR removed, and every contributor's push would start depending on horizon.stellar.org, circle.com, aqua.network and ondo.finance being reachable.
It would also be red immediately rather than eventually: see #978 — circle.com/.well-known/stellar.toml returns 404, so USDC and EURC cannot be corroborated by toml at all today. That is a real gap in our registry metadata, but it is not something a contributor's unrelated PR should go red for.
Use npm run verify:assets:offline there if you want a push-path check — it covers the parity your point 3b work is about, without the network.
While you are rebasing: scripts/verify-asset-registry.mjs was also restructured by #892, so that file is worth re-reading before changing it further. The fail-open paths you were asked to close may already be closed there.
Build byproducts are still committed
frontend/wallet/public/sw.js 1+/1- regenerated service worker
frontend/wallet/next-env.d.ts 1+/1-
frontend/wallet/middleware.js 2+/0-
git checkout -- on those three. sw.js matters beyond tidiness: committing a locally-built service worker pins chunk hashes that will not exist in the real deploy. (This catches me too — I dirtied the same files running next build last week.)
And #760 is still open
#760 adds a competing lib/assetRegistry.ts in both apps and edits frontend/mobile/lib/assets.ts, which you rewrite. Now that your wiring is in, the two PRs overlap on the display layer as well as the data. Yours is the better-sourced registry; whichever lands second will need to drop its own copy rather than merge alongside it. Two parallel registries for a feature whose purpose is detecting impostors would make drift the vulnerability.
Fix the CI line and drop the three byproducts and I am happy to merge this.
|
Correction to my review — I lumped It is the CSP
The two that genuinely are byproducts:
So the remaining list is shorter than my review implied: the |
Summary
Stellar mainnet has 20+ issuers for
USDYandEURCeach, most of them fakes — one counterfeitEURCeven publishes ablackrock.com.sedomain. The wallet had no way to distinguish the real issuer from impostors; it resolved whatever it found first.This PR adds a static verified asset registry shared across web and mobile, pins exact mainnet issuers for USDC, XLM, EURC, AQUA, and USDY, and enforces that any unregistered issuer renders as
"Unverified: CODE (issuer GABC…)"everywhere: balance rows, pickers, quotes. A new CI script (verify-asset-registry.mjs) fetches each issuer'sstellar.tomlat build time and fails the pipeline on any mismatch.Related issue
Closes #729
Type of change
Component
Checklist
cargo testpasses (contracts)npm run typecheckpasses (wallet / sdk / agent)npm run buildpasses (wallet / agent)Screenshots / test output