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

Minor surplus fixes #468

Merged
merged 7 commits into from
Sep 22, 2022
Merged

Minor surplus fixes #468

merged 7 commits into from
Sep 22, 2022

Conversation

aomafarag
Copy link
Contributor

@aomafarag aomafarag commented Sep 21, 2022

Closes #450.

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

Issues to be fixed:

  • Fix uniswap price fetching when there is no bids
  • Set nextMinimumBid to make sense (eg a uniswap price * gap to reach 1k net profit)
  • SurplusAuctionBidTransactionTable "state" should display state, not just date
  • SurplusAuctionBidTransactionTable end state should count down, but count up (ie Ended 2s ago, 3s ago, etc. First guess: can be caused by invalid determination of the earliestEndDate)

@aomafarag
Copy link
Contributor Author

I discovered that SurplusAuction was used as the prop instead of SurplusAuctionTransaction. Changing it fixed the Price on UniSwap not fetching issue, and the nextMinimumBid being initially 0 one.

@valiafetisov
Copy link
Contributor

Set nextMinimumBid to make sense (eg a uniswap price * gap to reach 1k net profit)

Why was it not addressed in this PR? I understand that it's not the UI problem, but you basically need to fix nextMinimumBid in the core to set a valid default instead of 0

@aomafarag
Copy link
Contributor Author

Set nextMinimumBid to make sense (eg a uniswap price * gap to reach 1k net profit)

Why was it not addressed in this PR? I understand that it's not the UI problem, but you basically need to fix nextMinimumBid in the core to set a valid default instead of 0

In the core it's set as bidAmountMKR * increaseCoefficient. Could the increaseCoefficient be initially 0?

@valiafetisov
Copy link
Contributor

Initially, there is no bidAmountMKR as there is no bids yet. But you only need to check if nextMinimumBid is zero to set it to another default, since 0 is not a valid bid amount

@aomafarag
Copy link
Contributor Author

... (eg a uniswap price * gap to reach 1k net profit)

I adjusted the code with this suggestion

valiafetisov
valiafetisov previously approved these changes Sep 21, 2022
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.

  • For me uniswap price still not fetched properly (see 1 below)

1
image

@aomafarag
Copy link
Contributor Author

OK, so after hours of digging, here's where I think the problem is: the simulation is not enriching newly created surplus auctions. The thing is, the Price on Uniswap is not fetched separately from the auction, but by the marketUnitPrice property of the auction, which is initialized when the auction is enriched. So, the fact that a forked block of auctions displays the marketUnitPrice correctly and the newly created auction doesn't would be due to the newly created auction not being enriched properly.

@valiafetisov
Copy link
Contributor

Not sure I fully understand without a link to the source code where the problem is. What is the proposed solution?

@aomafarag
Copy link
Contributor Author

There are two scenarios of the simulation:

  1. Create a new surplus auction
  2. Fork a block of surplus auctions

Price on Uniswap is not fetched only in the first case (the issue Lukas refers to). This is because the price is fetched with the marketUnitPrice property of the SurplusAuctionEnriched interface. So, the reason it doesn't show in the first simulation case would be that the auctions are not enriched in that first case, not due to the frontend not fetching it properly.

@valiafetisov
Copy link
Contributor

the auctions are not enriched in that first case

Ok, the problem is reproduced. Do you want to investigate why is it not enriched in this case and propose a solution?

@aomafarag
Copy link
Contributor Author

I checked the two simulation cases for the debt auctions, and they work fine. The difference to the surplus cases seems to be that debt auctions have a fixed bid. Surplus auctions, however, start with no bids, and their calculation of marketUnitPrice relies on bidAmountMKR. Since there are no bids, that calculation gives off NaN.

I guess such a behavior is acceptable, since we can't calculate a unit price if we have no units. @valiafetisov, what do you think?

@valiafetisov
Copy link
Contributor

Since we want to show "unit price" and not the total price for the whole collateral, we can (only in case when there is no bids yet) compute marketUnitPrice using arbitrary number, eg 1. The price will be different in case the bid amount will be significantly bigger, but I think it's better to show at least some kind of unit price as a reference than no unit price at all. Since then the user will still need to go to external platform to get it. Do you agree, @LukSteib?

@aomafarag
Copy link
Contributor Author

... we can (only in case when there is no bids yet) compute marketUnitPrice using arbitrary number, eg 1

That's what I went for in the latest commit

@LukSteib
Copy link
Contributor

Do you agree, @LukSteib

Agree to the propsal by @valiafetisov. Better to use arbitrary number (in this case 1) than none.

@aomafarag aomafarag self-assigned this Sep 22, 2022
@valiafetisov valiafetisov merged commit bfaee17 into main Sep 22, 2022
@valiafetisov valiafetisov deleted the fix-minor-surplus-issues branch September 22, 2022 18:32
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.

Fix surplus-related minor issues
3 participants