refactor(swap-verification): slim verifier contract and unify logs - #31
Merged
Merged
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 selected for processing (6)
📝 WalkthroughWalkthroughThe pull request refactors swap reconciliation and verification flows, treating reconciliation as side-effect-only, removing affiliate verification fields from API responses, and introducing centralized verification logging. Type safety is improved through stronger enum-based typing of swapper names, and actual amounts are now explicitly tracked in verification results. Changes
Sequence DiagramsequenceDiagram
participant PollService as Poll Flow
participant SwapService as Swaps Service
participant VerifyService as Verification Service
participant Database as Database/Prisma
participant Logger as Logger
PollService->>SwapService: pollSwapStatus(swapId)
SwapService->>VerifyService: verifySwap(swapDetails)
VerifyService->>VerifyService: Compute verification result<br/>(actualBuyAmount, actualFeeAmount)
VerifyService->>Logger: logVerification(result)
Logger->>Logger: Format log segments<br/>(affiliate status, amounts)
VerifyService-->>SwapService: SwapVerificationResult<br/>(without swapId/swapperName)
SwapService->>SwapService: reconcileSwap(result)<br/>(side-effect only)
SwapService->>Database: prisma.swap.update<br/>(persist verification amounts)
Database-->>SwapService: Updated
SwapService->>Database: fetch swap data<br/>(no affiliate fields)
Database-->>SwapService: SwapStatusResponse<br/>(affiliate details not included)
SwapService-->>PollService: SwapStatusResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
The SwapVerificationResult shape carried fields that no caller actually
read — swapId, swapperName, and details were echoed by every verifier but
only ever inspected by the now-deleted POST /:id/verify-affiliate route.
Reducing the contract to what reconcileSwap consumes makes the verifier's
"output spec" reflect reality and shrinks the per-verifier boilerplate.
Threading through that change:
- SwapVerificationResult drops swapId / swapperName / details and adds
required actualBuyAmountCryptoBaseUnit / actualAffiliateFeeAmountCryptoBaseUnit
as string | undefined (required keys force every verifier to consciously
decide whether it can produce them — silent omission is no longer possible).
- pollSwapStatus no longer round-trips isAffiliateVerified /
affiliateVerificationDetails — the only caller never read them, and external
consumers read those columns directly off the swap row. SwapStatusResponse
shrinks accordingly; SwapReconciliation type is removed; reconcileSwap is
now Promise<void>, doing only the side-effect write (and now persists the
two new actual-amount columns from whatever the verifier returns).
- updateSwapStatus drops actualBuyAmountCryptoBaseUnit — there's no producer
anywhere after the HTTP route was deleted, and it's now sourced from
verification.
Verifier mechanics:
- All 16 verifiers' returns slimmed; details blocks gone; dead variables
that only fed details cleaned up (feeAmountUsd, referrerFeeUnits,
fillTxnRef).
- Per-verifier summary logs unified via a logVerification helper in
verification/utils.ts. Pipe-format with stable column shape:
<Name> verification | swapId=<id> | affiliate=<addr> (<bps> bps) | sell=<n> | buy=<n> | fee=<n> | <extras>
Missing amounts render as `=none` so operators can grep for unverified
swaps; verifier-specific context (memo, status, feeAmount) lives in extras.
Type tightening:
- CreateSwapDto.swapperName: string → SwapperName.
- affiliateFeeAsset's strategy map keyed by SwapperName (exhaustive — picks
up ArbitrumBridge / Debridge / Test which previously fell through to null).
No verifier extracts actualBuy/actualFee yet; those land per-swapper in
follow-ups (NearIntents first).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/swap-service/src/verification/swap-verification.service.ts (1)
220-228:⚠️ Potential issue | 🟡 MinorInconsistent logging: Relay/Chainflip/0x verifiers don't call
logVerificationon the success path.The PR description states "All verifier logs follow a pipe-separated stable format" via
logVerification, butverifyRelay(Lines 220-228),verifyChainflip(Lines 639-647), andverifyZrx(Lines 720-728) construct and return the result directly without callinglogVerification. This leaves a gap in the unified format you're trying to establish — three high-volume swappers will silently skip the new log line.🛠 Suggested fix (apply analogously to all three verifiers)
- return { + const result: SwapVerificationResult = { isVerified: true, hasAffiliate: hasShapeshiftAffiliate, affiliateBps, affiliateAddress, verifiedSellAmountCryptoBaseUnit, actualBuyAmountCryptoBaseUnit: undefined, actualAffiliateFeeAmountCryptoBaseUnit: undefined, } + + logVerification(this.logger, SwapperName.Relay, swapId, result) + + return resultAlso applies to: 639-647, 720-728
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/swap-service/src/verification/swap-verification.service.ts` around lines 220 - 228, verifyRelay, verifyChainflip and verifyZrx currently return success results directly and skip the unified log line; update each verifier to call logVerification(...) with the final result object (same fields you return: isVerified, hasAffiliate, affiliateBps, affiliateAddress, verifiedSellAmountCryptoBaseUnit, actualBuyAmountCryptoBaseUnit, actualAffiliateFeeAmountCryptoBaseUnit) before returning it so the pipe-separated stable format is emitted for these high-volume verifiers; locate calls in functions verifyRelay, verifyChainflip and verifyZrx and invoke logVerification(contextOrParams, result) (matching the existing logVerification signature used elsewhere) and then return the logged result.
🧹 Nitpick comments (3)
apps/swap-service/src/verification/utils.ts (1)
16-42: Helper looks clean; one optional consideration on duplicate keys.If a caller passes an
extrakey that collides with a base segment (e.g.,swapId,sell,buy,fee,affiliate), the line will end up with two segments using the same key. Consider either dropping conflicting keys or documenting reserved key names. This is purely defensive — current call sites passstatus,memo,feeAmount, none of which collide.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/swap-service/src/verification/utils.ts` around lines 16 - 42, The logVerification helper can produce duplicate keys if callers include extras that collide with base segments; update logVerification to defensively drop or rename conflicting keys in the incoming extra object before building segments: define a reserved set (e.g., 'swapId','sell','buy','fee','affiliate' and any other base keys used in segments), then filter Object.entries(extra) to exclude any key in that reserved set (or alternatively prefix remaining keys), and finally build the extras array from the filtered entries so no duplicate keys appear in the joined log; keep the function name logVerification and the same output format.apps/swap-service/src/utils/affiliateFeeAsset.ts (1)
27-41: Tighten exhaustiveness now that the strategy map isRecord<SwapperName, FeeAssetStrategy>.Two small follow-ups:
if (!strategy) return null(Line 29) is now unreachable since the map is exhaustive over theSwapperNameenum.- The
'none'case is handled implicitly viadefault, which masks future additions toFeeAssetStrategy. Making'none'explicit and using anever-typed default lets TypeScript catch newly-added strategies.♻️ Proposed refactor
export function resolveAffiliateFeeAssetId(swapperName: SwapperName, sellAsset: Asset, buyAsset: Asset): string | null { const strategy = SWAPPER_FEE_STRATEGY[swapperName] - if (!strategy) return null switch (strategy) { case 'buy_asset': return buyAsset.assetId case 'sell_asset': return sellAsset.assetId case 'fixed_base': return 'eip155:8453/erc20:0x833589fcd6edb6e08f4c7c32d4f71b54bda02913' - default: + case 'none': return null + default: { + const _exhaustive: never = strategy + void _exhaustive + return null + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/swap-service/src/utils/affiliateFeeAsset.ts` around lines 27 - 41, The function resolveAffiliateFeeAssetId currently checks `if (!strategy) return null` which is now unreachable because `SWAPPER_FEE_STRATEGY` is a Record<SwapperName, FeeAssetStrategy>; remove that guard, add an explicit case for the 'none' strategy (returning null) in the switch, and replace the `default` branch with a `const _exhaustiveCheck: never = strategy;` style exhaustiveness assertion (or call an assertNever(strategy)) so TypeScript will error if FeeAssetStrategy gains new members; reference resolveAffiliateFeeAssetId, SWAPPER_FEE_STRATEGY, SwapperName and FeeAssetStrategy when making these changes.apps/swap-service/src/verification/swap-verification.service.ts (1)
137-141: Replace inline literals with class constants for the NearIntents referral.The hardcoded
'shapeshift'(Line 137) and the single-element list['shapeshifttokenomics.sputnik-dao.near'](Line 140) break the pattern used elsewhere in this service (shapeshiftRelayReferrer,shapeshiftMayaAffiliate, etc.). Promoting them to private readonly fields keeps Near consistent with the other verifiers, makes future updates a one-line change, and avoids implicit case sensitivity on the recipient match.♻️ Proposed refactor
+ private readonly shapeshiftNearReferral = 'shapeshift' + private readonly shapeshiftNearFeeRecipients = ['shapeshifttokenomics.sputnik-dao.near'] ... - const hasShapeshiftReferral = referral?.toLowerCase() === 'shapeshift' + const hasShapeshiftReferral = referral?.toLowerCase() === this.shapeshiftNearReferral const shapeshiftFee = hasShapeshiftReferral - ? appFees.find(({ recipient }) => ['shapeshifttokenomics.sputnik-dao.near'].includes(recipient)) + ? appFees.find(({ recipient }) => this.shapeshiftNearFeeRecipients.includes(recipient?.toLowerCase())) : undefinedNote: NEAR account IDs are case-insensitive in the protocol (canonical form is lowercase), so an explicit
toLowerCase()onrecipientdefends against any upstream casing drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/swap-service/src/verification/swap-verification.service.ts` around lines 137 - 141, Replace the inline literals in the swap verification logic with class constants: add private readonly fields (e.g., shapeshiftReferral = 'shapeshift' and shapeshiftTokenomicsRecipient = 'shapeshifttokenomics.sputnik-dao.near') alongside the existing shapeshiftRelayReferrer/shapeshiftMayaAffiliate, then update the two usages in the method that define hasShapeshiftReferral and shapeshiftFee to reference those fields; also normalize recipient before matching (recipient?.toLowerCase()) so the appFees.find uses the constant comparison defensively.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/shared-types/src/index.ts`:
- Line 73: Request bodies can contain arbitrary strings for swapperName because
CreateSwapDto is only an interface and there's no global ValidationPipe; this
allows invalid values to reach resolveAffiliateFeeAssetId and the swappers
lookup (swaps.service.ts) and bypass the exhaustive switch in
swap-verification.service.ts. Fix by converting CreateSwapDto to a class and add
`@IsEnum`(SwapperName) on the swapperName property (or apply a per-endpoint
validation pipe that checks SwapperName) and enable validation at the controller
boundary (or enable global ValidationPipe as noted by the TODO in main.ts) so
incoming requests are rejected for invalid SwapperName values before they reach
resolveAffiliateFeeAssetId or the swappers[...] lookup.
---
Outside diff comments:
In `@apps/swap-service/src/verification/swap-verification.service.ts`:
- Around line 220-228: verifyRelay, verifyChainflip and verifyZrx currently
return success results directly and skip the unified log line; update each
verifier to call logVerification(...) with the final result object (same fields
you return: isVerified, hasAffiliate, affiliateBps, affiliateAddress,
verifiedSellAmountCryptoBaseUnit, actualBuyAmountCryptoBaseUnit,
actualAffiliateFeeAmountCryptoBaseUnit) before returning it so the
pipe-separated stable format is emitted for these high-volume verifiers; locate
calls in functions verifyRelay, verifyChainflip and verifyZrx and invoke
logVerification(contextOrParams, result) (matching the existing logVerification
signature used elsewhere) and then return the logged result.
---
Nitpick comments:
In `@apps/swap-service/src/utils/affiliateFeeAsset.ts`:
- Around line 27-41: The function resolveAffiliateFeeAssetId currently checks
`if (!strategy) return null` which is now unreachable because
`SWAPPER_FEE_STRATEGY` is a Record<SwapperName, FeeAssetStrategy>; remove that
guard, add an explicit case for the 'none' strategy (returning null) in the
switch, and replace the `default` branch with a `const _exhaustiveCheck: never =
strategy;` style exhaustiveness assertion (or call an assertNever(strategy)) so
TypeScript will error if FeeAssetStrategy gains new members; reference
resolveAffiliateFeeAssetId, SWAPPER_FEE_STRATEGY, SwapperName and
FeeAssetStrategy when making these changes.
In `@apps/swap-service/src/verification/swap-verification.service.ts`:
- Around line 137-141: Replace the inline literals in the swap verification
logic with class constants: add private readonly fields (e.g.,
shapeshiftReferral = 'shapeshift' and shapeshiftTokenomicsRecipient =
'shapeshifttokenomics.sputnik-dao.near') alongside the existing
shapeshiftRelayReferrer/shapeshiftMayaAffiliate, then update the two usages in
the method that define hasShapeshiftReferral and shapeshiftFee to reference
those fields; also normalize recipient before matching
(recipient?.toLowerCase()) so the appFees.find uses the constant comparison
defensively.
In `@apps/swap-service/src/verification/utils.ts`:
- Around line 16-42: The logVerification helper can produce duplicate keys if
callers include extras that collide with base segments; update logVerification
to defensively drop or rename conflicting keys in the incoming extra object
before building segments: define a reserved set (e.g.,
'swapId','sell','buy','fee','affiliate' and any other base keys used in
segments), then filter Object.entries(extra) to exclude any key in that reserved
set (or alternatively prefix remaining keys), and finally build the extras array
from the filtered entries so no duplicate keys appear in the joined log; keep
the function name logVerification and the same output format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6edfedb-e313-441f-a908-225334de3d48
📒 Files selected for processing (6)
apps/swap-service/src/swaps/swaps.service.tsapps/swap-service/src/swaps/types.tsapps/swap-service/src/utils/affiliateFeeAsset.tsapps/swap-service/src/verification/swap-verification.service.tsapps/swap-service/src/verification/utils.tspackages/shared-types/src/index.ts
💤 Files with no reviewable changes (1)
- apps/swap-service/src/swaps/types.ts
kaladinlight
force-pushed
the
refactor/swap-verification-contract
branch
from
April 27, 2026 22:14
9d322f8 to
f8919bb
Compare
This was referenced Jul 1, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Reduces the verifier contract to what callers actually consume, drops dead return paths, and unifies per-verifier logs behind a small helper.
SwapVerificationResult shape change.
swapId,swapperName,detailswere echoed by every verifier but only ever inspected by the now-deletedPOST /:id/verify-affiliateHTTP route. With that route gone, those fields are pure noise for the only remaining caller (reconcileSwap), which already has the swap object in scope. Removing them shrinks per-verifier boilerplate considerably.New required fields.
actualBuyAmountCryptoBaseUnitandactualAffiliateFeeAmountCryptoBaseUnitare added asstring | undefined(required keys, optional values) so the type system forces every verifier — current and future — to explicitly decide whether it can produce them.reconcileSwapwrites both to the swap row alongsideisAffiliateVerified. No verifier extracts them yet; per-swapper extraction lands in follow-ups (NearIntents first).pollSwapStatusno longer round-trips verification metadata.isAffiliateVerified/affiliateVerificationDetailswere declared onSwapStatusResponsebut never read by the polling service (the only consumer). External consumers read those columns straight off the swap row viaGET /swaps/:id.reconcileSwapbecomesPromise<void>— its job is the side-effect write.SwapReconciliationtype is removed.UpdateSwapStatusDtodropsactualBuyAmountCryptoBaseUnitsince the verifier owns that column now.Logs. Pulled the pipe-separated pattern into a
logVerificationhelper inverification/utils.ts. Stable column shape across all 13 verifiers that emit logs:Missing amounts render as
=noneso operators cangrep "fee=none"to find unverified swaps. Verifier-specific context (Thorchainmemo, Acrossstatus, PortalsfeeAmount) lives in theextrasarg.Type tightening.
CreateSwapDto.swapperName: string→SwapperName— catches typos in internal callers at compile time.affiliateFeeAsset.tsstrategy map is now keyed bySwapperNameenum, exhaustive over every variant (picks upArbitrumBridge/Debridge/Testwhich previously fell through tonullsilently).Cleanup. Dead variables that only fed the removed
detailsblocks removed:feeAmountUsd(Portals),referrerFeeUnits(Stonfi),fillTxnRef(Across).Testing
isAffiliateVerified/affiliateVerificationDetailsfromSwapStatusResponse(verified via grep across this repo and shapeshift/web — only consumers read them directly off the swap row)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor