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.

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

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

Merged
merged 24 commits into from
Nov 20, 2024

Conversation

woodenfurniture
Copy link
Member

@woodenfurniture woodenfurniture commented Nov 11, 2024

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

@woodenfurniture woodenfurniture changed the base branch from develop to limit-orders-more-input-wiring November 11, 2024 21:19
Base automatically changed from limit-orders-more-input-wiring to develop November 12, 2024 23:42
@woodenfurniture woodenfurniture marked this pull request as ready for review November 13, 2024 01:06
@woodenfurniture woodenfurniture requested a review from a team as a code owner November 13, 2024 01:06
Copy link
Collaborator

@NeOMakinG NeOMakinG left a comment

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@NeOMakinG NeOMakinG left a comment

Choose a reason for hiding this comment

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

✅ 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

image

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!

Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

Big chunky boy but actually a pretty straightforward change conceptually, big 🐐 on the higher-order slice/selectors as a concept ser that's actually a perfect and does avoid an almost 1/1 slice/selectors duplication here!

Runtime pass to follow.

Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

@woodenfurniture woodenfurniture merged commit eb68f9c into develop Nov 20, 2024
1 check passed
@woodenfurniture woodenfurniture deleted the limit-orders-redux branch November 20, 2024 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redux slice for limit orders input
3 participants