Unifying DeFi errors to a common format#393
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 1 minute and 31 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThe PR removes the protected abstract ChangesDeFi Error Handling Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/walletkit/src/defi/crypto-onramp/CryptoOnrampManager.ts (1)
102-120:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve provider-selection errors instead of collapsing them into
DepositFailed.Line [103] can throw
DefiError(NoDefaultProvider/ProviderNotFound) fromDefiManager.getProvider, but Lines [116]-[120] currently wrap that asCryptoOnrampErrorCode.DepositFailed. This masks the real failure mode and breaks downstream error mapping behavior.Suggested fix
- try { - const deposit = await this.getProvider(selectedProviderId).createDeposit(params); + const provider = this.getProvider(selectedProviderId); + try { + const deposit = await provider.createDeposit(params); @@ } catch (error) { log.error('Failed to create crypto onramp deposit', { error, params }); if (error instanceof CryptoOnrampError) throw error; throw new CryptoOnrampError( 'Failed to create crypto onramp deposit', CryptoOnrampErrorCode.DepositFailed, error, ); }🤖 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 `@packages/walletkit/src/defi/crypto-onramp/CryptoOnrampManager.ts` around lines 102 - 120, The catch block in the createDeposit method is wrapping all errors as DepositFailed, but it should preserve provider-selection errors from getProvider. Add a check after the CryptoOnrampError instanceof check to detect if the error is a DefiError (which can be thrown from getProvider when it cannot find the provider), and re-throw that error as-is without wrapping it. Only wrap the error as DepositFailed when it is neither a CryptoOnrampError nor a DefiError, ensuring that provider-selection failures maintain their original error type for downstream error mapping.
🤖 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.
Inline comments:
In `@packages/walletkit/src/defi/gasless/GaslessManager.spec.ts`:
- Around line 113-118: The test for the setDefaultProvider method only verifies
that a DefiError is thrown, but does not assert the specific error code. Update
the expect statement to check for both the error class and the specific
ProviderNotFound error code or error message to ensure the correct error
semantics are maintained. Use a more specific assertion pattern (such as
checking the error code property or error message) alongside the DefiError class
check in the setDefaultProvider test.
In `@packages/walletkit/src/defi/staking/StakingManager.ts`:
- Around line 49-50: The code is collapsing all caught errors into
StakingErrorCode.InvalidParams across multiple methods, losing the original
error semantics. In all five error handling blocks (the catch block around line
49-50, the blocks at lines 63-67, 90-95, 112, and 130-134), replace the blanket
InvalidParams normalization with conditional error handling: if the caught error
is already a StakingError, re-throw it unchanged; if it's a DefiError from
getProvider, map it to an appropriate StakingErrorCode; only use InvalidParams
when the error is actually due to invalid parameters. This pattern should be
applied consistently across all catch blocks in the affected methods to preserve
actionable error semantics for callers.
---
Outside diff comments:
In `@packages/walletkit/src/defi/crypto-onramp/CryptoOnrampManager.ts`:
- Around line 102-120: The catch block in the createDeposit method is wrapping
all errors as DepositFailed, but it should preserve provider-selection errors
from getProvider. Add a check after the CryptoOnrampError instanceof check to
detect if the error is a DefiError (which can be thrown from getProvider when it
cannot find the provider), and re-throw that error as-is without wrapping it.
Only wrap the error as DepositFailed when it is neither a CryptoOnrampError nor
a DefiError, ensuring that provider-selection failures maintain their original
error type for downstream error mapping.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23c76cc9-27b8-450d-b8b6-95ea5a62ef7c
📒 Files selected for processing (9)
packages/appkit-react/src/features/onramp/widgets/crypto-onramp/utils/map-crypto-onramp-error.tspackages/appkit-react/src/locales/en.tspackages/walletkit/src/defi/DefiManager.tspackages/walletkit/src/defi/crypto-onramp/CryptoOnrampManager.tspackages/walletkit/src/defi/crypto-onramp/errors.tspackages/walletkit/src/defi/gasless/GaslessManager.spec.tspackages/walletkit/src/defi/gasless/GaslessManager.tspackages/walletkit/src/defi/staking/StakingManager.tspackages/walletkit/src/defi/swap/SwapManager.ts
💤 Files with no reviewable changes (2)
- packages/walletkit/src/defi/gasless/GaslessManager.ts
- packages/walletkit/src/defi/swap/SwapManager.ts
| it('throws DefiError when the providerId is not registered', () => { | ||
| const { manager } = makeManager(); | ||
| manager.registerProvider(makeProvider('first')); | ||
|
|
||
| expect(() => manager.setDefaultProvider('missing')).toThrow(GaslessError); | ||
| expect(() => manager.setDefaultProvider('missing')).toThrow(DefiError); | ||
| }); |
There was a problem hiding this comment.
Assert the specific ProviderNotFound behavior, not just the error class.
Line [117] only checks DefiError, which can hide regressions in error-code semantics for this path.
Suggested test tightening
-import { DefiError } from '../errors';
+import { DefiError, DefiErrorCode } from '../errors';
@@
- expect(() => manager.setDefaultProvider('missing')).toThrow(DefiError);
+ expect(() => manager.setDefaultProvider('missing')).toThrowError(
+ expect.objectContaining({ name: 'DefiError', code: DefiErrorCode.ProviderNotFound }),
+ );As per coding guidelines, "Test behaviors, not implementation details. Always update existing tests related to your changes."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('throws DefiError when the providerId is not registered', () => { | |
| const { manager } = makeManager(); | |
| manager.registerProvider(makeProvider('first')); | |
| expect(() => manager.setDefaultProvider('missing')).toThrow(GaslessError); | |
| expect(() => manager.setDefaultProvider('missing')).toThrow(DefiError); | |
| }); | |
| it('throws DefiError when the providerId is not registered', () => { | |
| const { manager } = makeManager(); | |
| manager.registerProvider(makeProvider('first')); | |
| expect(() => manager.setDefaultProvider('missing')).toThrowError( | |
| expect.objectContaining({ name: 'DefiError', code: DefiErrorCode.ProviderNotFound }), | |
| ); | |
| }); |
🤖 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 `@packages/walletkit/src/defi/gasless/GaslessManager.spec.ts` around lines 113
- 118, The test for the setDefaultProvider method only verifies that a DefiError
is thrown, but does not assert the specific error code. Update the expect
statement to check for both the error class and the specific ProviderNotFound
error code or error message to ensure the correct error semantics are
maintained. Use a more specific assertion pattern (such as checking the error
code property or error message) alongside the DefiError class check in the
setDefaultProvider test.
Source: Coding guidelines
| throw new StakingError('Failed to get staking quote', StakingErrorCode.InvalidParams, { error, params }); | ||
| } |
There was a problem hiding this comment.
Do not normalize all failures to StakingErrorCode.InvalidParams.
Lines [49], [63]-[67], [90]-[95], [112], and [130]-[134] currently convert every caught error (including DefiError from getProvider and existing typed staking/provider errors) into InvalidParams. That erases actionable error semantics for callers and UI mapping.
Suggested pattern (apply across all five methods)
+import { DefiError } from '../errors';
@@
- try {
- const quote = await this.getProvider(providerId).getQuote(params);
+ const provider = this.getProvider(providerId);
+ try {
+ const quote = await provider.getQuote(params);
log.debug('Received staking quote', quote);
return quote;
} catch (error) {
+ if (error instanceof DefiError || error instanceof StakingError) throw error;
throw new StakingError('Failed to get staking quote', StakingErrorCode.InvalidParams, { error, params });
}Also applies to: 63-67, 90-95, 112-113, 130-134
🤖 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 `@packages/walletkit/src/defi/staking/StakingManager.ts` around lines 49 - 50,
The code is collapsing all caught errors into StakingErrorCode.InvalidParams
across multiple methods, losing the original error semantics. In all five error
handling blocks (the catch block around line 49-50, the blocks at lines 63-67,
90-95, 112, and 130-134), replace the blanket InvalidParams normalization with
conditional error handling: if the caught error is already a StakingError,
re-throw it unchanged; if it's a DefiError from getProvider, map it to an
appropriate StakingErrorCode; only use InvalidParams when the error is actually
due to invalid parameters. This pattern should be applied consistently across
all catch blocks in the affected methods to preserve actionable error semantics
for callers.
heyllog
changed the title
2 participants
Summary by CodeRabbit