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: clean up of trade related slices and selectors #6035

Merged
merged 10 commits into from
Jan 19, 2024
Merged

chore: clean up of trade related slices and selectors #6035

merged 10 commits into from
Jan 19, 2024

Conversation

woodenfurniture
Copy link
Member

@woodenfurniture woodenfurniture commented Jan 17, 2024 •
edited
Loading

Description

All 'round cleanup of slices and selectors related to trading:

  • combines swapperApi and swappersApi into swapperApi
  • renames swappersSlice to tradeInputSlice as it manages all state related to trade input parameters
  • improves and dedupes naming of selectors to prevent misuse
    • otherwise duplicate selectors for trade input params use *Input* vernacular
    • otherwise duplicate selectors for trade quote dat use *Quote* vernacular
  • Moves all accountId selectors into tradeInputSlice
  • Removed dead code
  • Fixed 1 non-consequential instance of selector used from wrong slice (due to duplicate naming)

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 #5742

Risk

High Risk PRs Require 2 approvals

High risk. Refactors selectors on one of our most impactful parts of our platform.

Testing

Compare to production, as there should be zero functoinal change introduce with this PR:

  • quotes
  • trades
  • account selection in trade input
  • funds land in selected account
  • quote amounts
  • slippage

Engineering

Operations

Screenshots (if applicable)

@woodenfurniture
Copy link
Member Author

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@woodenfurniture woodenfurniture marked this pull request as ready for review January 17, 2024 22:17
@woodenfurniture woodenfurniture requested a review from a team as a code owner January 17, 2024 22:17
gomesalexandre
gomesalexandre reviewed Jan 18, 2024
View reviewed changes
src/state/apis/swapper/swapperApi.ts
'getIsTradingActiveApi: error getting trading status',
)

export const GET_TRADE_QUOTE_POLLING_INTERVAL = 20_000
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nitpicky but move somewhere at the very top or to a constant file

gomesalexandre
gomesalexandre reviewed Jan 18, 2024
View reviewed changes
src/state/apis/swapper/swapperApi.ts

// hydrate crypto market data for buy and sell assets
await dispatch(
marketApi.endpoints.findByAssetIds.initiate([sellAsset.assetId, buyAsset.assetId]),
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nitpicky and not introduced by this PR but could pass an object instead so we don't ever get in a situation where we pass these in the wrong order

gomesalexandre
gomesalexandre reviewed Jan 18, 2024
View reviewed changes
src/state/apis/swapper/swapperApi.ts
Comment on lines +112 to +113
// this should never be needed but here for paranoia
if (!sellAssetUsdRate) throw Error('missing sellAssetUsdRate')
Copy link
Contributor

Choose a reason for hiding this comment

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

💜

gomesalexandre
gomesalexandre reviewed Jan 18, 2024
View reviewed changes
src/state/apis/swapper/swapperApi.ts
unorderedQuotes.filter(({ errors }) => errors.length > 0),
happyQuotes.length,
)
const orderedQuotes: ApiQuote[] = [...happyQuotes, ...errorQuotes]
Copy link
Contributor

Choose a reason for hiding this comment

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

Not introduced by this PR but concat if we feel like it, doesn't really matter but OCD

gomesalexandre
gomesalexandre reviewed Jan 18, 2024
View reviewed changes
src/state/slices/tradeQuoteSlice/selectors.ts
Comment on lines +429 to 431
selectQuoteSellAmountCryptoPrecision,
selectInputSellAssetUsdRate,
(sellAmountCryptoPrecision, sellAssetUsdRate) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can reflect the vernacular input/quote in the callback if we want to, to signify the intent that this is intended and not a mistake?

gomesalexandre
gomesalexandre reviewed Jan 18, 2024
View reviewed changes
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.

Very first pass - didn't notice anything that seems obviously wrong here. To be followed-up with extensive runtime testing

gomesalexandre
gomesalexandre approved these changes Jan 18, 2024
View reviewed changes
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.

Note, this has been tested locally against develop (which had the dangerous sell amount warning fix at the time of testing), not prod. Haven't noticed any regression here, so assuming this gets a second stamp and ops are happy with this as well, get in.

Quotes flows

Account address balances and sorting

The right assets are displayed and are properly sorted

  • This diff
image
  • Develop
image

Account selection is happy and the right balances are reflected

  • This diff
image
  • Develop
image

Quotes getting

Not enough asset for gas

  • This diff
image
  • Develop
image

Dangerous amounts

  • This diff
image
  • Develop
image

Safe amounts

  • This diff
image
  • Develop
image

Manual receive address required

  • This diff
image
  • Develop
image

Manual address entered

  • This diff
image
  • Develop
image

Max token swaps

  • This diff
image
  • Develop
image

FOX - ETH (0x)

  • This diff
image image image image
  • Develop
image image

WETH - AVAX (1inch)

  • This diff
image image image image image image
  • Develop
image image image image image

ETH - DOGE (custom receive address)

  • This diff
image image image image image image image
  • Develop
image image image image image image image

0xApotheosis
0xApotheosis approved these changes Jan 19, 2024
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.

Tested against develop and noticed no regressions in quote data.

I did see some differences in the way we are handling error messages (we are surfacing them more), but I believe this is expected.

@0xApotheosis 0xApotheosis merged commit deffcae into develop Jan 19, 2024
4 checks passed
@0xApotheosis 0xApotheosis deleted the tidy-up-slices branch January 19, 2024 03:01
@0xApotheosis 0xApotheosis mentioned this pull request Jan 19, 2024
Merged
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

@gomesalexandre gomesalexandre gomesalexandre approved these changes

Assignees
No one assigned
Labels
high risk
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Merge tradeQuoteSlice and swappersSlice
3 participants
@woodenfurniture @gomesalexandre @0xApotheosis