-
Notifications
You must be signed in to change notification settings - Fork 273
Sam/all ramp plugins #5697
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sam/all ramp plugins #5697
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with the other PR, address non-deprecation warnings.
Really simple hands off task with gpt-5-fast
in Cursor if claude+opencode is struggling for some reason on this.
Also, try to trim down the logging noise if possible.
walletAddress == null ? '' : `&walletAddress=${walletAddress}` | ||
|
||
if (direction === 'buy') { | ||
url = `https://api.moonpay.com/v3/currencies/${cryptoCurrencyObj.code}/buy_quote/?apiKey=${apiKey}"eCurrencyCode=${cryptoCurrencyObj.code}&baseCurrencyCode=${fiatCode}&paymentMethod=${paymentMethod}&areFeesIncluded=true&${amountParam}${walletAddressParam}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make https://api.moonpay.com/v3/currencies/
a constant at the top of the file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add to infoOptions for the provider so we can swap out the URL for dev/e2e testing on mock servers/sandboxes/prod (meastro)
coin: BanxaCryptoCoin, | ||
allowedCurrencyCodes: Record<FiatDirection, FiatProviderAssetMap> | ||
): void => { | ||
const wallet = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be using account.currencyConfig[pluginId].allTokens. We might not have
the wallet for a supported asset.
if (!checkMinMax(exchangeAmount, paymentObj)) { | ||
if (gt(exchangeAmount, paymentObj.max)) { | ||
console.warn( | ||
`Banxa: ${paymentType} over limit for ${fiatCode}: ${exchangeAmount} > ${paymentObj.max}` | ||
) | ||
} else if (lt(exchangeAmount, paymentObj.min)) { | ||
console.warn( | ||
`Banxa: ${paymentType} under limit for ${fiatCode}: ${exchangeAmount} < ${paymentObj.min}` | ||
) | ||
} | ||
continue | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original provider threw min/max errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One commit to fix all cases of fetchQuote not throwing when it should!
? bityQuote.output.minimum_amount | ||
: bityQuote.input.minimum_amount | ||
if (minimumAmount != null && lt(amount, minimumAmount)) { | ||
// Under minimum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throw the appropriate FiatProviderError
if (amountType === 'fiat') { | ||
if (gt(exchangeAmount, '1000')) { | ||
// Over limit | ||
console.error( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throw the appropriate FiatProviderError
if (lt(kBityQuote.output.amount, exchangeAmount)) { | ||
// Over limit | ||
console.error( | ||
'Bity fetchQuote error: Buy crypto amount exceeds 1000 fiat equivalent', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throw the appropriate FiatProviderError
if (lt(kBityQuote.input.amount, exchangeAmount)) { | ||
// Over limit | ||
console.error( | ||
'Bity fetchQuote error: Sell crypto amount exceeds 1000 fiat equivalent', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throw the appropriate FiatProviderError
0d0138a
to
37c5bde
Compare
This is an attempt to get the agent to automatically document lessons learned to the docs/ directory. This way conventions are maintained and over time as the agent is course corrected.
- Create docs/localization-guidelines.md with mandatory UI string localization rules - Create docs/component-styling-guidelines.md with styled HOC usage patterns - Update AGENTS.md with Documentation section indexing all docs/ files - Add rule requiring all docs/ markdown files to be indexed in AGENTS.md - Add localized strings for TradeRegionSelectScene to en_US.ts
37c5bde
to
212ac91
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codex Review: Here are some suggestions.
Reply with @codex fix comments
to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
const config = { | ||
initOptions, | ||
store, | ||
account, | ||
navigation: null as any, // Navigation will be provided by components that need it | ||
onLogEvent: () => {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P0] Pass real navigation to ramp plugins
Every ramp plugin is instantiated with navigation: null
in useRampPlugins
, yet the plugin implementations call navigation.navigate
/pop
during quote approval (for example banxaRampPlugin
around its approveQuote
handler). When a user taps a quote the plugin will dereference null
and crash before any webview or approval flow can start. The hook should supply a real NavigationBase
or the plugins must guard against navigation
being absent; otherwise no ramp purchase/sell flow can complete.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a valid bug
This file contains run configuration such as RN_SIMULATOR and RN_PORT to be used by a rn-ios script. This is for development purposes
This is a consistent card design for the new fiat buy UI.
- Implement TradeCreateScene and TradeOptionSelectScene for buy/sell flow - Add Paybis as first ramp plugin with full API integration - Create reusable hooks for ramp plugin management (useRampPlugins, useRampQuotes) - Add payment type icon system with comprehensive mappings - Implement quote fetching and comparison across multiple providers - Add best rate badge component for quote comparison - Create ramp plugin type definitions and store utilities - Add comprehensive documentation for migration and architecture - Include unit tests for payment types and store IDs - Update navigation and deeplink handlers for ramp flows BREAKING CHANGE: Replaces legacy FiatPluginUi with new ramp plugin system
Include plugin IDs in React Query key so quotes refetch when region/currency/wallet changes affect available plugins. Simplify queryFn to always check current plugins while reusing valid cached quotes.
d6f1032
to
0793a2c
Compare
const openWebView = async (): Promise<void> => { | ||
await new Promise<void>((resolve, reject) => { | ||
navigation.navigate('guiPluginWebView', { | ||
url: urlObj.href, | ||
onUrlChange: async (uri: string): Promise<void> => { | ||
console.log('Moonpay WebView url change: ' + uri) | ||
|
||
if (uri.startsWith(RETURN_URL_PAYMENT)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can crash since navigation
can be null
const openWebView = async (): Promise<void> => { | ||
navigation.navigate('guiPluginWebView', { | ||
url: webviewUrl, | ||
onUrlChange: newUrl => { | ||
handleUrlChange(newUrl).catch(showError) | ||
} | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the other comment, this can crash due to navigation being null
const handleQuotePress = async (quote: RampQuoteResult): Promise<void> => { | ||
try { | ||
await quote.approveQuote({ | ||
coreWallet: rampQuoteRequest.wallet! | ||
}) | ||
} catch (error) { | ||
console.error('Failed to approve quote:', error) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to inject navigation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to fix the navigation issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codex fix comments
For now, I can only help with PRs you've created. |
Replaced by #5740 |
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
#5691
Requirements
If you have made any visual changes to the GUI. Make sure you have: