1334/only update connected wallet #1684

alfetopito · 2021-10-21T22:56:53Z

Summary

Waterfalls into #1639

Do not update orders that don't belong to the connected wallet

To Test

Place an order and open the link to the Explorer
Switch to a different account
Wait until the order is matched

You should NOT get a notification about the order being filled
The filled order should not be visible in the activity history

Repeat steps after cancelling and when an order is about to expire

There should be no notification for orders not belonging to the connected account

github-actions · 2021-10-21T23:03:45Z

🔭 GP Swap: Gnosis Protocol v2 Swap UI

MareenG · 2021-10-22T11:32:15Z

I encountered some issues while testing.
First I tried with rinkeby and always got the following error message:

No matter which token pair or account I am using.
Its the same for Xdai

MareenG · 2021-10-22T11:37:31Z

On mainnet it works however there is another issue
For some trades the explorer website didn't update when using the link in the pop up modal. It was stuck in the loading process.
When using the link from recent activities after the order was filled it worked also refreshing the site after a while worked

MareenG · 2021-10-22T11:48:02Z

Otherwise it works as expected :)

elena-zh · 2021-10-22T13:56:21Z

Hey @alfetopito , great!
I have run several tests in order to check whether #1159 case was accidentally fixed with your changes. Unfortunately, not.
But I think it would be logical to fix it as well due to order confirmation modal remains to be displayed when:

change an account
change a network

Besides, it would be nice to display an order status (expired/filled) when we get back to an account. WDYT?

alfetopito · 2021-10-22T17:28:07Z

@MareenG

I encountered some issues while testing. First I tried with rinkeby and always got the following error message: No matter which token pair or account I am using. Its the same for Xdai

That's super interesting, will investigate.

For some trades the explorer website didn't update when using the link in the pop up modal. It was stuck in the loading process.
When using the link from recent activities after the order was filled it worked also refreshing the site after a while worked

Since the orderId is the same on both the behaviour seems correct from CowSwap side.
Seems like a loading problem on Explorer side.
Haven't bothered that much since a refresh fixes it, but we for sure could improve that.
Would you mind creating an issue on Explorer issue?

@elena-zh

I have run several tests in order to check whether #1159 case was accidentally fixed with your changes. Unfortunately, not.
But I think it would be logical to fix it as well due to order confirmation modal remains to be displayed when:

I don't fully get what you mean.

Are you suggesting to - when account/network changes - remove all currently displayed toast notifications?

Besides, it would be nice to display an order status (expired/filled) when we get back to an account. WDYT?

I might be misremembering, but I think we had that at first but removed because the user might connect to an account at an undefined amount of time later and get the notifications which might be out of context/no longer relevant.

alfetopito · 2021-10-22T20:36:04Z

@MareenG I couldn't reproduce but I've created this PR in an attempt to fix your issue #1686

Let me know whether it works for you there

elena-zh · 2021-10-25T08:52:15Z

Are you suggesting to - when account/network changes - remove all currently displayed toast notifications?

@alfetopito , exactly

I might be misremembering, but I think we had that at first but removed because the user might connect to an account at an undefined amount of time later and get the notifications which might be out of context/no longer relevant.

Hm. I still face them in a mobile view and have been thinking that it is OK. That is why I asked about it.

Co-authored-by: Leandro Boscariol <[email protected]>

anxolin

Nice! looking fw to cherry grab this one!

anxolin · 2021-10-27T17:40:36Z

src/custom/state/orders/reducer.ts

+  status: OrderTypeKeys,
+  order: SerializedOrder
+): void {
+  // Attempt to fix `TypeError: Cannot add property <x>, object is not extensible`


I see you got the idea from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Cant_define_property_object_not_extensible

..but, I don't really get it, one reason we use reduxjs/toolkit it's because we can mutate fearlessly the state. Who do you suggest is using preventExtensions?

We have similar logics in all other reducers. Should we be consider with the others? why this is happening now?

I'm not completely sure what was causing it, but this seemed to be the place where issues Mareen was facing came from.
For more context check the PR where this was added #1686
I was not able to reproduce it, but as you can see from the from PR, it seemed that the problem was trying to edit a readonly object.

src/custom/state/orders/updaters/PendingOrdersUpdater.ts

anxolin

Approving, although the REDUX fix bugs me a bit. It makes me a bit uncofortable to change code without understanding what is happening.

Ideally we would reproduce the case and understand it better, cause maybe this is covering something else 🤷

* Added /account/<address>/orders api endpoint * Added helper function to get explorer address link * Requiring needed properties instead of concrete types on utils fns * Added addOrUpdateOrdersBatch to orders/actions * Added useAddOrUpdateOrdersBatch to orders/hooks * Added addOrUpdateOrdersBatch to orders/reducer * Added APIOrdersUpdater that checks for orders from API on connection * Adjusted displayed orders: - At most 10 regular orders and as much pending as there are - Show only connected account orders - Sort orders by creation date descending * Show only txs for connected account * Refactoring * Don't actually need a helper fn... * Added comment to make sort more explicit * Using 'Link to Explorer' instead of 'Clear activities' * Refactoring * Refactor: cache tokens loaded const * Refactor: removed duplicated css property * Refactor: removed duplicated CSS property * Refactor: extracted function transformApiOrderToStoreOrder * Refactor: renamed _orders to apiOrders * Refactor: removed duplicated CSS property * Adjusting `View all orders` link size * Added `View all orders` to the bottom of account activity * Refactored computeOrderSummary to deal with pending orders * Computing order summary for orders loaded from API * Setting isCancelling state if order is pending and cancelled on API * Actually syncing `isCancelling` flag for existing orders * Refactor: created const for magic number MAXIMUM_ORDERS_TO_DISPLAY * Refactor: simplified addOrUpdateOrdersBatch reducer * Refactor: renamed API -> Api on file name * Refactor: replaced magic number with AMOUNT_OF_ORDERS_TO_FETCH * Refactor: converted classifyLocalStatus fn to a map * Refactor: removed `Batch` from addOrUpdateOrders * Refactor: moved error exit condition up * 1334/load new tokens not yet loaded (#1504) * Added non-hook methods for dealing with contracts * New hook useTokensLazy * Exporting stuff from original codebase * Added getMultipleCallsResults under custom/state/multicall/utils Non-hook way of fetching multiple onchain queries through multicall * Refactor ApiOrdersUpdater to load tokens not yet in the UI * Typo fix * 1334/do not refetch when token added (#1536) * Removing annoying log * No longer re-fetching orders when a token is added to the list Co-authored-by: Leandro Boscariol <[email protected]> * Refactored useTokensLazy: - Added contracts cache - Improved comments * Improved doc string on multicall utils Co-authored-by: Leandro Boscariol <[email protected]> * Fix new Erc20 import path * Adjust to new multicall interface and fetchChunk changes * Adapt to renamed of ApiOrderStatus to OrderTransitionStatus * Adjusting (again) styles for `view all activity` * Handling presignaturePending state from backend * The order is concrete at this point, no need for `?.` * 1334/only update connected wallet (#1684) * Only update cancelled orders for connected wallet * Only check for unfillable orders for connected wallet * Only check pending orders for connected wallet * Always create a new state object when adding an order (#1686) Co-authored-by: Leandro Boscariol <[email protected]> * Refactor: comparing account with order.owner lowercased for safety Co-authored-by: Leandro Boscariol <[email protected]> * Added new hook useTokenLazyNoMulticall to fetch token data one by one * Using new hook to fetch tokens instead of multicall * Removed multicall/utils * Removed useTokensLazy hook * Renamed file useTokensLazy to useTokenLazy * Renamed useTokenLazyNoMulticall to useTokenLazy * Removed unused import * Updated debug log messages * Small refactor in #1639 (#1857) * Only init bytes32Contract if we really need to * Fixing build error: only proceed if library is set Co-authored-by: Leandro Boscariol <[email protected]> * 1334/with swr (#1906) * 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]> Co-authored-by: Leandro Boscariol <[email protected]> Co-authored-by: Anxo Rodriguez <[email protected]>

* Only update cancelled orders for connected wallet * Only check for unfillable orders for connected wallet * Only check pending orders for connected wallet * Always create a new state object when adding an order (gnosis/cowswap#1686) Co-authored-by: Leandro Boscariol <[email protected]> * Refactor: comparing account with order.owner lowercased for safety Co-authored-by: Leandro Boscariol <[email protected]>

Leandro Boscariol added 3 commits October 21, 2021 15:51

Only update cancelled orders for connected wallet

1d2fa1a

Only check for unfillable orders for connected wallet

cae8cd0

Only check pending orders for connected wallet

c6cc3eb

alfetopito self-assigned this Oct 21, 2021

alfetopito requested review from a team October 21, 2021 22:57

alfetopito mentioned this pull request Oct 21, 2021

1334/activity orders from api rebased #1639

Merged

4 tasks

This was referenced Oct 25, 2021

[Pre-sign] UI issues with network indicator #1687

Closed

[1.5.0] Firefox: activity modal has a square right corners #1688

Closed

MareenG mentioned this pull request Oct 25, 2021

Order doesn't load gnosis/gp-ui#792

Closed

Always create a new state object when adding an order (#1686)

bf70822

Co-authored-by: Leandro Boscariol <[email protected]>

anxolin reviewed Oct 27, 2021

View reviewed changes

Refactor: comparing account with order.owner lowercased for safety

68b0b2e

alfetopito requested a review from anxolin October 27, 2021 22:05

anxolin mentioned this pull request Nov 4, 2021

Filter by connected user #1783

Merged

anxolin approved these changes Nov 4, 2021

View reviewed changes

anxolin mentioned this pull request Nov 5, 2021

Do not update orders other accounts #1791

Merged

alfetopito merged commit 2aa2163 into 1334/activity-orders-from-api-rebased Nov 8, 2021

alfetopito deleted the 1334/only-update-connected-wallet branch November 8, 2021 22:49

MareenG mentioned this pull request Nov 16, 2021

[1.5.0] Can't place an order due to 'can't define property' error #1844

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1334/only update connected wallet #1684

1334/only update connected wallet #1684

alfetopito commented Oct 21, 2021

github-actions bot commented Oct 21, 2021

MareenG commented Oct 22, 2021

MareenG commented Oct 22, 2021

MareenG commented Oct 22, 2021

elena-zh commented Oct 22, 2021

alfetopito commented Oct 22, 2021

alfetopito commented Oct 22, 2021

elena-zh commented Oct 25, 2021

anxolin left a comment

anxolin Oct 27, 2021

alfetopito Oct 27, 2021

anxolin Nov 4, 2021

anxolin left a comment

1334/only update connected wallet #1684

1334/only update connected wallet #1684

Conversation

alfetopito commented Oct 21, 2021

Summary

To Test

github-actions bot commented Oct 21, 2021

MareenG commented Oct 22, 2021

MareenG commented Oct 22, 2021

MareenG commented Oct 22, 2021

elena-zh commented Oct 22, 2021

alfetopito commented Oct 22, 2021

alfetopito commented Oct 22, 2021

elena-zh commented Oct 25, 2021

anxolin left a comment

Choose a reason for hiding this comment

anxolin Oct 27, 2021

Choose a reason for hiding this comment

alfetopito Oct 27, 2021

Choose a reason for hiding this comment

anxolin Nov 4, 2021

Choose a reason for hiding this comment

anxolin left a comment

Choose a reason for hiding this comment