fix(wallets): support locator-based token matching in balances filter#1819
fix(wallets): support locator-based token matching in balances filter#1819mPaella wants to merge 4 commits into
Conversation
The otherTokens filter in transformBalanceResponse only matched by symbol (e.g. 'aaplx'), dropping tokens requested via chain locators like 'solana:<mint>'. This adds locator-aware matching for Solana mintHash, EVM contractAddress, and Stellar contractId. Co-Authored-By: maxwell@paella.dev <maxwell@paella.dev>
Original prompt from maxwell@paella.dev
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
🦋 Changeset detectedLatest commit: dddd78d The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/wallets/src/wallets/wallet.ts:374-412
**Extract `matchesRequestedToken` into a private method**
Per the team's convention, validation/matching helpers should be extracted into separate, descriptively-named functions rather than defined inline inside another method. `transformBalanceResponse` is now ~110 lines; extracting this closure into a private `matchesRequestedToken(token, requestedTokenSet)` method would keep each function focused and make the logic independently testable.
### Issue 2 of 2
packages/wallets/src/wallets/wallet.ts:374-412
**Missing unit tests for locator-based matching**
The new matching paths (Solana `mintHash`, EVM `contractAddress`, Stellar `contractId`, and their prefixed `chain:<value>` forms) are not covered by tests in `wallet.test.ts`. The existing test at line 252 only exercises symbol-based filtering (`wallet.balances(["dai"])`). Cases like `wallet.balances(["solana:XsbEh..."])` or `wallet.balances(["base-sepolia:0x..."])` should be added to confirm the fix works as described and to prevent regressions.
Reviews (1): Last reviewed commit: "fix(wallets): support locator-based toke..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const matchesRequestedToken = (token: GetBalanceSuccessResponse[number]): boolean => { | ||
| if (requestedTokenSet.size === 0) return true; | ||
|
|
||
| const symbol = (token.symbol ?? "").toLowerCase(); | ||
| if (requestedTokenSet.has(symbol)) return true; | ||
|
|
||
| const chainData = token.chains?.[this.chain]; | ||
|
|
||
| if (this.chain === "solana" && chainData != null && "mintHash" in chainData) { | ||
| const mint = (chainData.mintHash as string | undefined)?.toLowerCase(); | ||
| if (mint != null) { | ||
| if (requestedTokenSet.has(mint)) return true; | ||
| if (requestedTokenSet.has(`solana:${mint}`)) return true; | ||
| } | ||
| } | ||
|
|
||
| if ( | ||
| this.chain !== "solana" && | ||
| this.chain !== "stellar" && | ||
| chainData != null && | ||
| "contractAddress" in chainData | ||
| ) { | ||
| const addr = (chainData.contractAddress as string | undefined)?.toLowerCase(); | ||
| if (addr != null) { | ||
| if (requestedTokenSet.has(addr)) return true; | ||
| if (requestedTokenSet.has(`${this.chain}:${addr}`)) return true; | ||
| } | ||
| } | ||
|
|
||
| if (this.chain === "stellar" && chainData != null && "contractId" in chainData) { | ||
| const contractId = (chainData.contractId as string | undefined)?.toLowerCase(); | ||
| if (contractId != null) { | ||
| if (requestedTokenSet.has(contractId)) return true; | ||
| if (requestedTokenSet.has(`stellar:${contractId}`)) return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| }; |
There was a problem hiding this comment.
matchesRequestedToken into a private method
Per the team's convention, validation/matching helpers should be extracted into separate, descriptively-named functions rather than defined inline inside another method. transformBalanceResponse is now ~110 lines; extracting this closure into a private matchesRequestedToken(token, requestedTokenSet) method would keep each function focused and make the logic independently testable.
Rule Used: Extract validation logic into separate functions w... (source)
Learned From
Paella-Labs/crossbit-main#21036
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/wallets/wallet.ts
Line: 374-412
Comment:
**Extract `matchesRequestedToken` into a private method**
Per the team's convention, validation/matching helpers should be extracted into separate, descriptively-named functions rather than defined inline inside another method. `transformBalanceResponse` is now ~110 lines; extracting this closure into a private `matchesRequestedToken(token, requestedTokenSet)` method would keep each function focused and make the logic independently testable.
**Rule Used:** Extract validation logic into separate functions w... ([source](https://app.greptile.com/review/custom-context?memory=3cb55a22-7771-4eb7-96c6-7ad7dbe56fb2))
**Learned From**
[Paella-Labs/crossbit-main#21036](https://github.com/Paella-Labs/crossbit-main/pull/21036)
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fair point from the cold-blooded one 🦎 — extracted matchesRequestedToken into a proper private method. The snake slithered to the right conclusion this time.
| const matchesRequestedToken = (token: GetBalanceSuccessResponse[number]): boolean => { | ||
| if (requestedTokenSet.size === 0) return true; | ||
|
|
||
| const symbol = (token.symbol ?? "").toLowerCase(); | ||
| if (requestedTokenSet.has(symbol)) return true; | ||
|
|
||
| const chainData = token.chains?.[this.chain]; | ||
|
|
||
| if (this.chain === "solana" && chainData != null && "mintHash" in chainData) { | ||
| const mint = (chainData.mintHash as string | undefined)?.toLowerCase(); | ||
| if (mint != null) { | ||
| if (requestedTokenSet.has(mint)) return true; | ||
| if (requestedTokenSet.has(`solana:${mint}`)) return true; | ||
| } | ||
| } | ||
|
|
||
| if ( | ||
| this.chain !== "solana" && | ||
| this.chain !== "stellar" && | ||
| chainData != null && | ||
| "contractAddress" in chainData | ||
| ) { | ||
| const addr = (chainData.contractAddress as string | undefined)?.toLowerCase(); | ||
| if (addr != null) { | ||
| if (requestedTokenSet.has(addr)) return true; | ||
| if (requestedTokenSet.has(`${this.chain}:${addr}`)) return true; | ||
| } | ||
| } | ||
|
|
||
| if (this.chain === "stellar" && chainData != null && "contractId" in chainData) { | ||
| const contractId = (chainData.contractId as string | undefined)?.toLowerCase(); | ||
| if (contractId != null) { | ||
| if (requestedTokenSet.has(contractId)) return true; | ||
| if (requestedTokenSet.has(`stellar:${contractId}`)) return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| }; |
There was a problem hiding this comment.
The new matching paths (Solana mintHash, EVM contractAddress, Stellar contractId, and their prefixed chain:<value> forms) are not covered by tests in wallet.test.ts. The existing test at line 252 only exercises symbol-based filtering (wallet.balances(["dai"])). Cases like wallet.balances(["solana:XsbEh..."]) or wallet.balances(["base-sepolia:0x..."]) should be added to confirm the fix works as described and to prevent regressions.
Rule Used: Add unit tests when implementing new validation lo... (source)
Learned From
Paella-Labs/crossbit-main#21014
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/wallets/src/wallets/wallet.ts
Line: 374-412
Comment:
**Missing unit tests for locator-based matching**
The new matching paths (Solana `mintHash`, EVM `contractAddress`, Stellar `contractId`, and their prefixed `chain:<value>` forms) are not covered by tests in `wallet.test.ts`. The existing test at line 252 only exercises symbol-based filtering (`wallet.balances(["dai"])`). Cases like `wallet.balances(["solana:XsbEh..."])` or `wallet.balances(["base-sepolia:0x..."])` should be added to confirm the fix works as described and to prevent regressions.
**Rule Used:** Add unit tests when implementing new validation lo... ([source](https://app.greptile.com/review/custom-context?memory=9dc35ad5-5868-49a3-bbcb-a42edc5ee697))
**Learned From**
[Paella-Labs/crossbit-main#21014](https://github.com/Paella-Labs/crossbit-main/pull/21014)
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Even a broken lizard is right twice a day 🐍 — added tests for all locator paths: EVM contractAddress (prefixed + bare), Solana mintHash (prefixed), Stellar contractId (prefixed), and case-insensitive matching. All 103 tests pass.
…and changeset Co-Authored-By: maxwell@paella.dev <maxwell@paella.dev>
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/wallets/src/wallets/wallet.ts:421
**Empty array now silently becomes "no filter"**
`requestedTokenSet.size === 0` returns `true` (include all tokens) for both `requestedTokens = undefined` and `requestedTokens = []`. In the original code, `[].includes(anything)` is always `false`, so `wallet.balances([])` would produce an empty `tokens` array. With this change, an explicitly empty array behaves identically to no argument — returning all non-native/non-usdc tokens. If any caller dynamically builds the filter array and occasionally lands on `[]` expecting zero tokens back, they'll silently receive all tokens instead.
Reviews (2): Last reviewed commit: "refactor: extract matchesRequestedToken ..." | Re-trigger Greptile |
Co-Authored-By: maxwell@paella.dev <maxwell@paella.dev>
|
Alright, credit where it's due — the reptile shed some light on a real regression 🐊 Fixed: |
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/wallets/src/wallets/wallet.ts:432-436
**Case-insensitive Solana mint matching may produce false positives**
Solana addresses are base58-encoded and case-sensitive — `XsbEhp...` and `xSBEHp...` are entirely different valid addresses that share the same lowercased string. Applying `.toLowerCase()` before comparing means a caller who accidentally requests the all-lowercase version of a Solana mint could silently match a different token. EVM hex addresses (EIP-55 mixed-case checksum) are the intended beneficiary of case-insensitive matching here; for Solana, an exact comparison would be safer.
Reviews (3): Last reviewed commit: "fix: preserve empty-array semantics for ..." | Re-trigger Greptile |
|
Appreciate the concern, but I think the gecko is overthinking this one 🦎 While it's true that base58 is case-sensitive in theory, in practice the callers here are passing locator strings like I'll leave this as-is — happy to revisit if a human reviewer disagrees! |
| "contractAddress" in chainData | ||
| ) { | ||
| const addr = (chainData.contractAddress as string | undefined)?.toLowerCase(); | ||
| if (addr != null) { |
There was a problem hiding this comment.
@devinai in general there are many unnecessary nested ifs in this funtion couldn't we add chainData.contractAddress != null in the first if? This happens with all the three if statements lets try not to add so many if levels
There was a problem hiding this comment.
Good call — flattened the nested ifs by pulling chainData == null into an early return, then combining each chain's null check into the outer if condition. Much cleaner now, thanks!
| ) { | ||
| const addr = (chainData.contractAddress as string | undefined)?.toLowerCase(); | ||
| if (addr != null) { | ||
| if (requestedTokenSet.has(addr)) return true; |
There was a problem hiding this comment.
@devinai if we got to this point where this.chain !== "stellar" we definitely are not going to enter this.chain === "stellar" so can we just return requestedTokenSet.has(addr) || requestedTokenSet.has(${this.chain}:${addr}`) instead of having to ifs that return true, and we shortcircuit the funciton. I think we can do this in the three if statements, all are mutually exclusive so once they got to that point we can already return
There was a problem hiding this comment.
Agreed — since the three branches (solana/stellar/evm) are mutually exclusive, each now returns the result directly instead of using nested ifs that fall through. Pushed in dddd78d.
Co-Authored-By: maxwell@paella.dev <maxwell@paella.dev>
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/wallets/src/wallets/wallet.ts:432-443
The bare-address path for Solana (`requestedTokenSet.has(mint)`) and Stellar (`requestedTokenSet.has(contractId)`) is implemented but has no test coverage. Tests exist for prefixed locators (`solana:<mint>`, `stellar:<contractId>`) and for bare EVM `contractAddress`, but the equivalent bare-mint and bare-contractId paths are untested. A user calling `wallet.balances(["XsbEhpSupF7..."])` would silently return empty results if there's ever a regression here.
Reviews (4): Last reviewed commit: "refactor: flatten nested ifs and short-c..." | Re-trigger Greptile |
|
The cold-blooded reviewer is getting nitpicky now 🐊 We already test bare address matching for EVM ( |
2 participants
Description
wallet.balances(tokens)filters returned tokens viarequestedTokens.includes(token.symbol), which only works when requested tokens are symbols (e.g."aaplx"). When tokens are requested by chain locator (e.g.solana:XsbEh...), the API returns the token correctly but the SDK drops it because the response symbol (AAPLx) does not match the locator string.This adds a private
matchesRequestedTokenmethod that performs case-insensitive matching against:mintHash(bare orsolana:<mint>format)contractAddress(bare or<chain>:<address>format)contractId(bare orstellar:<contractId>format)The three chain branches are mutually exclusive and short-circuit, keeping the method flat and readable.
nullvs empty-array semantics are preserved:undefinedmeans no filter (return all),[]means return none.Test plan
contractAddressviabase-sepolia:<address>locatorcontractAddressvia bare addressmintHashviasolana:<mint>locatorcontractIdviastellar:<contractId>locatorwallet.balances(["dai"])) still passeswallet.test.tspasspnpm lint)Package updates
@crossmint/wallets-sdk: patch — changeset added via.changeset/locator-balance-fix.mdLink to Devin session: https://crossmint.devinenterprise.com/sessions/b31034c19c8d4387adefb47237ba2953
Requested by: @mPaella