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

chore: refactor multi-hop state tracking to avoid duplicate states and associated logic #5745

Merged
merged 2 commits into from
Dec 4, 2023

Conversation

woodenfurniture
Copy link
Member

@woodenfurniture woodenfurniture commented Dec 1, 2023

Description

Refactors the state management of multi-hop trade execution to remove duplicate and redundant states and the resulting logic required to keep them aligned. As a result, there is less responsibility of the view layer to manage and know about the working of trades at a higher level and allows them to dispatch narrow-context actions to redux.

  • State is deduplicated to ensure state is unique and purpose specific
  • State is nested to ensure the scope of each layer of nesting is easy to track and optional parts of trades (e.g the second hop and approvals) can be skipped without convoluted logic
    • callbacks to track error states of hooks owned by child components are now removed
    • state changes are dispatched on a per-step basis rather than using blanket incrementation, which simplifies logic at the cost of verbosity
  • The numerous new redux actions ensure state is consistent and integrity is maintained to avoid corrupted state

Pull Request Type

  • 🐛 Bug fix (Non-breaking Change: Fixes an issue)
  • 🛠️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
  • 💅 New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

closes #5740

Risk

All of the work here is focused on multi-hop trades (behind feature flag), but there is a risk these changes could affect the functionality of the production trade and approval state management.

Testing

Test trades and erc20 approvals are still working as expected.

Engineering

Operations

Screenshots (if applicable)

Multi-hop UI still working correctly with mock hooks
image

@woodenfurniture
Copy link
Member Author

woodenfurniture commented Dec 1, 2023

@woodenfurniture woodenfurniture force-pushed the multi-hop-improved-state-tracking branch from 6b70f3d to 04f57ac Compare December 4, 2023 00:14
@woodenfurniture woodenfurniture marked this pull request as ready for review December 4, 2023 00:33
@woodenfurniture woodenfurniture requested a review from a team as a code owner December 4, 2023 00:33
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.

First pass, looks excellent and more declarative - will stamp after a functional regression pass

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.

Tested locally:

  • Confirmed approval checks are still happy with allowance granted
image image
  • Confirmed approval checks are still happy without allowance granted
image image image
  • EVM trades are going through
image image

@woodenfurniture woodenfurniture force-pushed the multi-hop-improved-state-tracking branch from 04f57ac to 36b94d9 Compare December 4, 2023 19:58
@woodenfurniture woodenfurniture enabled auto-merge (squash) December 4, 2023 19:59
@woodenfurniture woodenfurniture merged commit 4bdeab8 into develop Dec 4, 2023
6 checks passed
@woodenfurniture woodenfurniture deleted the multi-hop-improved-state-tracking branch December 4, 2023 20:04
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.

Isolate state tracking of multi-hop trades into discrete state machines
2 participants