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

chore: swap refactor (#3731) #3799

Open
wants to merge 28 commits into
base: develop
Choose a base branch
from
Open

chore: swap refactor (#3731) #3799

wants to merge 28 commits into from

Conversation

stackchain
Copy link
Member

@stackchain stackchain commented Jan 27, 2025 •
edited
Loading

Description / Change(s) / Related issue(s)

Work in progress, don't commit to it

  • Dexhunter Sign endpoint
  • First full run attempt
  • Product changes to adapt to multiple aggregators, specially but not limited to Splits (multiple trades instead of only a single 1)
  • Add 100% tests coverage

@jorbuedo @stackchain
chore: swap refactor (#3731)
197b35c 
Co-authored-by: Juliano Lazzarotto <[email protected]>
@stackchain stackchain marked this pull request as ready for review January 27, 2025 11:57
stackchain
stackchain commented Jan 28, 2025
View reviewed changes
apps/wallet-mobile/src/features/Swap/SwapNavigator.tsx
return (
<KeyboardAvoidingView style={[styles.flex, styles.root]}>
<SafeAreaView edges={['bottom', 'left', 'right']} style={styles.flex}>
<Tab.Navigator
screenOptions={({route}) => ({
...defaultMaterialTopTabNavigationOptions(atoms, color),
...(isSearchBarVisible && {tabBarStyle: {height: 0}}),
tabBarLabel: route.name === 'token-swap' ? strings.tokenSwap : strings.orderSwap,
tabBarLabel: {'token-swap': strings.tokenSwap, orders: strings.orderSwap, 'manager-config': 'Manager'}[
Copy link
Member Author

Choose a reason for hiding this comment

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

  • Fix: according product requirements (in-progress)

stackchain
stackchain commented Jan 28, 2025
View reviewed changes
apps/wallet-mobile/src/features/Swap/SwapNavigator.tsx
name="orders"
getComponent={() => ListOrders}
/>
{isDev && <Tab.Screen listeners={listeners} name="manager-config" component={ManagerConfig} />}
Copy link
Member Author

Choose a reason for hiding this comment

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

ibid idem

stackchain
stackchain commented Jan 28, 2025
View reviewed changes
apps/wallet-mobile/src/features/Swap/common/helpers.ts Outdated
import {PRICE_IMPACT_HIGH_RISK, PRICE_IMPACT_MODERATE_RISK} from './constants'
import {SwapPriceImpactRisk} from './types'

/*
Copy link
Member Author

Choose a reason for hiding this comment

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

  • Chore: implement the update for it

stackchain
stackchain commented Jan 28, 2025
View reviewed changes
apps/wallet-mobile/src/features/Swap/useCases/CreateOrder/ProviderSplits.tsx Outdated

const swapForm = useSwap()

const dex = swapForm.estimate?.splits[0]?.dex
Copy link
Member Author

Choose a reason for hiding this comment

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

  • fix: update according to product design (in-progress)

stackchain
stackchain commented Jan 28, 2025
View reviewed changes
apps/wallet-mobile/src/features/Swap/useCases/ReviewSwap/ReviewSwap.tsx
Copy link
Member Author

Choose a reason for hiding this comment

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

@banklesss probably this should be replaced by the review tx.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for pointing this out @stackchain

This is a screen to review the swap order that is only in the swap funnel. It is considered in the specs but since it is something particular to the swap funnel I decided that it would depend on the swap funnel itself.

let me know if you think otherwise!

Copy link
Member Author

Choose a reason for hiding this comment

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

is more on the product end, I'd say if we can leverage the ReviewTx, we need to discuss with Thomas about using it as well for the TxDetail, it doesn't make sense anymore to display it that old weird way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it has more information about the swap, because we're creating it, than what can be got from the cbor, that's why it's separate. We can move it to ReviewTx as some "added info" I guess.

Copy link
Member Author

Choose a reason for hiding this comment

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

yap, I think @banklesss has it implemented already, he can update this part.

@stackchain stackchain force-pushed the refactor/swap-module branch from 5581914 to e3d7ab2 Compare January 28, 2025 16:26
jorbuedo and others added 22 commits January 29, 2025 12:04
@stackchain
Copy link
Member Author

@banklesss can you fix the conflicts, and update the tx review flow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@banklesss banklesss banklesss left review comments

@jorbuedo jorbuedo jorbuedo left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stackchain @jorbuedo @banklesss