feat: special case arb search #6584
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #6584 +/- ##
==========================================
- Coverage 62.62% 62.47% -0.15%
==========================================
Files 714 714
Lines 21039 21044 +5
Branches 6956 6956
==========================================
- Hits 13175 13147 -28
- Misses 7786 7819 +33
Partials 78 78
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
I completely agree about 1 offs, and suggested using huge volume differences as a metric to override the default behaviour of showing the token on the wallet's current chain, but product wants to keep the scope of this change to just ARB for now. We can't re-use the MATIC logic, because that logic relies on Polygon MATIC being a native token |
src/graphql/data/SearchTokens.ts
Outdated
| // Always priotize natives, and if both tokens are native, prefer native on current chain (i.e. Matic on Polygon over Matic on Mainnet ) | ||
| if (current.standard === 'NATIVE' && (existing.standard !== 'NATIVE' || current.chain === searchChain)) return true | ||
| // Special case: always prefer Arbitrum ARB over Mainnet ARB | ||
| if (current.address === ARB.address) return current |
There was a problem hiding this comment.
I'm seeing mainnet ARB over arbitrum ARB in the preview
| if (current.address?.toLowerCase() === ARB_ADDRESS) return current | ||
| if (existing.address?.toLowerCase() === ARB_ADDRESS) return existing |
There was a problem hiding this comment.
Are you sure both of these are necessary? Having just the first check, if (current.address?.toLowerCase() === ARB_ADDRESS) return current, with the lowercase casting works for mainnet and arbitrum networks
There was a problem hiding this comment.
It depends on the order the backend returns the two ARB tokens in, if the backend started returning them in the opposite order, only having one check wouldn't work.
| @@ -87,7 +99,7 @@ export function useSearchTokens(searchQuery: string, chainId: number) { | |||
| data?.searchTokens?.forEach((token) => { | |||
| if (token.project?.id) { | |||
| const existing = selectionMap[token.project.id] | |||
| if (isMoreRevelantToken(token, existing, searchChain)) selectionMap[token.project.id] = token | |||
There was a problem hiding this comment.
it looks like there's a behavior change here that might not be intentional--in the old one there were cases where selectionMap[token.project.id] wouldn't be assigned a value here, but now every iteration assigns a value. Is that intended?
There was a problem hiding this comment.
In cases where isMoreRevelantToken would've returned false, dedupeCrosschainTokens returns existing, so this is just a refactor for readability
|
|
Description
Special cases the ARB token on Arbitrum to always display over the ARB token on Mainnet in search results when both are returned by the backend, regardless of the user's current chain.
Also cleans up / renames the cross token search code to make functionality more clear.
Linear ticket
Screen capture
QA (ie manual testing)