chore(swap-service): use relayTokenToAssetId from @shapeshiftoss/swapper#40
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughSwapVerificationService is refactored to use the external ChangesRelay Verification Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/swap-service/src/verification/swap-verification.service.ts (1)
193-197: ⚡ Quick winLog relay token conversion failures before fallback.
Line 193 currently swallows conversion errors silently; add a warning so malformed Relay payloads are observable during verification.
Proposed patch
- try { - return relayTokenToAssetId(currency) - } catch { - return undefined - } + try { + return relayTokenToAssetId(currency) + } catch (e) { + this.logger.warn( + `Relay - failed to derive affiliate fee assetId for relayId=${relayId}: ${ + e instanceof Error ? e.message : 'Unknown error' + }`, + ) + return undefined + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/swap-service/src/verification/swap-verification.service.ts` around lines 193 - 197, The catch block that swallows errors from relayTokenToAssetId(currency) must log a warning before returning undefined; update the try/catch in swap-verification.service.ts (the call to relayTokenToAssetId and the surrounding verification logic) to catch (err) and call the appropriate logger (e.g., this.logger.warn or processLogger.warn) with a message that includes the error, the malformed currency payload, and context ("relay token conversion failed for currency") so failures are observable, then return undefined as the fallback.apps/swap-service/src/verification/__tests__/setup.ts (1)
63-68: ⚡ Quick winUpdate the mock to match the real
relayTokenToAssetIdbehavior to catch regressions for non-EVM chains.The current stub only handles EVM tokens (returning
eip155:chainId/slip44:60oreip155:chainId/erc20:address), but the real implementation from@shapeshiftoss/swappersupports BTC, Solana, and Tron formats. Tests currently cover only Ethereum mainnet (chainId 1), so they won't catch breaks in non-EVM token mapping if this PR expands relay support to other chains.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/swap-service/src/verification/__tests__/setup.ts` around lines 63 - 68, The mock implementation of relayTokenToAssetId only covers EVM (isNative -> eip155:.../slip44:60 or erc20:...), so update the stub in setup.ts to mirror the real `@shapeshiftoss/swapper` logic used in production: detect chain/network and return BTC (e.g., slip44-based or proper btc asset id), Solana (solana:<pubkey> format), Tron (tron:<address>), and EVM variants (eip155:.../slip44:60 or eip155:.../erc20:lowercased) as the real function does; ensure you use the same token shape/fields and lowercasing where appropriate so tests will catch regressions for non-EVM chains when calling relayTokenToAssetId.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/swap-service/src/verification/__tests__/setup.ts`:
- Around line 63-68: The mock implementation of relayTokenToAssetId only covers
EVM (isNative -> eip155:.../slip44:60 or erc20:...), so update the stub in
setup.ts to mirror the real `@shapeshiftoss/swapper` logic used in production:
detect chain/network and return BTC (e.g., slip44-based or proper btc asset id),
Solana (solana:<pubkey> format), Tron (tron:<address>), and EVM variants
(eip155:.../slip44:60 or eip155:.../erc20:lowercased) as the real function does;
ensure you use the same token shape/fields and lowercasing where appropriate so
tests will catch regressions for non-EVM chains when calling
relayTokenToAssetId.
In `@apps/swap-service/src/verification/swap-verification.service.ts`:
- Around line 193-197: The catch block that swallows errors from
relayTokenToAssetId(currency) must log a warning before returning undefined;
update the try/catch in swap-verification.service.ts (the call to
relayTokenToAssetId and the surrounding verification logic) to catch (err) and
call the appropriate logger (e.g., this.logger.warn or processLogger.warn) with
a message that includes the error, the malformed currency payload, and context
("relay token conversion failed for currency") so failures are observable, then
return undefined as the fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b280e754-c69e-4ad7-bc04-57180729ca3a
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
apps/swap-service/src/verification/__tests__/setup.tsapps/swap-service/src/verification/swap-verification.service.tsapps/swap-service/src/verification/types.tspackage.json
Replaces inline assetId construction with the canonical helper from the swapper package (bumped to 17.7.3), which handles non-EVM chains and Relay's chain-id mapping correctly. Adds the function to the test mock. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kaladinlight
force-pushed
the
chore/swap-relay-token-to-asset-id
branch
from
ee5e045 to
660dc03
Compare
1 participant
Description
Replaces the inline
appFeeCurrencyObject→AssetIdconstruction in the Relay verifier with the canonicalrelayTokenToAssetIdhelper now exported from@shapeshiftoss/swapper(bumped 17.7.0 → 17.7.3). The helper correctly handles non-EVM chains (BTC/Solana/Tron) and Relay's numeric-chainId mapping — both of which the inline implementation got wrong.Also adds
relayTokenToAssetIdto the test-side mock of@shapeshiftoss/swapperinverification/__tests__/setup.tsand tightens the localRelayTokeninterface to non-optional fields (matching the package contract).Testing
cd apps/swap-service && npx jest src/verification/__tests__/relay.test.ts— existing relay tests still pass (native ETH, ERC20 USDT, missing currency cases)Summary by CodeRabbit
Bug Fixes
Chores