feat: migrate limit orders to utilize redux for state management #8112

woodenfurniture · 2024-11-11T00:30:23Z

Description

Migrates the WIP limit orders feature to utilize redux for state management. As the inputs of spot and limit trades are basically identical and contain extremely high risk code, a higher-order slice pattern has been adopted to deduplicate the logic and minimise surface area.

Also includes 2 fixes suggested by @NeOMakinG in #8095:

Issue (if applicable)

closes #8111

Risk

High Risk PRs Require 2 approvals

High risk. The migration to use of a new design pattern for trades means that although the core logic is left totally untouched, we should review and test this with extreme caution.

What protocols, transaction types, wallets or contract interactions might be affected by this PR?

All trades for all protocols, specifically trade input (including receive address, slippage, everything).

Testing

⚠️ For trades, no change from prod should be observed.

A complete regression test of trades should be completed, including various inputs, configs, swappers, multi-hop, custom recipient, different accounts, etc.

Engineering

Operations

🏁 My feature is behind a flag and doesn't require operations testing (yet)

Screenshots (if applicable)

Swap via THORChain to manual receive address

Swap via LiFi to custom account number (#1) and custom slippage (1.5%)

Swap via CoW with custom slippage (1.5%)

Swap via 0x

gomesalexandre · 2024-11-11T09:56:33Z

src/state/slices/common/tradeInputBase/createTradeInputBaseSelectors.ts

+ */
+export const createTradeInputBaseSelectors = (sliceName: keyof ReduxState) => {
+  // Base selector to get the slice
+  const selectBaseSlice = (state: ReduxState) => state[sliceName] as TradeInputBaseState


ser that's turbo brain time 🧠

NeOMakinG

Pretty straighforward, there is no so much risks tbh it's basically taking the trade input slices and make it reusable so we don't duplicate things limit order logic (I'm not saying that it's not a big amount of work, nicely done @woodenfurniture 🐐 )

Small review, pending runtime testing that I'm going to do right now

NeOMakinG · 2024-11-15T09:44:04Z

src/components/MultiHopTrade/components/LimitOrder/hooks/useLimitOrderRecipientAddress.tsx

+    dispatch(limitOrderInput.actions.setManualReceiveAddress(undefined))
+  }, [dispatch])

  const handleEditManualReceiveAddress = useCallback(() => {
-    setIsManualReceiveAddressEditing(true)
-  }, [])
+    dispatch(limitOrderInput.actions.setIsManualReceiveAddressEditing(true))
+  }, [dispatch])

  const handleCancelManualReceiveAddress = useCallback(() => {
-    setIsManualReceiveAddressEditing(false)
+    dispatch(limitOrderInput.actions.setIsManualReceiveAddressEditing(false))
    // Reset form value and valid state on cancel so the valid check doesn't wrongly evaluate to false after bailing out of editing an invalid address
-    setIsManualReceiveAddressValid(undefined)
-  }, [])
+    dispatch(limitOrderInput.actions.setIsManualReceiveAddressValid(undefined))
+  }, [dispatch])

  const handleResetManualReceiveAddress = useCallback(() => {
    // Reset the manual receive address in store
-    setManualReceiveAddress(undefined)
+    dispatch(limitOrderInput.actions.setManualReceiveAddress(undefined))
    // Reset the valid state in store
-    setIsManualReceiveAddressValid(undefined)
-  }, [])
+    dispatch(limitOrderInput.actions.setIsManualReceiveAddressValid(undefined))
+  }, [dispatch])
+
+  const handleSubmitManualReceiveAddress = useCallback(
+    (address: string) => {
+      dispatch(limitOrderInput.actions.setManualReceiveAddress(address))
+      dispatch(limitOrderInput.actions.setIsManualReceiveAddressEditing(false))
+    },
+    [dispatch],
+  )
+
+  const handleIsManualReceiveAddressValidatingChange = useCallback(
+    (isValidating: boolean) => {
+      dispatch(limitOrderInput.actions.setIsManualReceiveAddressValidating(isValidating))
+    },
+    [dispatch],
+  )

-  const handleSubmitManualReceiveAddress = useCallback((address: string) => {
-    setManualReceiveAddress(address)
-    setIsManualReceiveAddressEditing(false)
-  }, [])
+  const handleIsManualReceiveAddressValidChange = useCallback(
+    (isValid: boolean) => {
+      dispatch(limitOrderInput.actions.setIsManualReceiveAddressValid(isValid))
+    },
+    [dispatch],
+  )


q: don't you want to use your new super hook?

OoOoOoooh actually yes will do for this one!

ebc7b62 🎉

NeOMakinG · 2024-11-15T09:46:16Z

src/components/MultiHopTrade/components/TradeInput/components/RecipientAddress.tsx

@@ -29,14 +29,14 @@ export const RecipientAddress = ({

  const handleIsValidatingChange = useCallback(
    (isValidating: boolean) => {
-      dispatch(tradeInput.actions.setManualReceiveAddressIsValidating(isValidating))
+      dispatch(tradeInput.actions.setIsManualReceiveAddressValidating(isValidating))


rubber: surely you didn't use your new hook pattern because you want to avoid too much diffs, what's the plan for these, should we plan to refactor them at some point or it will just be used in the future as a more convenient way?

Yes we could refactor them later, or when convenient. But yeah, avoided doing it here to minimise diff on this PR.

NeOMakinG · 2024-11-15T09:47:10Z

src/hooks/useActions.tsx

+  const dispatch = useDispatch()
+
+  return useMemo(() => bindActionCreators(actions, dispatch), [actions, dispatch])
+}


NeOMakinG · 2024-11-15T09:48:29Z

src/lib/fees/model.test.ts

    const tradeAmountUsd = bn(FEE_CURVE_NO_FEE_THRESHOLD_USD + 0.01)
    const foxHeld = bn(0)
+    const isSnapshotApiQueriesRejected = selectIsSnapshotApiQueriesRejected(store.getState())


very-nitpick: pretty sure you could do it in a beforeEach instead

NeOMakinG · 2024-11-15T09:49:46Z

src/lib/fees/model.ts

@@ -64,7 +69,7 @@ export const calculateFees: CalculateFeeBps = ({ tradeAmountUsd, foxHeld, feeMod
    new Date().getUTCFullYear() < THORSWAP_MAXIMUM_YEAR_TRESHOLD

  // failure to fetch fox discount results in free trades.
-  const isFallbackFees = selectIsSnapshotApiQueriesRejected(store.getState())
+  const isFallbackFees = isSnapshotApiQueriesRejected


rubber-duck: oh, now I get it, it's way saner like this from a functional programming perspective, did you change it for this reason or there were a limitation I can't find?

There were circular dependencies because this file was importing redux, but redux slice was import this file ♻️. To break the cycle I opted to just add a parameter

NeOMakinG

✅ THORChain => ARB

https://jam.dev/c/794b1c7b-ae13-446c-829a-158f3316a8ce

✅ Li.Fi Multi hop

https://jam.dev/c/13233bb7-e02b-4457-8fd3-ab62cd6bd93d
⚠️ I had a problem with the second hop but I think it's unrelated, can you confirm @woodenfurniture ?

✅ 0x single hop

https://jam.dev/c/2535485d-f872-4f3d-9359-1c4a54caaefb

✅ Portals swap with custom slippage (1.5%)

https://jam.dev/c/224ad78d-84df-40fc-bc18-ba402023114b

✅ Custom receive address

https://jam.dev/c/4a4765ad-f4c2-4ba1-b0e1-92b33153082f

General smoke test

Small visual issue, the centered content is sticked to the top (not blocking)
Learn more text is bigger than regular text

https://jam.dev/c/96b774bb-f0b8-46b3-9007-8c591cc9cb3d

Looks good to me!

woodenfurniture added the high risk label Nov 11, 2024

gomesalexandre reviewed Nov 11, 2024

View reviewed changes

woodenfurniture changed the base branch from develop to limit-orders-more-input-wiring November 11, 2024 21:19

woodenfurniture force-pushed the limit-orders-redux branch from 8f43289 to 56645b2 Compare November 12, 2024 22:37

woodenfurniture force-pushed the limit-orders-more-input-wiring branch from 756c0ee to 2c0ee18 Compare November 12, 2024 22:42

Base automatically changed from limit-orders-more-input-wiring to develop November 12, 2024 23:42

woodenfurniture force-pushed the limit-orders-redux branch from 56645b2 to 2b14c3b Compare November 12, 2024 23:53

woodenfurniture mentioned this pull request Nov 13, 2024

feat: limit orders quote wiring #8095

Merged

1 task

woodenfurniture marked this pull request as ready for review November 13, 2024 01:06

woodenfurniture requested a review from a team as a code owner November 13, 2024 01:06

NeOMakinG reviewed Nov 15, 2024

View reviewed changes

NeOMakinG approved these changes Nov 15, 2024

View reviewed changes

woodenfurniture added 18 commits November 16, 2024 08:44

feat: migrate tradeInputSlice to higher-order-slice pattern

8e0117c

feat: move common input selectors into a selector factory

f19057e

chore: add some code comments

7c03183

feat: added limitOrderInputSlice

33a4641

feat: migrate the rest of the shared selectors

2d77e62

feat: wire in redux user slippage for limit orders

3eafc9c

feat: wire in redux sell asset for limit orders

5dedd59

feat: wire in redux buy asset for limit orders

1f50150

fix: use hardcoded cowswap for default limit order slippage

bb7474d

feat: migrate marketPriceBuyAsset to redux

3f25226

feat: migrate limit order account IDs to redux

fc76235

feat: migrate limit order input amounts to redux

3539519

chore: move selectCalculatedFees into into snapshot selectors

f4597d9

chore: cleanup voting power selectors

ca39e07

feat: migrate limit order useLimitOrderRecipientAddress to redux

2e22e89

chore: cleanup slippage percentages in limit order input

e30f46a

feat: migrate remaining limit order input handlers to redux

9e5f647

feat: introduced useActions hook for max cleanliness

4d87276

woodenfurniture added 5 commits November 16, 2024 08:44

chore: unify naming of trade input actions and selectors

2e7b941

fix: free trades are free

ae28f8e

fix: reset market rate when switching assets

09c981d

fix: import from wrong slice, prevent it from happening again

0e3e789

chore: useActions for the useLimitOrderRecipientAddress hook

ebc7b62

woodenfurniture force-pushed the limit-orders-redux branch from 4c915c8 to ebc7b62 Compare November 15, 2024 21:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: migrate limit orders to utilize redux for state management #8112

feat: migrate limit orders to utilize redux for state management #8112

woodenfurniture commented Nov 11, 2024 •

edited

Loading

gomesalexandre Nov 11, 2024

NeOMakinG left a comment

NeOMakinG Nov 15, 2024

woodenfurniture Nov 15, 2024

woodenfurniture Nov 15, 2024

NeOMakinG Nov 15, 2024

woodenfurniture Nov 15, 2024

NeOMakinG Nov 15, 2024

NeOMakinG Nov 15, 2024

NeOMakinG Nov 15, 2024

woodenfurniture Nov 15, 2024

NeOMakinG left a comment

feat: migrate limit orders to utilize redux for state management #8112

Are you sure you want to change the base?

feat: migrate limit orders to utilize redux for state management #8112

Conversation

woodenfurniture commented Nov 11, 2024 • edited Loading

Description

Issue (if applicable)

Risk

Testing

Engineering

Operations

Screenshots (if applicable)

Choose a reason for hiding this comment

NeOMakinG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NeOMakinG left a comment

Choose a reason for hiding this comment

✅ THORChain => ARB

✅ Li.Fi Multi hop

✅ 0x single hop

✅ Portals swap with custom slippage (1.5%)

✅ Custom receive address

General smoke test

woodenfurniture commented Nov 11, 2024 •

edited

Loading