Skip to content
This repository was archived by the owner on Jun 24, 2022. It is now read-only.

1334/activity orders from api rebased #1639

Merged
merged 55 commits into from
Nov 29, 2021

Conversation

alfetopito
Copy link
Contributor

@alfetopito alfetopito commented Oct 15, 2021

Summary

Closes #1334

Follows up/supersedes #1395

Same content as that, but rebased against develop after the presign merge.

Lot's of changes, some things might not work
Activity orders from API: "Your orders, wherever you are"

Main changes:

  • At start/network/account change, fetches orders from API
  • Shows only orders for connected account/network
  • Shows only txs for connected account/network
  • Displays at most 10 regulars orders, and as many pending as there are
  • Shows orders sorted by creation date, descending
  • Replaced Clear activity with View all orders
  • Added a View all orders at the bottom of activity list
  • Show orders that have Tokens not yet loaded in the current UI

To Test

  1. Connect a wallet with existing orders
  2. Open Account Activities sidebar
  • There should be 10 orders or less, doesn't matter how old they are (There might be more if you have more than 10 pending orders)
  • There should be a link on top right that takes you to the Explorer to see all of your orders
  1. Change account AND/OR network
  • Orders should be updated to match the selected account/network
  1. Place an order
  2. While it's pending, open the same page in another browser / device (:warning: important to test in a different browser otherwise the same local storage will be used)
  • The pending order should be visible as pending in the new browser / device

NOT part of this PR

  • Load txs the same way as orders
  • Refresh pending orders in the background - right now they are updated only on account/network change

Not yet addressed

  • presignature pending state on order insertion before calling reducer
  • background update of orders for not the current connected wallet (1334/only update connected wallet #1684)
  • issues with safe not reflecting pre-signing
  • random safe hard errors

@alfetopito
Copy link
Contributor Author

Notes for @gnosis/gp-frontend & @gnosis/gp-qa

Code is mostly the same as #1395

To avoid a huge merge issue again and to make it easier for review, I'll address pending issues as separated PRs
The behaviour is good enough for EOAs, and still bad for SC/Safe wallets

I would prefer to merge this sooner if possible

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

@alfetopito alfetopito self-assigned this Oct 18, 2021
Leandro Boscariol added 26 commits October 21, 2021 14:59
- At most 10 regular orders and as much pending as there are
- Show only connected account orders
- Sort orders by creation date descending
@alfetopito
Copy link
Contributor Author

Read for re-review @gnosis/gp-qa

I've updated how the tokens are loaded, merged and resolved conflicts with latest develop.

To test, on mainnet place an order with this token 0x89d24a6b4ccb1b6faa2625fe562bdd9a23260359 (SAI) or MKR.
You don't need to buy it, actually it's better if you cancel the trade because it's a "dead" token.
Example order with it https://protocol-explorer.dev.gnosisdev.com/orders/0x8524db28d6683e6c2996e158115a37e38e6d486a790f9ddebadbd8da114db9a7dd9eb88c5c6d2a85a08a96c7f0cccce27cb843cb6192e772

The special thing about SAI and MKR tokens is that their name/symbol are encoded as bytes32 which is different that 99.9% of the tokens.

Once you place the order, clear the token from the user added tokens and refresh the page and/or open in another browser.

Of course, testing with other regular tokens is important, not just the outliers.
The token should be loaded, added to user tokens and the order displayed on the activities list.

@anxolin
Copy link
Contributor

anxolin commented Nov 16, 2021

@alfetopito should we use a new branch "orders-api" as the base, so we can safely merge this even if its not complete?

Maybe this would make easier to do small PR iterations after this one and we don't need to keep it open?

@anxolin anxolin mentioned this pull request Nov 16, 2021
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.

Approve with comments. I think we can start consolidating, just to be able to reiterate on the small things in new PRs (this one is already biggg)

@elena-zh
Copy link

Hey @alfetopito , SAI token was not added to the custom token list after its removal from there
https://watch.screencastify.com/v/efcFy6o6hcnlF5nWlkXo

However, orders history has been loaded

@elena-zh
Copy link

elena-zh commented Nov 17, 2021

During the day I was able to get 'Fee is insufficient' warning several times when I was testing #1861 PR. I thought that the issue is related to small trading amounts.
But the same issue has appeared when I started testing the current PR, and trading amounts were high. I reported it here #1868 (comment)
image

@alfetopito , could you please take a look at it?

@elena-zh
Copy link

Also, I see 'token not found' message in the console when I place an order with a custom token (ZRX) in Rinkeby.
And it does not appear in the Custom token's list when I delete them, and then reload the page
image
image

* Only init bytes32Contract if we really need to

* Fixing build error: only proceed if library is set

Co-authored-by: Leandro Boscariol <[email protected]>
@alfetopito alfetopito mentioned this pull request Nov 24, 2021
@alfetopito
Copy link
Contributor Author

@elena-zh could you try again again on #1906 ?

Just keep in mind it has the latest from develop but doesn't yet have all the fixes included on release/1.5 branch, so some regressions are expected.

Also, I've tried on different browsers and several accounts/wallets and could not reproduce your issue.
Every time the tokens loaded fine.

🤞 hopping this time it's fixed for you as well

@elena-zh
Copy link

Hi @alfetopito , changes LGTM in #1906
Besides, I reported a separate issue #1919 for displaying 'Filled on' on loaded activity list for 'Filled' orders, as API request might be changed and will have this info in the near future.

The most interesting thing (that is also a 'won't fix') is that activity in https://cowswap.dev.gnosisdev.com/#/swap shows a pending/cancelling/etc. order from https://barn.cowswap.exchange/#/swap as they both related to Dev server.
The same will be for Prod and Stage environments.

alfetopito and others added 3 commits November 25, 2021 08:17
* Refactor: removed unnecessary variable

* Added SWR as dev dependency

* Added hook useApiOrders using useSWR hook

* Using new hook for fetching api orders

* Removed unused imports...

Co-authored-by: Leandro Boscariol <[email protected]>
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 great!

nit: maybe we want to consider to change the message to "No activity yet". Now we are sure there's no activity for that account

image

Why is this PR adding presignaturePending all over? isn't this from presign PRs already?

@alfetopito
Copy link
Contributor Author

@anxolin I'll merge as is and address your comments in follow up PRs

nit: maybe we want to consider to change the message to "No activity yet". Now we are sure there's no activity for that account

Well there might be approvals/wraps. We are not syncing them yet

Why is this PR adding presignaturePending all over? isn't this from presign PRs already?

This type was not exposed where I needed it

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.

Load orders from API in the Recent Orders
4 participants