Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: full wire-up #44

Merged
merged 99 commits into from
Feb 10, 2025
Merged

feat: full wire-up #44

merged 99 commits into from
Feb 10, 2025

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented Feb 4, 2025 •
edited
Loading

Description

Baseline:

  • Matches mocks UI
  • Use CF assets only
  • QR code wiring
  • react-hook-form form state
  • bech32 validation
  • two-way route<-> state binding with full state serialization
  • quote fetch
  • affiliate/fees fetching
  • fetch swap
  • swap fetch and open deposit channel
  • reflect deposit channel
  • swap status fetch/polling
  • swap states surfaced
  • swap complete state
  • fix rekt 0. input
  • Txids/links
  • ShapeShift fee/protocol fee surfacing
  • expiry dangerous address expires after warnings/checks
  • cleanup cursor bs

🇵🇱 :

  • Channel Id link
  • there should be a row with a summary of the buy amount
  • gray out secondary data to give some contrast to things
  • input screen assets buttons should be clickable
  • ensure rate used in status screen is swap rate and doesn't get polled from e.g coingecko
  • full PR pass/code cleanup

Issue

@gomesalexandre gomesalexandre changed the title feat: ui progression feat: ui progression/full wire-up Feb 4, 2025
@gomesalexandre gomesalexandre marked this pull request as ready for review February 7, 2025 11:03
gomesalexandre
gomesalexandre commented Feb 7, 2025
View reviewed changes
src/lib/const.ts Outdated Show resolved Hide resolved
0xApotheosis
0xApotheosis approved these changes Feb 10, 2025
View reviewed changes
Copy link
Member

@0xApotheosis 0xApotheosis left a comment

Choose a reason for hiding this comment

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

Fundamentally excellent. Nothing blocking considering we are still very much in WIP mode, but a number of comments to tackle either in this PR, or we can ticked for follow-ups.

In addition to the comments throughout the PR I noticed a few trivial things during functional testing:

  • Error handling needed (e.g. invalid address for ChainId causing a 400 etc)
  • Disable "Start transaction" when form not ready
  • Center "To get this" skeleton
  • The assets on the /status page (whilst waiting for a deposit) need to show the chainId/logo (I was super confused trying to remember if my quote was for Arbitrum ETH or Mainnet ETH)
  • We need a way to get back to the asset selection page after doing a swap, so the user can do another swap after their wonderful experience!

src/queries/chainflip/assets.ts Outdated Show resolved Hide resolved
src/queries/chainflip/assets.ts Outdated Show resolved Hide resolved
src/lib/validation.ts Outdated Show resolved Hide resolved
src/lib/const.ts Outdated Show resolved Hide resolved
src/lib/bignumber/bignumber.ts Outdated Show resolved Hide resolved
src/components/Status/Status.tsx Outdated Show resolved Hide resolved
src/components/Status/Status.tsx Show resolved Hide resolved
src/components/TradeInput.tsx
return fromBaseUnit(quote.egressAmountNative, buyAsset.precision)
}, [quote?.egressAmountNative, buyAsset])

const rate = useMemo(() => {
Copy link
Member

Choose a reason for hiding this comment

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

I might be missing something, but it's not clear to me why we need market data at all here.
We seem to use it to calculate the rate, but we already get that for free on the quote (estimatedPrice).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed we are, nice catch! Base implementation before wiring up cf data used Coingecko for rate... With that being said, I don't think we really need it anymore, the only time we will default to it would be:

  1. In empty state
image
  1. In an invalid sell amount state
image

I think both of these can and should be solved by proper empty states 🙏🏽

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Removal of rate consumption a915749

https://jam.dev/c/8afd03e3-5c14-46eb-bc27-4e8f972fafb0

  1. Hide rate when no quote and animate 0256fcf

https://jam.dev/c/26855140-f02b-4521-a1dc-0161df8c2b72

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI @0xApotheosis the queries still exist as we do need rate still for summing up various fees as USD in status route, but they're now fully removed in the input one!

og/src/components/Status/Status.tsx

Lines 307 to 344 in 8c0a9fd

// Collect feeAssetIds
const feeAssetIds = useMemo(() => {
if (!quote?.includedFees) return []
return [
...new Set(
quote.includedFees
.filter(fee => fee.type !== 'broker')
.map(fee => chainflipToAssetId[fee.asset])
.filter(Boolean),
),
]
}, [quote?.includedFees])
// Ensure we have market-data for all fee assets so we can sum em up
const feeMarketData = useQueries({
queries: feeAssetIds.map(assetId => ({
...reactQueries.marketData.byAssetId(assetId),
})),
})
// And finally, do sum em up
const protocolFeesFiat = useMemo(() => {
if (!quote?.includedFees) return '0'
return quote.includedFees
.filter(fee => fee.type !== 'broker')
.reduce((total, fee) => {
const assetId = chainflipToAssetId[fee.asset]
if (!assetId) return total
const marketData = feeMarketData[feeAssetIds.indexOf(assetId)]?.data
if (marketData?.price) {
return total.plus(bnOrZero(fee.amount).times(marketData.price))
}
return total
}, bnOrZero(0))
.toString()
}, [quote?.includedFees, feeMarketData, feeAssetIds])

src/components/TradeInput.tsx Show resolved Hide resolved
src/components/TradeInput.tsx Show resolved Hide resolved
@gomesalexandre gomesalexandre merged commit 86e0a02 into develop Feb 10, 2025
3 checks passed
@gomesalexandre gomesalexandre mentioned this pull request Feb 10, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xApotheosis 0xApotheosis 0xApotheosis approved these changes

@NeOMakinG NeOMakinG Awaiting requested review from NeOMakinG

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@gomesalexandre @0xApotheosis