Skip to content

Conversation

samholmes
Copy link
Contributor

@samholmes samholmes commented Aug 13, 2025

CHANGELOG

Does this branch warrant an entry to the CHANGELOG?

  • Yes
  • No

Dependencies

none

Requirements

If you have made any visual changes to the GUI. Make sure you have:

  • Tested on iOS device
  • Tested on Android device
  • Tested on small-screen device (iPod Touch)
  • Tested on large-screen device (tablet)
Simulator Screenshot - iPhone 15 Pro - 2025-08-13 at 12 37 57 Simulator Screenshot - iPhone 15 Pro - 2025-08-13 at 12 37 40 Simulator Screenshot - iPhone 15 Pro - 2025-08-13 at 12 28 37 Simulator Screenshot - iPhone 15 Pro - 2025-08-13 at 12 28 43 Simulator Screenshot - iPhone 15 Pro - 2025-08-13 at 12 28 46 Simulator Screenshot - iPhone 15 Pro - 2025-08-13 at 12 28 51

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
cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@Jon-edge
Copy link
Collaborator

Jon-edge commented Aug 15, 2025

image

I think there are better ways to start this flow. The UI/UX should always to the best of our ability make it obvious what to do without reading instructions on how to use the app.

There's only one thing to do in this part of the flow: to select the region. This entire flow can actually be gated by region selection. This is one aspect of our legacy flow that I liked.

If the country selection modal is dismissed, THEN we can expose a brief message on the scene:

"Select your region for personalized ways to buy crypto:" (personalized "options" sounds vague)
[Select your region]

The rest of the scenes similarly should make it obvious what to do for that particular step.

@Jon-edge
Copy link
Collaborator

Jon-edge commented Aug 15, 2025

image

The "Next" button is a moving tap target.

Anchor it to the bottom of the scene/top of the keyboard with KavButton.tsx. The initial implementation called for a full width button for KavButton, though I disagree with that design. This is a good opportunity to keep our UI unified and update it to typical button dimensions we use in the rest of our app.

@Jon-edge
Copy link
Collaborator

Jon-edge commented Aug 15, 2025

image

Is this always the exact rate? If it's a variable rate scenario, we should be sure to represent that with a "≈" sign

AGENTS.md Outdated
@@ -1,5 +1,16 @@
# Edge React GUI - Agent Guidelines

> **⚠️ IMPORTANT: Learning and Documentation**
>
> **ALWAYS search for relevant documentation first** before starting any task that lacks sufficient context:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"before starting any task that lacks sufficient context:"

This will likely be a noop. Agents (at least in my experience) can rarely tell whether they have sufficient information. This sentence just adds extra thinking time.

I see two distinct sections here that make this a cleaner thinking process

# Initialization
**ALWAYS ensure `docs/` is in context before beginning a task. If not, **Use `find docs/ -name "*.md" -type f`** to recursively list all `.md` files in `docs/` folder to get an index of available documentation, then **Read relevant docs** to understand existing conventions, patterns, and business logic.

# Workflow
...rest of the items

@@ -359,3 +359,117 @@ export const closestRateForTimestamp = (
}
return bestRate
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just do a single call to getHistoricalRate but using the current date. This removes all this code

const checkAssetSupport = (
direction: FiatDirection,
fiatCurrencyCode: string,
cryptoPluginId: string,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just access an EdgeAsset instead of separate pluginId,tokenId

// Check if crypto is supported
const paybisCc =
EDGE_TO_PAYBIS_CURRENCY_MAP[`${cryptoPluginId}_${tokenId ?? ''}`]
if (!paybisCc) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did ai write this? need explicit null check

request: RampQuoteRequest
): Promise<RampQuoteResult[]> => {
await ensureStateInitialized()
if (!state) throw new Error('Plugin state not initialized')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did AI write this? need explicit null check

amountType: lastUsedInput,
direction: 'buy',
regionCode: {
countryCode: countryCode || 'US',
Copy link
Collaborator

@Jon-edge Jon-edge Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

countryCode ?? US


// Calculate exchange rate from best quote
const quoteExchangeRate = React.useMemo(() => {
if (!bestQuote?.cryptoAmount || !bestQuote.fiatAmount) return 0
Copy link
Collaborator

@Jon-edge Jon-edge Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (bestQuote?.cryptoAmount == null || bestQuote.fiatAmount == null)

headerTitle={lstrings.buy_crypto}
headerTitleChildren={
<PillButton
icon={() =>
Copy link
Collaborator

@Jon-edge Jon-edge Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

icon={renderRegionIcon}


interface Props extends BuyTabSceneProps<'pluginListBuy'> {}

export const TradeCreateScene = (props: Props): React.ReactElement => {
Copy link
Collaborator

@Jon-edge Jon-edge Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export const TradeCreateScene: React.FC<Props> = (props: Props) => {

displayName: selectedQuote.pluginDisplayName,
icon: { uri: selectedQuote.partnerIcon }
}}
renderRight={isBestRate ? () => <BestRateBadge /> : undefined}
Copy link
Collaborator

@Jon-edge Jon-edge Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renderRight={renderBestRate}

icon: { uri: selectedQuote.partnerIcon }
}}
renderRight={isBestRate ? () => <BestRateBadge /> : undefined}
onPress={async () => {
Copy link
Collaborator

@Jon-edge Jon-edge Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onPress={handlePress}


interface Props extends BuyTabSceneProps<'rampSelectOption'> {}

export const TradeOptionSelectScene = (props: Props): React.JSX.Element => {
Copy link
Collaborator

@Jon-edge Jon-edge Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export const TradeOptionSelectScene: React.FC<Props> = (props: Props) => {

onProviderPress: () => Promise<void> | void
}

export const PaymentOptionCard = (props: Props): React.JSX.Element => {
Copy link
Collaborator

@Jon-edge Jon-edge Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export const PaymentOptionCard: React.FC<Props> = (props: Props) => {

disabled?: boolean
}

export const PillButton = (props: PillButtonProps): React.ReactElement => {
Copy link
Collaborator

@Jon-edge Jon-edge Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export const PillButton: React.FC<PillButtonProps> = (props: PillButtonProps) => {

testID?: string
}

export const DropDownInputButton = (
Copy link
Collaborator

@Jon-edge Jon-edge Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export const DropDownInputButton: React.FC<DropDownInputButtonProps> = (props: DropDownInputButtonProps) => {

import { EdgeText } from '../themed/EdgeText'

// TODO: Render a badge icon
export const BestRateBadge = () => {
Copy link
Collaborator

@Jon-edge Jon-edge Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export const BestRateBadge: React.FC = () => {

.filter(check => check.supported)
.map(check => check.plugin)
},
enabled: Boolean(
Copy link
Collaborator

@Jon-edge Jon-edge Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enabled: (selectedWallet != null && selectedCrypto != null && selectedFiatCurrencyCode != null && regionCode != null),

fiatProviderDeeplinkHandler(link)
// Try ramp deeplink handler first
try {
rampDeeplinkManager.handleDeeplink(link)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const handled = rampDeeplinkManager.handleDeeplink(link) if (!handled) fiatProviderDeeplinkHandler(link)

this.listener = null
}

handleDeeplink(link: FiatProviderLink): void {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handleDeeplink(link: FiatProviderLink): boolean {
  if (this.listener == null) return false
  const { direction, providerId, deeplinkHandler } = this.listener
  if (link.providerId !== providerId) return false
  if (link.direction !== direction) return false
  if (Platform.OS === 'ios') SafariView.dismiss()
  this.unregister()
  deeplinkHandler(link)
  return true
}

}
}

const privateKey = atob(state!.privateKeyB64)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const privateKey = Buffer.from(state!.privateKeyB64, 'base64').toString('binary')


{/* Bottom Input (Crypto by design) */}
<InputRow>
<DropDownInputButton onPress={handleCryptDropdown}>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid empty uri image source.
{selectedCrypto == null || selectedWallet == null ? null : ( <CryptoIcon sizeRem={1.5} pluginId={selectedWallet.currencyInfo.pluginId} tokenId={selectedCrypto.tokenId} /> )}

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
fixup! Add learning technique to AGENTS.md

Restructure into Initialization and Workflow sections for clarity
Comment on lines 359 to 381
**Before:**
```typescript
const assets = await provider.getSupportedAssets({
direction: 'buy',
paymentTypes: ['credit', 'bank'],
regionCode: { countryCode: 'US', stateCode: 'CA' }
})
```

**After:**
```typescript
const assets = await plugin.getSupportedAssets({
direction: 'buy',
paymentTypes: ['credit', 'bank'],
regionCode: { countryCode: 'US', stateCode: 'CA' }
})
```

The API remains the same, maintaining compatibility with existing code. Note that the method returns the asset map for the first supported payment type from the provided array.

## Removing getSupportedAssets

The ramp plugin architecture has been simplified by removing the separate `getSupportedAssets` method. All support checking is now done within the `fetchQuote` method.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like conflicting information around getSupportedAssets

}: UseSupportedPluginsParams): UseSupportedPluginsResult => {
// Build region code
const regionCode: FiatPluginRegionCode | undefined = React.useMemo(() => {
if (!countryCode) return undefined
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

countryCode == null


return {
supportedPlugins,
isLoading,
Copy link
Collaborator

@Jon-edge Jon-edge Sep 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't seem to be doing anything with isLoading? Should be true while we're loading

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used in the TradeCreateScene

Comment on lines +247 to +248
const rateA = parseFloat(a.fiatAmount) / parseFloat(a.cryptoAmount)
const rateB = parseFloat(b.fiatAmount) / parseFloat(b.cryptoAmount)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No guards against 0.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not addressed

@Jon-edge
Copy link
Collaborator

Jon-edge commented Sep 1, 2025

"Fix all non deprecation lint warnings in all changed files"

Useful rule:
"After completing code changes, run yarn tsc and fix any errors. Once those are addressed, run files=($(git diff HEAD --name-only -- '*.ts' '*.tsx')); if (( ${#files} )); then ./node_modules/.bin/eslint --fix "${files[@]}"; else echo "No TS/TSX changes since HEAD."; fi, and manually fix any non-deprecation warnings that cannot be automatically fixed, if necessary. It is not necessary to fix any lint warnings/errors on files that you did not modify yourself."


try {
return (
parseFloat(bestQuote.fiatAmount) / parseFloat(bestQuote.cryptoAmount)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No guard for div by 0

const rateA = parseFloat(a.fiatAmount) / parseFloat(a.cryptoAmount)
const rateB = parseFloat(b.fiatAmount) / parseFloat(b.cryptoAmount)
return rateA - rateB
})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Zero Crypto Amount Causes Quote Sorting Failure

The quote sorting logic in useRampQuotes and TradeOptionSelectScene divides by cryptoAmount without handling cases where it's zero or invalid. This can produce NaN or Infinity rates, breaking the sort comparison and leading to incorrect quote ordering.

Additional Locations (1)

Fix in Cursor Fix in Web

Copy link
Collaborator

@Jon-edge Jon-edge Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

@Jon-edge Jon-edge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all divide by 0 comments are addressed

Comment on lines +247 to +248
const rateA = parseFloat(a.fiatAmount) / parseFloat(a.cryptoAmount)
const rateB = parseFloat(b.fiatAmount) / parseFloat(b.cryptoAmount)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not addressed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants