NFT gallery (Soroban CAP-46) - #600
Conversation
|
Someone is attempting to deploy a commit to the miracle656's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Thanks for the review. I’ve fixed the in-repo code issues that were causing the mobile typecheck and wallet build failures: removed the duplicate merge-conflict style blocks in the mobile app mobile typecheck passes the Vercel checks are failing because the project has not been authorized for deployment in GitHub, so those jobs cannot complete until repo/Vercel access is granted |
|
The NFT gallery itself (web |
|
Apologies for the month this waited. It no longer merges — eight conflicts: But the good news is that the conflicts are all in the wiring, not the feature. The substance of this PR is new files that nothing else has touched: Those apply cleanly. What conflicts is where you hooked the gallery into existing screens — and every one of those screens has been redesigned since (dashboard via #656/#664, and the mobile screens have moved under Suggested way backRebase and keep the three new files as they are, then redo only the integration points against current Two specifics while you are in there:
Worth sayingAn NFT gallery with tests is a real feature and the Not closing it. If you would rather hand it over after the delay, tell me and I will turn the surviving files into an issue so the work is not lost. |
Wraith indexes NFT transfers in their own table, keyed by contract and token
id, and serves them at /nfts/transfers and /nfts/owners/:contract/:token_id.
This branch read the fungible feed instead -- /transfers/address/:addr -- and
tried to recover NFTs from it with a heuristic:
t.standard === 'CAP-46' || t.type === 'nft' || t.type === 'cap46' ||
t.contractStandard === 'CAP-46' || t.contractId.includes('cap46') ||
t.tokenId !== undefined || t.isCap46
None of those fields exist on a Transfer row -- the model is {id, network,
contractId, fromAddress, toAddress, amount, ledger, ledgerClosedAt, txHash} --
and a contract id is base32, so it never contains "cap46" either. Every clause
is false for every row, so the gallery could only ever render empty. There was
no failing test because the suite asserted against invented row shapes rather
than the ones Wraith returns.
Rewritten against the real endpoints, which also fixes the deeper problem: a
transfer feed says what MOVED, not what is HELD. A token received and later
sent on appears in the feed with the wallet on both sides, and the old code
would have listed it as owned -- with `owner: t.owner || t.to || walletAddress`
naming the recipient, so the gallery could show someone else's NFT as yours.
Ownership is now the destination of a token's most recent transfer, which is
how Wraith answers /nfts/owners too, and each held token is confirmed against
that endpoint before it renders. currentHoldings() is exported and tested on
its own: received, sent away, came back, same-ledger tie, and per-token
independence within one contract.
Other changes to the data layer:
- Metadata comes from the indexer's cache and, best-effort, the token's
tokenUri. ipfs:// resolves through a gateway. A token with no image renders a
placeholder tile that says so, rather than a hardcoded Unsplash photo
presented as its art.
- A missing NEXT_PUBLIC_WRAITH_URL now raises IndexerNotConfiguredError, and
the page renders it differently from a fetch failure -- no Retry button,
because retrying a URL that was never set cannot succeed. Neither is the
empty state, which means the indexer answered and the wallet holds nothing.
- The transfer feed is paged to exhaustion (200 per page, 10 pages) rather than
reading only the first 50.
- No hardcoded default indexer URL. main already treats NEXT_PUBLIC_WRAITH_URL
as optional in the dashboard, and .env.example now documents it that way for
both the feed and the gallery.
Fixture gating is kept exactly as submitted -- explicit opt-in, never a
fallback for an empty result or a failed fetch -- and tested. The fixtures
themselves lost their real-looking contract ids and stock photography, so they
cannot be mistaken for indexed tokens.
On the page:
- Wallet address reads through walletLocal/walletSession, so testnet and
mainnet cannot see each other's wallet. The branch used raw localStorage,
which predates the namespacing.
- The "Simulate Error State" / "Simulate Empty State" buttons are development
affordances and now only render outside production. Retry stays.
- Guarded the two <Image> renders for a null image, and the search filter for
an absent symbol.
Everything the branch changed outside the gallery is dropped in favour of main:
- frontend/mobile/app/{send,receive,buy,index,swap}.tsx -- 34-line placeholder
screens ("Send tokens to another address") that would have replaced the
working ones.
- frontend/wallet/tsconfig.json -- the branch removes the react,
react/jsx-runtime and @stellar/stellar-sdk path mappings that main relies on.
- frontend/wallet/jest.config.js -- the branch's copy predates the @/ alias
mapping and points @veil/sdk at useInvisibleWallet.
- The dashboard, which on the branch is old enough to drop the PRF-downgrade
banner, hide-amounts, the greeting, currency formatting and the 24h change,
and to weaken WebAuthnSignature to any. Only the gallery entry point is
taken, as a chip beside Pools.
Verified: wallet tsc clean, 22 tests in the NFT suite, `next build` compiles
/nfts. lib/__tests__/feeBump.test.ts fails on this machine with "TextEncoder is
not defined"; it fails identically on a clean checkout of main.
Miracle656
left a comment
Miracle656
left a comment
There was a problem hiding this comment.
Approved and merging — reworked on your branch (3bdcc93), since the wave has closed.
The gallery UI is the reason I wanted to land this. Search, filter tabs, the detail modal with raw metadata, an image-failure fallback tile, and — the part most PRs skip — three genuinely distinct states for loading, empty and error, instead of one empty grid that means all three. The fixture gating is right too, and right for the reason you gave: never a fallback for an empty result or a failed fetch.
What I changed, and why
The data layer was reading the wrong feed. Wraith indexes NFT transfers in their own table, keyed by contract + token id, and serves them at /nfts/transfers and /nfts/owners/:contract/:token_id. This branch read the fungible feed, /transfers/address/:addr, and tried to recover NFTs from it:
t.standard === 'CAP-46' || t.type === 'nft' || t.type === 'cap46' ||
t.contractStandard === 'CAP-46' || t.contractId.includes('cap46') ||
t.tokenId !== undefined || t.isCap46None of those fields exist on a Transfer row — the model is {id, network, contractId, fromAddress, toAddress, amount, ledger, ledgerClosedAt, txHash} — and a contract id is base32, so it never contains "cap46" either. Every clause is false for every row. The gallery could only ever render empty, and it would have looked like a wallet with no NFTs rather than like a bug.
Nothing caught it because the test suite asserted against invented row shapes rather than the ones Wraith actually returns. That's the failure mode worth naming: a test written from the same assumption as the code confirms the assumption instead of checking it.
Transfers are not holdings. Even with a working filter, a feed tells you what moved. A token you received and later sent on appears with your address on both rows, and the old code would list it as yours — with owner: t.owner || t.to || walletAddress naming the recipient, so the gallery could show someone else's NFT as belonging to you.
Ownership is now the destination of a token's most recent transfer, which is how Wraith answers /nfts/owners too, and each held token is confirmed against that endpoint before it renders. currentHoldings() is exported and tested on its own — received, sent away, came back, same-ledger tie, per-token independence within one contract.
Smaller things in the same direction:
- A token with no image gets a placeholder that says so, instead of a hardcoded Unsplash photo presented as its art.
- A missing
NEXT_PUBLIC_WRAITH_URLnow raisesIndexerNotConfiguredError, and the page renders that differently from a fetch failure — no Retry button, since retrying a URL that was never set cannot succeed. Neither is the empty state, which means the indexer answered and you hold nothing. Three failure-ish states, three different things said. - The feed is paged to exhaustion rather than reading only the first 50.
- No hardcoded default indexer URL —
mainalready treatsNEXT_PUBLIC_WRAITH_URLas optional in the dashboard, and.env.examplenow documents it that way. - Wallet address reads through
walletLocal/walletSession, so testnet and mainnet can't see each other's wallet. - The two "Simulate …" buttons only render outside production now. They're useful; they just aren't a feature.
One thing to know for next time
Everything the branch touched outside frontend/wallet came from a base old enough that merging it would have reverted shipped work, silently — no conflict markers, because the branch was the only side that touched those files:
frontend/mobile/app/{send,receive,buy,index,swap}.tsx— 34-line placeholders ("Send tokens to another address") that would have replaced the working screens.frontend/wallet/tsconfig.json— drops thereact,react/jsx-runtimeand@stellar/stellar-sdkpath mappingsmaindepends on.frontend/wallet/jest.config.js— predates the@/alias mapping.- The dashboard — old enough to drop the PRF-downgrade banner, hide-amounts, the greeting, currency formatting and the 24h change, and to weaken
WebAuthnSignaturetoany.
All resolved by taking main. Only the gallery entry point was kept, as a chip beside Pools. Rebasing early and often is the whole defence here; this is the fourth PR this wave where a stale base would have quietly removed working features.
Verified: wallet tsc --noEmit clean, 22 tests in the NFT suite, next build compiles /nfts. (lib/__tests__/feeBump.test.ts fails on my machine with TextEncoder is not defined — it fails identically on a clean checkout of main, so it isn't yours.)
Thanks — this is a real feature and the gallery is the best-looking page in the wallet.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 33210559 | Triggered | Generic Password | 3bdcc93 | examples/sveltekit/src/lib/network.ts | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
2 participants
##closes #349