Improve price fetching#603
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR improves the pricing API by adding a bundled price endpoint and enhancing error handling and input validation across both backend and frontend services.
- Introduces new types and a bundled endpoint for aggregating prices across providers.
- Refactors service error handling to use standardized custom error types.
- Updates input validation logic and front‑end querying to support the new endpoint.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| shared/src/endpoints/price.endpoints.ts | Added types for the bundled pricing endpoint. |
| frontend/src/services/api/price.service.ts | Added getAllPricesBundled and deprecated the older getAllPrices method. |
| frontend/src/sections/FeeComparison/priceProviders.tsx | Removed per‑provider query functions in favor of centralized bundled fetching. |
| frontend/src/sections/FeeComparison/FeeProviderRow/index.tsx | Removed onPriceFetched usage and adjusted price extraction from bundled results. |
| frontend/src/sections/FeeComparison/FeeComparisonTable/index.tsx | Replaced individual provider queries with a bundled query and recalculated provider prices. |
| frontend/src/constants/cache.ts | Introduced a new cache key for bundled prices. |
| api/src/api/services/*.(transak | moonpay |
| api/src/api/routes/v1/price.route.ts | Added a new route for the bundled endpoint and adjusted existing routes. |
| api/src/api/middlewares/validators.ts | Added bundled price input validation middleware. |
| api/src/api/controllers/price.controller.ts | Implemented controller logic for aggregating bundled provider prices. |
Comments suppressed due to low confidence (2)
frontend/src/sections/FeeComparison/FeeComparisonTable/index.tsx:39
- The variables 'sourceAssetSymbol' and 'targetAssetSymbol' are used in the queryKey but are not defined in this diff. Confirm if they should be replaced with already available values (such as those from useRampFormStore) or imported from elsewhere.
queryKey: [cacheKeys.allPrices, amount, sourceAssetSymbol, targetAssetSymbol, selectedNetwork],
api/src/api/middlewares/validators.ts:88
- Ensure that PriceEndpoints is properly imported in this file; otherwise, a ReferenceError may occur when accessing PriceEndpoints.isValidCryptoCurrency.
if (!fromCrypto || !PriceEndpoints.isValidCryptoCurrency(fromCrypto)) {
✅ Deploy Preview for pendulum-pay ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
|
||
| const response: PriceEndpoints.AllPricesResponse = {}; | ||
|
|
||
| results.forEach((result) => { |
There was a problem hiding this comment.
What do you think about changing .forEach to .map to indicate that we don't mutate the content of the results array? I think it is clearer and more understandable
There was a problem hiding this comment.
Hmm not sure. To me, .forEach doesn't indicate mutation. On the other hand .map indicates to me that I want to map results to something else and assign the new transformed value to a variable. Which we don't do here.
There was a problem hiding this comment.
Yes we are changing response not result. Actually I would use a for loop because to me is more explicit and the forEach won't await if there is any async inside, but it is not the case now, so all options should work I guess.
| } | ||
|
|
||
| let body: AlchemyPayResponse; | ||
| try { |
There was a problem hiding this comment.
I'm wondering, we use this let/try/catch pattern in many places in our app. I really don't like using let because the type can be reassigned and doesn't impose immutability. I think we should introduce a helper function like
type Success<T> = {
data: T;
error: null;
};
type Failure<E> = {
data: null;
error: E;
};
type Result<T, E = Error> = Success<T> | Failure<E>;
export async function tryCatch<T, E = Error>(
promise: Promise<T>,
): Promise<Result<T, E>> {
try {
const data = await promise;
return { data, error: null };
} catch (error) {
return { data: null, error: error as E };
}
}
to make the try/catch handling easier and clearer (maybe in another PR?)
const body = await tryCatch(response.json());
There was a problem hiding this comment.
I'm fine with refactoring it. We can refactor it using the neverthrow package but let's not do it on this PR.
| // Analyze error message for specific types | ||
| const lowerErrorMessage = errorMessage.toLowerCase(); | ||
| if ( | ||
| lowerErrorMessage.includes('invalid fiat currency') || |
There was a problem hiding this comment.
We could use here an enum instead of directly passing the strings, wdyt?
There was a problem hiding this comment.
True we could. Not sure if this would improve the readability though. We have to assume that all the code in transak.service.ts, moonpay.service.ts and alchemypay.service.ts is completely tailored to each service provider and we can't reuse code anyway. So maybe we just live with these strings in those files for now?
| retry: false, | ||
| }); | ||
| // Determine if there's an error from the result | ||
| const error = result?.status === 'rejected' ? result.reason : undefined; |
There was a problem hiding this comment.
There shouldn't be an error because we filter them on the const providerPrices = useMemo(() function right? Still, don't mind if we leave it as redundancy for a future change.
There was a problem hiding this comment.
No, there could be an error. The check/filtering happens on a different object that is used to show the best provider. The row itself gets an 'unchecked' response and it could contain an error.
This pull request introduces significant enhancements to the pricing API, focusing on error handling, input validation, and support for a new bundled endpoint that aggregates prices from multiple providers. The changes improve robustness, maintainability, and user experience by standardizing error responses, centralizing validation logic, and providing a comprehensive view of pricing data.
New Features
/allroute to fetch prices from all providers in a single request. This includes thegetAllPricesBundledcontroller andAllPricesResponsetype for consistent response formatting. [1] [2]Error Handling Improvements
InvalidAmountError,InvalidParameterError, andProviderInternalErrorto handle provider-specific and generic errors consistently across services. [1] [2] [3] [4]AlchemyPay,Moonpay, andTransakservices to use the new error types, log errors, and handle edge cases like JSON parsing failures and missing fields in responses. [1] [2] [3]Input Validation
validateBundledPriceInputmiddleware to validate query parameters for the new bundled endpoint, ensuring proper input before processing.API Refactoring
Miscellaneous
allPricesto support caching for the bundled endpoint in the frontend.