Skip to content
This repository has been archived by the owner on Feb 2, 2024. It is now read-only.

Partial fills table surplus #440

Merged
merged 6 commits into from
Apr 11, 2023

Conversation

alfetopito
Copy link
Collaborator

@alfetopito alfetopito commented Apr 10, 2023

Summary

Adding surplus to trades fills table

image

To Test

  1. Open a partial fill order with fills
  • Surplus amount and percentage should be displayed

Notes:

  • The surplus displayed is relative to the amounts being bough/sold in the tx
  • The details page shows the average surplus for the whole order

Check this order for example: 0x62baf4be8adec4766d26a2169999cc170c3ead90ae11a28d658e6d75edc05b185b0abe214ab7875562adee331deff0fe1912fe42644d2bb7

image
image

@vercel
Copy link

vercel bot commented Apr 10, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
explorer-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback

📖 Storybook ↗︎

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

Looks good, but it was hard to test because the buy sell amounts are not correct, so hard to check the actual surplus %

Also, wouldn't make more sense the order of the columns: Sell amount, Buy Amount, surplus?

image

Probably not in the scope of this PR, but just mentioning. Should we order surplus info in the header of the fills? (we have it in the overview, but i think is nice to add there too)

Lastly, and also not from this PR (sorry for this), but now that i re-read how we show the amounts for partials, I would think is better to omit some info (it feels a bit hard to read)

image

@alfetopito
Copy link
Collaborator Author

Looks good, but it was hard to test because the buy sell amounts are not correct, so hard to check the actual surplus %

What do you mean the amounts are not correct? The order?

Also, wouldn't make more sense the order of the columns: Sell amount, Buy Amount, surplus?

I agree, just didn't do that yet because I wanted to do it in a different PR

@alfetopito alfetopito requested a review from a team April 11, 2023 09:47
@elena-zh
Copy link

Hey @alfetopito , great!
I have caught the app crash when I try to hover a mouse on a surplus column on a TX details page/User details page
app crash

@elena-zh
Copy link

Also in the current PR, there is an empty space in the table.
image
(not relevant in the #412)

@alfetopito
Copy link
Collaborator Author

alfetopito commented Apr 11, 2023

Also in the current PR, there is an empty space in the table. image (not relevant in the #412)

Fixed the crash issue.
Will address this issue in a new PR
Actually, I had removed a column by mistake, fixed!

Still, will addressAnxo's comments in another PR

@elena-zh
Copy link

Hey @alfetopito , one more issue that would be great to fix in your upcoming PR: surplus icon is shifted in a mobile view now (User details/Transaction details page)
image

And there is no such icon on the Fills tab. Should we add it (or remove from User details/Tx details tables)?
image

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

Approving given the fact that the rest issues will be addressed in a separate PR.

@alfetopito alfetopito merged commit 052a822 into partial-fills-table Apr 11, 2023
@alfetopito alfetopito deleted the partial-fills-table_surplus branch April 11, 2023 15:25
alfetopito added a commit that referenced this pull request Apr 13, 2023
* Remove unused css

* add view fills button

* add TableHeading layout

* Update fills component

* Fix stories

* Small fix

* Fix percent formatting (#413)

* Fix error displaying 0% and 100%

* Add testing for formatting function

* Rmove helper function

* Style fixes for  Order fills page (#423)

* Refactoring to make the code unambiguous and reduced lines of code 38->12

* Fixed displaying of order assets info in <FilledProgress/>

* Fixed layout for FillsTable component

1. Added <TokenAmount> component for buy/sell/execution amounts. It helps to display them shorter
2. Fixed displaying of "View batch graph" button, it was too big for mobile view
3. Fixed global layout (in MainWrapper) to fix page with in mobile view. Warning! This change might brake smth in other pages

* Fixed filled progress width for DetailsTable in mobile view

* Execution price calculation for Fills table

* Added invert button for mobile layout

* isPriceInverted

* Execution time for trades table (#425)

* Execution time for trades table

* TradesTimestamps type

* Fix type

* Fix type

* Cache for fetchTradesTimestamps()

* Fixed blinking of fills table

isFirstLoading - seems to be excessive, because we should display loader only once at start and areTokensLoaded is enough

* Fixed lint errors and remove node version restriction

* Rollback node restriction

* Changed isLoading flag meaning in useOrderTrades()

Now it actually means isFirstLoading

* Fix lint errors (#426)

* Partial fills table/#437 fix fill or kill txhash (#438)

* Removed old placeholder

* Fixed txhash for fill or kill orders

* Do not mutate original obj

* Do not show fills tab for fill or kill orders (#439)

* Do not show fills tab for fill or kill orders

* Hide fills button

* Do not show fills tab for orders with single fills

* Do not show fills tab if there are no trades

* Moved fills button display logic to parent component

* Show tx hash for partial fills with single fill

* Updated label to be explicit about partial fills

* Fixed storybook

* Consistently using the same rule for displaying the fills tab, button and tx hash row:

When there are 2 trades or more, show fills tab and button, and hide tx hash row

* Do not show fills tab if there are no trades 🤦

* Partial fills table surplus (#440)

* Added new component SurplusComponent

* Added calculation for trade surplus getTradeSurplus

* Added surplus to fills table

* Refactored OrderSurplusDisplay to use SurplusComponent

* Fixed issue with surplus tooltip

* Removed buy amount value by mistake 😅

* Partial fills table adjust columns (#442)

* Inverted sell amount and buy amount positions on fills table

* Removed batch viewer link column from fills tab

* Moved surplus column to after buy amount on fills tab

* Added total surplus to fills table

* Removed `of <total amount>` from filled progress

* Partial fills table styling (#443)

* partial fills table styling

* partial fills table styling

* conditional lineBreak for filled amount

* Table header arrow wrap

* fix arrow green

* add surplus green icon to fills

* remove bottom border

* remove bottom border

* fix top padding sticky cards

* revert button wrapping

---------

Co-authored-by: fairlight <31534717+fairlighteth@users.noreply.github.com>

---------

Co-authored-by: Mati Dastugue <mdastugu@amazon.com>
Co-authored-by: Agustín Longoni <agustin.longoni@altoros.com>
Co-authored-by: Alexandr Kazachenko <shoom3301@gmail.com>
Co-authored-by: Alfetopito <alfetopito@users.noreply.github.com>
Co-authored-by: fairlight <31534717+fairlighteth@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants