Skip to content
This repository has been archived by the owner on Apr 4, 2022. It is now read-only.

Adding signature pending orderStatus #826

Merged
merged 7 commits into from
Nov 15, 2021
Merged

Conversation

henrypalacios
Copy link
Contributor

@henrypalacios henrypalacios commented Nov 9, 2021

Summary

Closes #628

  • Adding signing in order status
    Selection_351
  • Maintain consistency with status 'Cancelled' instead of 'Canceled' from cowswap
    Selection_352

To test

Signing status.

View the OrderStatus signing label

@github-actions
Copy link

github-actions bot commented Nov 9, 2021

@elena-zh
Copy link

elena-zh commented Nov 9, 2021

Hey @henrypalacios , just have tried to test your PR.
I was not able to get 'Signing' (or so?) status in the explorer for an order.
I connected the app with Gnosis Safe, then signed an order: the order was always in 'Open' status when it was in 'Signing' phase
open

@elena-zh
Copy link

elena-zh commented Nov 9, 2021

Hey @henrypalacios , could you please take a look into #612 task in this PR?
It is also related to sync statuses case.

Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Looking and working great, some minor comments regarding the code

src/api/operator/types.ts Outdated Show resolved Hide resolved
src/components/orders/StatusLabel/index.tsx Outdated Show resolved Hide resolved
src/utils/operator.ts Outdated Show resolved Hide resolved
@henrypalacios
Copy link
Contributor Author

@elena-zh from #612 I have only been able to change 'Canceled' to 'Cancelled' to maintain consistency with cowswap as you suggest, I have not found a way to be able to synchronize the cancelling status.

As @alfetopito mentioned we don't get this kind of status from the back-end, it occurs to me to show it in cancelling until we don't have a confirmation of mining on the blockchain.

But in any case, we will handle this in another issue.

@elena-zh
Copy link

HI @henrypalacios , thank you for taking a look into the #612 issue and your explanation.
I have verified the recent changes: now I'm able to see an order in 'Signature pending' status.
However, could we make statuses the same in CowSwap and in the Explorer: use 'Signing' instead of 'Signature pending'?
signing
signing1

It will make the status shorter and it will be better displayed in a UI (in 1 line in the Orders list and in a mobile versions with small screen width)

Also, we update statuses each 30 sec, and it might happen that the order is already filled, but it is still in 'Signature pending' status in Explorer.
filled
Maybe we could decrease this interval?
@alfetopito , WDYT?

@henrypalacios henrypalacios force-pushed the 628-status-for-pre-sign branch 2 times, most recently from c486035 to 5fbbc55 Compare November 10, 2021 13:59
@henrypalacios
Copy link
Contributor Author

@elena-zh I agree with you, I have changed it to signing.

In your experience in Mainnet, what do you think would be the most convenient refresh time?,
I have tried to lower it to 25sec.

But maybe @alfetopito can suggest something.

@elena-zh
Copy link

In your experience in Mainnet, what do you think would be the most convenient refresh time?

Can't guess... That is why I addresses this question to @alfetopito

The only thing I can add that we can place orders in XDAI besides Mainnet. Transactions run quickly there.

@elena-zh
Copy link

@henrypalacios , 'signing' status looks much better!
Just while we are waiting for Leandro's answer, I'd like to mention a new nitpick: shimming effect for the 'signing' status is displayed only for the 'key' icon, not for the whole status.
Could we show shimming effect to the whole status?
image

@henrypalacios
Copy link
Contributor Author

@elena-zh Done!
signing_status

@alfetopito
Copy link
Contributor

I don't think it's a big deal to wait a little longer to see the status changing, 30s is fine.

Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

2 minor comments

src/apps/explorer/const.ts Outdated Show resolved Hide resolved
src/utils/operator.ts Outdated Show resolved Hide resolved
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.

Great!

Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Approved!

Only one minor change below :)

src/api/operator/types.ts Outdated Show resolved Hide resolved
@alfetopito alfetopito merged commit b03729c into develop Nov 15, 2021
@alfetopito alfetopito deleted the 628-status-for-pre-sign branch November 15, 2021 18:24
@henrypalacios henrypalacios self-assigned this Nov 15, 2021
@anxolin
Copy link
Contributor

anxolin commented Nov 16, 2021

Nice!! I would close version with this and whatever is ready 👌

This was referenced Nov 16, 2021
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.

[presign] Explorer: Status for pre-signing
4 participants