Sam/ramps #5760
Sam/ramps #5760
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
samholmes
force-pushed
the
sam/ramps
branch
7 times, most recently
from
981e4c4 to
ea62b8a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
src/hooks/useMarginRemStyle.ts
Outdated
| props: MarginRemProps, | ||
| // TODO: Remove this prop once all designs using this prop have been updated. | ||
| /** @deprecated Your design should expect the component to have 0.5rem margins */ | ||
| defaultRem: number = 0.5 |
There was a problem hiding this comment.
defaultRem shouldn't be configurable, should just be a constant
There was a problem hiding this comment.
I know, it's only there to not break other places where useMarginRemStyle is used. See the comment.
There was a problem hiding this comment.
What about {aroundRem: 0}?
There was a problem hiding this comment.
That would override the defaultRem (should be called fallbackRem actually).
There was a problem hiding this comment.
I mean pass {aroundRem: 0} at callsites like FilledTextInput if the marignRem props are not provided. FilledTextInput and others would perform the fallback logic. It will be uglier at the FilledTextInput, but then we don't need to even have this deprecation TODO to deal with in the common hook, and no one will mess up its usage in the future.
src/hooks/useMarginRemStyle.ts
Outdated
| // TODO: rename this hook to not be specific to margins because we want to | ||
| // our design system around these props |
| const { isNative, address } = asset | ||
|
|
||
| if (!asset.rampProducts?.includes(direction)) continue | ||
| if (asset.rampProducts?.includes(direction) !== true) continue |
There was a problem hiding this comment.
Bug: Asset Filtering Logic Error
The condition !asset.rampProducts?.includes(direction) was changed to asset.rampProducts?.includes(direction) !== true. This change is reported to alter the filtering logic for assets where rampProducts is undefined or null, as the two expressions are considered non-equivalent in this context.
There was a problem hiding this comment.
!asset.rampProducts?.includes(direction) means asset.rampProducts?.includes(direction) === false || asset.rampProducts?.includes(direction) == null, so the simplification of that is asset.rampProducts?.includes(direction) !== true.
| nativeAmount: '', | ||
| fiatAmount: '', | ||
| fieldChanged: fieldNum ? 'fiat' : 'crypto' | ||
| fieldChanged: fieldNum !== 0 ? 'fiat' : 'crypto' |
There was a problem hiding this comment.
Bug: Field Mapping Error Causes Incorrect Amount Tracking
The fieldChanged assignment was updated to use fieldNum !== 0 instead of fieldNum. This change inverts the field mapping when fieldNum is 0, causing fieldChanged to be 'fiat' instead of 'crypto' and potentially leading to incorrect amount change tracking.
There was a problem hiding this comment.
What?... The whole point is to fix the lint error. fieldNum !== 0 is what fieldNum meant before anyway.
| const dataMatch = /{{([^:]+):([^:]+)(?::([^}]+))?}}/.exec(data) | ||
|
|
||
| if (dataMatch) { | ||
| if (dataMatch != null) { |
There was a problem hiding this comment.
Bug: Redundant Null Check in Regex Execution
The if (dataMatch != null) check for the regex exec result is redundant. exec returns either a truthy array or falsy null, so if (dataMatch) is sufficient and aligns better with common patterns.
There was a problem hiding this comment.
It's just to fix the lint error.
| nativeAmount: '', | ||
| fiatAmount: '', | ||
| fieldChanged: fieldNum ? 'fiat' : 'crypto' | ||
| fieldChanged: fieldNum !== 0 ? 'fiat' : 'crypto' |
There was a problem hiding this comment.
Bug: Field Change Detection Fails For Negative Numbers
The fieldChanged assignment condition was updated from fieldNum ? to fieldNum !== 0 ?. This change from implicit truthiness to an explicit non-zero check raises concerns that it could lead to incorrect field change detection if fieldNum takes on values other than the expected 0 or 1, such as negative numbers.
| error != null || | ||
| (zeroString(spendInfo.spendTargets[0].nativeAmount) && | ||
| !SPECIAL_CURRENCY_INFO[pluginId].allowZeroTx) | ||
| getSpecialCurrencyInfo(pluginId).allowZeroTx !== true) |
There was a problem hiding this comment.
Bug: Slider Incorrectly Disabled for Truthy Values
The disableSlider condition for zero-amount transactions now uses allowZeroTx !== true instead of !allowZeroTx. This change incorrectly disables the slider when allowZeroTx is a truthy non-boolean value (e.g., 1), preventing transactions that should be allowed.
| } else { | ||
| directionChange = 'to' | ||
| amount = isMaxAmount ? amount : round(amount, CRYPTO_DECIMALS) | ||
| } |
There was a problem hiding this comment.
Bug: API Requests Use Incorrect Negative Precision
The round function uses negative precision values for both fiat and crypto amounts when preparing API requests. This causes amounts to round to the nearest hundred or hundred million, rather than to two or eight decimal places, leading to incorrect quote calculations.
| } | ||
|
|
||
| const flagUri = | ||
| countryData != null |
There was a problem hiding this comment.
Bug: SwapInput Tracks Wrong Field On Change
The SwapInput component's onAmountChanged callback incorrectly tracks which input field was modified. The change from fieldNum ? 'fiat' : 'crypto' to fieldNum !== 0 ? 'fiat' : 'crypto' reverses the intended mapping of fieldNum to the fieldChanged type, causing misidentification of whether the fiat or crypto amount was adjusted.
Additional Locations (1)
| assetAction: params.assetAction, | ||
| savedAction: params.savedAction | ||
| }) | ||
| } |
There was a problem hiding this comment.
Bug: Transaction Metadata Guard Fails for Native Sells
The saveTxAction call in both Moonpay and Paybis ramp plugins is incorrectly guarded by if (tokenId != null). This prevents transaction metadata from being saved for native cryptocurrency sells, where tokenId is null.
Additional Locations (1)
| }, | ||
| hiddenFeaturesMap: { | ||
| address: true | ||
| } |
There was a problem hiding this comment.
Bug: Deep Link Handler Error Handling Flaw
The deeplinkHandler in fiatPlugin.tsx can return void or a Promise<void>. If the handler returns void and throws a synchronous error, the error won't be caught by the subsequent .catch() call, potentially leading to unhandled exceptions.
Additional Locations (1)
2 participants
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneRequirements
If you have made any visual changes to the GUI. Make sure you have:
Note
Introduces a new Ramp Plugins architecture with multiple providers (Banxa, Bity, Moonpay, Paybis, Revolut, Simplex), new buy/sell flows and scenes, ramp deeplinks, persisted ramp settings, and supporting UI/utilities using React Query.
banxa,bity,moonpay,paybis,revolut,simplex.RampCreateScene,RampSelectOptionScene; integrate intobuyTab/sellTabwithBuySellTabParamList.pluginListBuyOld/pluginListSellOld.edge://ramp/...parsing andrampDeeplinkManager; keepfiatProviderhandling for backward compatibility.rampLastFiatCurrencyCodeandrampLastCryptoSelection; new actions to update.PillButton,DropdownInputButton,PaymentOptionCard,ErrorCard,BestRateBadge,ShimmerCard; enhanceAlertCardUi4,Shimmer.QueryClientProvider(React Query); add@tanstack/react-querydependency.secondaryButtonDisabled.DeepLinkingActionsandDeepLinkParserto support ramp links.verify).Written by Cursor Bugbot for commit 7131a73. This will update automatically on new commits. Configure here.