-
Notifications
You must be signed in to change notification settings - Fork 8
Conversation
|
Hey @matextrem , nice changes!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the notes above
hey @elena-zh I addressed some of the bugs you shared. Regarding 1 & 3 it needs to be changed. I left it is since we need to discuss what to put in there. maybe @alongoni could help us in it.
|
@matextrem @elena-zh This is because that solver was deactivated but the subgraph currently is not handling the toggle of the isSolver flag to false. There's a pending PR in cowprotocol/subgraph#65 to fix that. To validate whether a mainnet solver is active or not using onchain data:
|
@matextrem this issue is related to #241 (comment) |
That is ridiculous :) |
Hey @matextrem , thanks for the explanation.
Thank you! |
Indeed it's ridiculous, but this is for internal testing before @GabrielCamba deploys the changes to fix it. |
Request for a follow up PR: Update the overall search so when a solver address is searched it leads to the solver details page. For example. Searching on https://pr241--explorer.review.gnosisdev.com/gc for |
Hey @alfetopito , I have created #244 task for this |
Hey @matextrem , cases 1, 2, 4, 5 from this comment are still not fixed. Besides, I have another question: since the number of trades is limited with 1000 now, should we show this card on the UI? In most of cases it will always show 1K until we implement a solution how to show more data |
FYI the Total Settlements number will be updated once the subgraph update is deployed, hopefully by the EOW. |
06cc8c2
to
8c4b956
Compare
Hey @elena-zh! I updated the branch with the feedback. Regarding point 2, It's working for me. Regarding 5, I don't fully understand what's the behavior you want here. They are vertically centered on Tablet/Mobile view : Maybe @alongoni could give feedback on this as well. |
Hey @matextrem , overall, changes look good to me. But:
Please, take a look at the Settlements tab. Active solvers tab looks good. Thanks! |
@elena-zh updated! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @matextrem , changes LGTM now.
We still have issues related to the Supgraph:
- Total settlements are 1000 for a single solver, and on Settlements table
- No ETH cost
- Do 'Active since'
Also, I have created a separate task to adjust pagination displaying on tables across the whole app #246.
Thanks
Summary
Closes #236
Added Solver details page.
To Test
solvers
Disclaimer: There are some info left we have to update such as ETH cost and Active since box (it is harcoded right now)