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

Integrate Order progress bar #454

Merged
merged 31 commits into from
Jun 14, 2022

Conversation

ramirotw
Copy link
Contributor

Summary

Close #392 and #347

This integrates @alongoni's progress bar UI and @anxolin's progress bar function into the Activity details modal.
Requirements doc: https://docs.google.com/document/d/1rJPCmztsbpIi-jklCFAQIrrPusIBYmrJvswB26I8AWA/edit#heading=h.gjdgxs

image

alongoni and others added 8 commits April 8, 2022 18:06
* add progressbar component

* add warning

* add alert icon

* progressbar position

* add CheckIcon

* add pending status and styles

* add AMMs component

* fix duplicate import

* add progressbar variant and enhancements

* add message container and improve mobile view

* change amms animation time

* fix delay animtation

* improvements, color, mobile

* change clock colors
@github-actions
Copy link
Contributor

CLA Assistant Lite All Contributors have signed the CLA.

@ramirotw ramirotw requested a review from anxolin April 26, 2022 11:11
@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

@ramirotw
Copy link
Contributor Author

@elena-zh just pushed a small fix to hide the progress bar when the user cancels the order

@elena-zh
Copy link
Contributor

@ramirotw , thanks! I was going to leave this feedback to you right now.
Could you please also hide the progress bar for an expired order?
expired and cancelled

@ramirotw
Copy link
Contributor Author

Could you please also hide the progress bar for an expired order?

done!

@elena-zh
Copy link
Contributor

elena-zh commented Apr 26, 2022

Hey @ramirotw , I noticed, that the progress bar is moving too fast, and in few seconds (5-9) it starts to show 'too long' message, and it is weird from my perspective.
The issue is relevant for the Rinkeby and GC networks, in the Mainnet it runs slower.
Also, each time when I open the activity modal, I see the progress bar empty, then it is filled in. Can we save it's state somehow OR fill it in in the background in order to prevent this issue?

See the video for both issues: https://watch.screencastify.com/v/dUOSnu76BQmmFsOxYzvv

@elena-zh
Copy link
Contributor

And another case that is not included into this PR: when a user is conencted to a SC wallet (Gnosis safe or so), we should not show the progress bar until the order's status is changed to 'Open'
image
In other words, no need to show this bar when the order is in 'Signing' state

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.

So nice to see a working version.

I think this feature will need to be tested a bit with users, and will likely require some refinement, because still has some weird effects. However, i was expecting this. Now with the demo, we can explorer how to refine it.

I would avoid to merge this to develop, until we are more sure we are ready to release it.

One quick feedback. There's a weird effect when you open the modal, it shows the progress bar in 0% and it progresses super fast to the current %. Can we show the % in the current value from the begining to avoid this effect?

@anxolin
Copy link
Contributor

anxolin commented Apr 26, 2022

Not sure if this was just a small weird case, but I saw this message
image
and then, a few seconds later it just traded. I think @elena-zh detected similar cases before, and we had an issue, but AFAIT it was solved. So it was strange to see this again.

@ramirotw
Copy link
Contributor Author

One quick feedback. There's a weird effect when you open the modal, it shows the progress bar in 0% and it progresses super fast to the current %. Can we show the % in the current value from the begining to avoid this effect?

Fixed by initializing the percentage value with the current calculated one

Not sure if this was just a small weird case, but I saw this message
image
and then, a few seconds later it just traded. I think @elena-zh detected similar cases before, and we had an issue, but AFAIT it was solved. So it was strange to see this again.

Weird, I'm actually using the isUnfillable value which is derived in:

export function isOrderUnfillable(order: Order, price: Required<PriceInformation>): boolean {
// Build price object from stored order
const orderPrice = new Price(
order.inputToken,
order.outputToken,
order.sellAmount.toString(),
order.buyAmount.toString()
)
// Build current price object from quoted price
// Note that depending on the order type, the amount will be used either as nominator or denominator
const currentPrice =
order.kind === OrderKind.SELL
? new Price(order.inputToken, order.outputToken, order.sellAmount.toString(), price.amount as string)
: new Price(order.inputToken, order.outputToken, price.amount as string, order.buyAmount.toString())
// Calculate the percentage of the current price in regards to the order price
const percentageDifference = ONE_HUNDRED_PERCENT.subtract(currentPrice.divide(orderPrice))
console.debug(
`[UnfillableOrdersUpdater::isOrderUnfillable] ${order.kind} [${order.id.slice(0, 8)}]:`,
orderPrice.toSignificant(10),
currentPrice.toSignificant(10),
`${percentageDifference.toFixed(4)}%`,
percentageDifference.greaterThan(OUT_OF_MARKET_PRICE_DELTA_PERCENTAGE)
)
// Example. Consider the pair X-Y:
// Order price of 1X = 20459.60331Y
// Current market price is 1X = 20562.41538Y
// The different between order price and current price is -0.5025%
// That means the market price is better than the order price, thus, NOT unfillable
// Higher prices are worse, thus, the order will be unfillable whenever percentage difference is positive
// Check whether given price difference is > Price delta %, to allow for small market variations
return percentageDifference.greaterThan(OUT_OF_MARKET_PRICE_DELTA_PERCENTAGE)
}

@ramirotw
Copy link
Contributor Author

Also, each time when I open the activity modal, I see the progress bar empty, then it is filled in. Can we save it's state somehow OR fill it in in the background in order to prevent this issue?

This should be fixed @elena-zh

Hey @ramirotw , I noticed, that the progress bar is moving too fast, and in few seconds (5-9) it starts to show 'too long' message, and it is weird from my perspective.
The issue is relevant for the Rinkeby and GC networks, in the Mainnet it runs slower.

This behavior is defined in the requirements doc. The average execution time for Rinkeby and GC is set to 15 seconds, we should probably keep iterating over these numbers so it's aligned with what's actually going on. Right now the Looking for a CoW state for Rinkeby and GC lasts 4.95 seconds (15*0.33), the remaining 10.05 seconds is for Finding best onchain price and then when crossing the 15 seconds it comes the Your order is taking longer than usual state.

@elena-zh
Copy link
Contributor

I think @elena-zh detected similar cases before, and we had an issue, but AFAIT it was solved. So it was strange to see this again.

@anxolin , @ramirotw this might happen when the price comes back, so not too often, but I can face this.
Anyways, I will keep my eye on it, and if it start happening too often, I will create a separate issue for this.

…_updated' into ramirotw/issue-392-integrate-progress-bar
@ramirotw
Copy link
Contributor Author

FYI, I merged #444 into this branch so Elena could test orders from a Gnosis Safe wallet. Will revert the commit later.

@elena-zh
Copy link
Contributor

Hey @ramirotw , changes look much better now!
I have run several tests again, and found new issues:

  1. Cancel order button under the status is not clickable when 'Price out of range' message is displayed
    not working

  2. I have placed 2 orders one by one in Safe, and when my Activity modal was opened, I saw 2 progress bars in it
    2 progress bars
    Then I closed the modal, opened it once again, and the 2nd bar was no longer displayed

  3. When an order goes to the 'Open' status after 'Signing' phase, progress bar appears in the 'longer than usual' state: https://watch.screencastify.com/v/4GmNF6nyOpnoqorB0JfD
    I expect to see it starting with the beginning

  4. Again, when connected to the Safe wallet, and the order goes to the 'prico out of range' phase, the message is replaced by 'Longer than usual'. This replacement is visible when open the activity modal. see the video: https://watch.screencastify.com/v/YZxA3zuHpCSCHZ7sQN4X
    Could you please take a look into these issues?

Thanks!

@anxolin
Copy link
Contributor

anxolin commented Apr 29, 2022

tiny detail. But i think i didn't see Uniswap icon there in the liquidity we aggregate. I would definitely add it, because uni is the biggest DEX so its nice people know. the liquidty from uni is used:
image

@anxolin
Copy link
Contributor

anxolin commented Apr 29, 2022

Someone mentioned that also 0x, matcha are missing. Thankkkks

@ramirotw ramirotw changed the base branch from develop to progress-bar June 14, 2022 10:48
@ramirotw ramirotw merged commit ef11602 into progress-bar Jun 14, 2022
@ramirotw ramirotw deleted the ramirotw/issue-392-integrate-progress-bar branch June 14, 2022 10:49
@github-actions github-actions bot locked and limited conversation to collaborators Jun 14, 2022
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.

Integrate progress math function into progress bar UI
6 participants