[Solvers] Settlements Table #235

matextrem · 2022-10-05T23:28:43Z

Summary

Closes #234

Added Settlements table in Solvers page

To Test

Open the page solvers . You can use the header menu to navigate.
Click on Settlements tab
- You'll see the settlements table.

Notes: For ETH cost, we need some Subgraph PR to be merged first before we can get that info into the table.

coveralls · 2022-10-05T23:32:38Z

Pull Request Test Coverage Report for Build 3201733554

14 of 18 (77.78%) changed or added relevant lines in 11 files are covered.
68 unchanged lines in 5 files lost coverage.
Overall coverage increased (+0.04%) to 43.756%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/api/weth/WethApi.ts	1	2	50.0%
src/state/network/updater.tsx	1	2	50.0%
src/utils/walletconnectOptions.ts	0	1	0.0%
src/utils/wrap.ts	1	2	50.0%

Files with Coverage Reduction	New Missed Lines	%
src/api/wallet/WalletApiMock.ts	2	45.31%
src/api/web3/index.ts	5	72.73%
src/const.ts	15	81.62%
src/state/network/updater.tsx	16	32.05%
src/services/factories/tokenList.ts	30	4.8%

Totals
Change from base Build 3182895464:	0.04%
Covered Lines:	2214
Relevant Lines:	4286

💛 - Coveralls

github-actions · 2022-10-05T23:35:09Z

🔭 Explorer: Explorer test app
🔒 Safe Swap App: Trade test app (add to your safe apps)
📈 Trade: Trade test app
📚 Storybook: Component stories

ramirotw

Looking good.

A few things:

Change the base branch to 221/active-solvers so the commits are cleaned
Lets hide for now the ETH cost column as we don't have that value from the subgraph
The Tx hash link should go to the Transaction details page not Etherscan

elena-zh · 2022-10-06T11:27:42Z

Hey @matextrem , great job!

Some my issues:

The table shows only 100 settlements in each network. Is this OK?
It would be great not to navigate users to the Home page when switch networks (added this case to Do not navigate to Home page when switch network from Solvers page #230 )
Values in columns are not aligned on a reduced screen:
Maybe we should make token icons clickable (and add navigation links to blockexplorer), and more visible (when several tokens are displayed in the column), WDYT? At least, it would be great to add a tooltip with token names to icons, due to sometimes token icons may be missing/incorrect/etc. So users may be confused with these views (see images below):

@ramirotw

Lets hide for now the ETH cost column as we don't have that value from the subgraph

Just want to clarify it with you whether we really need to hide it as it was a request from Anxo in cowprotocol/cowswap#3721 (case 3). Or it is not going to be merged to Subgraph in the near future?

Thanks!

ramirotw · 2022-10-06T11:43:51Z

Just want to clarify it with you whether we really need to hide it as it was a request from Anxo in #156 (comment) (case 3). Or it is not going to be merged to Subgraph in the near future?

@GabrielCamba added a txCostNative field in the Settlement entity but currently that field is not the tx cost in eth (gas price * gas used) but just the gas price, so he needs to update that and re-deploy. Also, that field is only in the staging environment, you can it check here.

ramirotw · 2022-10-06T11:57:49Z

4. Maybe we should make token icons clickable (and add navigation links to blockexplorer), and more visible (when several tokens are displayed in the column), WDYT? At least, it would be great to add a tooltip with token names to icons, due to sometimes token icons may be missing/incorrect/etc. So users may be confused with these views (see images below):

agreed:

the tooltip should have the token symbol
the image should have a link to etherscan
why is there only 1 token displayed in many settlements? the minimum should be 2

* Bumping cow-sdk to latest * Removed files already incorporated into the SDK * Removed rinkeby from currently used spots * Updated network selector * removed it from storybook stories * Removed/edited from the legacy code. Should remove it all eventually * Fixed unit test

elena-zh · 2022-10-07T11:09:18Z

Hey @matextrem , nice changes!
I will list here what is left, and new issues I have found:

Still, navigation to TX details is Explorer is not fixed (should be in the same tab)
I still see only 100 Settlements in GC and Mainnet
When hover a mouse on a native token (ETH/XDAI), 'Unknown' is displayed in the tooltip
'+1' may be displayed when there are many tokens in a settlement. I would show what tokens are included into '+1' in the tooltip instead of nothing
Could we make token icons look nicer? I see some kind of curved boarders on tokens, some kind of a white border or so

Thanks!

elena-zh · 2022-10-18T14:10:31Z

Hey @matextrem , great job!
Some remaining issues:

MKR token is shown as 'unknown' token for the Mainnet
Now I see that the amount of settlements is 1000 in GC and Mainnet. Is this OK?
When BE request fails to load the table, we show 'No results found' message. Maybe we should show a bit different error something instead with 'Try again' or reload the page button? @ramirotw , WDYT?

Thanks!

matextrem · 2022-10-18T14:13:28Z

Regarding the first comment, the subgraph is responding with empty symbol for MKR token. @ramirotw any suggestions?

alfetopito

The query to fetch dune info is not used in the first tab, resulting in missing solver names, where on the second they are filled

alfetopito · 2022-10-18T16:14:28Z

src/apps/explorer/pages/Solver/index.tsx

-    setQuery('')
-    setPageOffset(0)
-  }, [networkId, setPageOffset, setQuery])
+  }, [networkId, setPageOffset, setQuery, tabViewSelected])


Is the useEffect intentionally denpendent on tabViewSelected although it's not used in it?

This is intentional since we need to reset the search bar query when changing between tabs

Cool! Can you add a comment to be explicit about that and avoid the next reviewer to ask the same question 😆

alfetopito

Looking great Mati!

Only one last thing that I noticed now, but I rather it be addressed in another PR as this one is already quite big.

The table is "jumping" when I move between tabs, likely because it triggers a reload on every change:

Screen.Recording.2022-10-19.at.18.08.56.mov

So I'm in favor of merging and addressing the pending stuff in different PRs

elena-zh · 2022-10-20T10:59:35Z

Hey @matextrem ,

changes LGTM besides know issues related to the Graph:

MKR token is shown as 'unknown' token for the Mainnet
the amount of settlements is 1000 in GC and Mainnet.
total settlements is 100 on the Active solvers tab

Besides, I have created separate issues to address them later:

Show a better message for Solvers tables when Graph API request fails/user is offline Show a better message for Solvers tables when Graph API request fails/user is offline #239
Navigate to the correct page selection when go back to the Settlements page from Tx details Navigate to the correct page selection when go back to the Settlements page from Tx details #238
Solvers: The table is "jumping" when move between tabs Solvers: The table is "jumping" when move between tabs #240

The only one issues that I would like you to fix is that some tokens (GNO/MIR) are too dark on the table, while the icons look good on Tx details pages:

Could you please fix it?

Thanks!

alfetopito · 2022-10-20T11:29:51Z

Specifically regarding this point

MKR token is shown as 'unknown' token for the Mainnet
Regarding the first comment, the subgraph is responding with empty symbol for MKR token

I suspect I know the reason.

Maker and and another token (I think DAI?) have the name stored as bytes32 rather than as string, which can be the root cause for this.

When fetching the token name from the contract, we are querying both versions of the contract to be safe.

Since this empty string is coming from the Subgraph, I suggest to:

Try to look it up in the local list of tokens - maybe it's already loaded
Follow the same loading step as other pages - try to get it from the contract

ramirotw

The Search by solver, address or name placeholder in the table header should be updated. Right now it's filtering by solver name and tx hash, it would also be helpful to filter by solver address so I don't need to go to the Active solvers tab to know the name based on the address and then come back and use that name to get the latest settlements by solver.

elena-zh · 2022-10-21T09:11:38Z

Hey @matextrem , icons LGTM!
As for the MKR token, it is still unknown

Also, AFAIU from @ramirotw comment, it would be great to have a possibility to search by a solver address in the Settlements table, but you removed this option from the placeholder and, of cause, the search by a solver address is not working there.

matextrem · 2022-10-21T17:46:41Z

@elena-zh updated!

MKR problem solved and search by address is working now.

elena-zh · 2022-10-24T10:31:05Z

Hey @matextrem , changes LGTM!
The last nitpick is to change the placeholder in the search field to 'Search by tx hash, solver name or address'

Thanks

matextrem · 2022-10-25T01:07:48Z

@elena-zh Updated!

elena-zh

Hey @matextrem , great!
I'm approving this PR in order to merge this.
Still need to resolve some issues that are related to Subgraph.
Also, I have created a low-priority issue #242 to display the tooltip in the search field better on reduced screens.

Thank you!

ramirotw

👌

matextrem added 5 commits September 29, 2022 23:52

Add UI for active solvers table

c6a3fa4

Refactor Active solvers page

7fa0fb4

Update feedback

fca3aee

Fix pagination styles

68933f7

Refactor code + update feedback

9b3aae8

matextrem added app:Explorer Explorer App Protofire labels Oct 5, 2022

matextrem requested review from alfetopito, alongoni, ramirotw and elena-zh October 5, 2022 23:28

matextrem self-assigned this Oct 5, 2022

ramirotw suggested changes Oct 6, 2022

View reviewed changes

elena-zh mentioned this pull request Oct 6, 2022

Do not navigate to Home page when switch network from Solvers page #230

Closed

matextrem requested a review from ramirotw October 7, 2022 02:23

matextrem changed the base branch from develop to solvers October 7, 2022 02:57

matextrem force-pushed the 224/settlements-table branch 3 times, most recently from e1039cd to 5bad0b6 Compare October 7, 2022 04:17

alfetopito and others added 6 commits October 7, 2022 01:18

Refactor useTable

ee6e059

Added logic for settlements table

64f41a9

Add TokensVisualizer component

8927192

Improve styles

354af62

Update feedback

3e6ddac

matextrem force-pushed the 224/settlements-table branch from 5bad0b6 to 0524353 Compare October 7, 2022 04:31

matextrem and others added 2 commits October 17, 2022 16:24

Merge branch 'solvers' into 224/settlements-table

a672394

Remove empty lines

de73a46

alfetopito reviewed Oct 18, 2022

View reviewed changes

Add solversInfo for active solvers

21d435a

matextrem requested a review from alfetopito October 19, 2022 15:56

alfetopito approved these changes Oct 19, 2022

View reviewed changes

ramirotw suggested changes Oct 20, 2022

View reviewed changes

Search in tokenList for empty symbol tokens

a8cf619

matextrem requested a review from ramirotw October 20, 2022 16:02

Fix token unknown issue + search by address

ceb70c6

matextrem mentioned this pull request Oct 21, 2022

236/solvers details #241

Merged

Update placeholder

3f559ab

elena-zh mentioned this pull request Oct 25, 2022

Solvers: placeholder is truncated in the search field in reduced screens #242

Closed

elena-zh approved these changes Oct 25, 2022

View reviewed changes

ramirotw approved these changes Oct 25, 2022

View reviewed changes

matextrem merged commit 60c8fef into solvers Oct 25, 2022

matextrem deleted the 224/settlements-table branch October 25, 2022 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Solvers] Settlements Table #235

[Solvers] Settlements Table #235

matextrem commented Oct 5, 2022

coveralls commented Oct 5, 2022 •

edited

Loading

github-actions bot commented Oct 5, 2022

ramirotw left a comment

elena-zh commented Oct 6, 2022

ramirotw commented Oct 6, 2022

ramirotw commented Oct 6, 2022

elena-zh commented Oct 7, 2022

elena-zh commented Oct 18, 2022

matextrem commented Oct 18, 2022

alfetopito left a comment

alfetopito Oct 18, 2022

matextrem Oct 19, 2022

alfetopito Oct 19, 2022

alfetopito left a comment

elena-zh commented Oct 20, 2022

alfetopito commented Oct 20, 2022

ramirotw left a comment

elena-zh commented Oct 21, 2022

matextrem commented Oct 21, 2022

elena-zh commented Oct 24, 2022

matextrem commented Oct 25, 2022

elena-zh left a comment

ramirotw left a comment

[Solvers] Settlements Table #235

[Solvers] Settlements Table #235

Conversation

matextrem commented Oct 5, 2022

Summary

To Test

coveralls commented Oct 5, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3201733554

💛 - Coveralls

github-actions bot commented Oct 5, 2022

ramirotw left a comment

Choose a reason for hiding this comment

elena-zh commented Oct 6, 2022

ramirotw commented Oct 6, 2022

ramirotw commented Oct 6, 2022

elena-zh commented Oct 7, 2022

elena-zh commented Oct 18, 2022

matextrem commented Oct 18, 2022

alfetopito left a comment

Choose a reason for hiding this comment

alfetopito Oct 18, 2022

Choose a reason for hiding this comment

matextrem Oct 19, 2022

Choose a reason for hiding this comment

alfetopito Oct 19, 2022

Choose a reason for hiding this comment

alfetopito left a comment

Choose a reason for hiding this comment

elena-zh commented Oct 20, 2022

alfetopito commented Oct 20, 2022

ramirotw left a comment

Choose a reason for hiding this comment

elena-zh commented Oct 21, 2022

matextrem commented Oct 21, 2022

elena-zh commented Oct 24, 2022

matextrem commented Oct 25, 2022

elena-zh left a comment

Choose a reason for hiding this comment

ramirotw left a comment

Choose a reason for hiding this comment

coveralls commented Oct 5, 2022 •

edited

Loading