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

Fixed net profit panel, sync updateAuctionsPrices calls #566

Merged
merged 4 commits into from
Dec 12, 2022

Conversation

aomafarag
Copy link
Contributor

Closes #564.

switching.mp4

Checklist:

  • issue number linked above after pound (#)
    • replace "Closes " with "Contributes to" or other if this PR does not close the issue
  • issue checkboxes are all addressed
  • manually checked my feature / not applicable
  • wrote tests / not applicable
  • attached screenshots / not applicable

@aomafarag
Copy link
Contributor Author

Since it could be tricky to directly test the fix with simulated or live auctions, I adjusted the default story of CollateralAuctionSwapTransaction to have two callees: one producing positive and the other negative net profit (see screencast).

Copy link
Contributor

@valiafetisov valiafetisov left a comment

Choose a reason for hiding this comment

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

This doesn't solve actual problem. Actual problem is: at first auction is fetched and added to the store, then another enrichment is applied that is not initially awaited. Instead, we should await all enrichments before the auction is added to the store

The proposal is to remove changes introduced here (or just open a new pr) and instead solve actual problem

@aomafarag
Copy link
Contributor Author

This doesn't solve actual problem. Actual problem is: at first auction is fetched and added to the store, then another enrichment is applied that is not initially awaited. Instead, we should await all enrichments before the auction is added to the store

The proposal is to remove changes introduced here (or just open a new pr) and instead solve actual problem

I guess this comment is meant for another PR? Here, we’re merely passing the selected auction’s net profit to the profit check panel.

@valiafetisov
Copy link
Contributor

valiafetisov commented Dec 8, 2022

No, I'm talking about the root cause of the problem and speculating on the solution (which might not be correct). The actual problem is not "profit check panel", but this state of the auction:

Screenshot 2022-12-08 at 18 00 47

Which should not exists in the first place. If automatic router is enabled, the UI should always:

  1. Request auction with automatic router
  2. Wait till every single market is enriched
  3. Update store

But what is correctly happening

  1. Frontend requests auction
  2. The auction is fetched with one market being undefined
  3. Later the auction is updated/object is modified with latest values

Above was a speculation on why it is happening: auto-router is not awaited

Comment on lines -70 to -71
:gross-profit="auctionTransaction.transactionGrossProfit"
:net-profit="auctionTransaction.transactionNetProfit"
Copy link
Contributor Author

@aomafarag aomafarag Dec 8, 2022

Choose a reason for hiding this comment

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

I still think the changes are relevant. Here, we are passing the gross and net profit from the auctionTransaction directly, which are based on suggestedMarketId alone, not the currently selected marketId. So, the problem will happen even for collaterals that don't support auto router.

Copy link
Contributor

@LukSteib LukSteib left a comment

Choose a reason for hiding this comment

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

Created a testing scenario via USDC-A auction simulation (see recording below). As required the net profit panel now is inline with the callee selection.

https://www.loom.com/share/a237a83a904e49c2bd414913de81847a

Comment on lines +238 to +241
updateAuctionsPricesIntervalId = setInterval(
async () => await dispatch('updateAuctionsPrices'),
TIMER_INTERVAL
);
Copy link
Contributor

Choose a reason for hiding this comment

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

btw, @KirillDogadin-std, this change doesn't make any difference 🙂

Suggested change
updateAuctionsPricesIntervalId = setInterval(
async () => await dispatch('updateAuctionsPrices'),
TIMER_INTERVAL
);
updateAuctionsPricesIntervalId = setInterval(() => dispatch('updateAuctionsPrices'), TIMER_INTERVAL);
  1. setInterval doesn't expect any promises and doesn't do anything different if received a promise
  2. () => Promise would already return the promise, making it async () => await Promise not change what is returned or how it's awaited

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, buuuuut i have an everlasting problem with forgetting to await sometimes. Aaand making the async syntax propagated to the top helps a bit because it explicitly screams of promises.

@valiafetisov valiafetisov merged commit c82676e into main Dec 12, 2022
@valiafetisov valiafetisov deleted the sync-net-profit branch December 12, 2022 08:45
@valiafetisov valiafetisov changed the title Sync net profit Fixed net profit panel, sync updateAuctionsPrices calls Dec 12, 2022
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.

Properly update net profit indicator panel
4 participants