Skip to content
This repository was archived by the owner on Feb 2, 2024. It is now read-only.

fix: fix graph for settlement contract trades #492

Merged
merged 4 commits into from
May 29, 2023

Conversation

alfetopito
Copy link
Collaborator

Summary

Closes #491

Fixing graph visualization for settlements where the settlement contract is also a trader
Screenshot 2023-05-26 at 16 02 16

Also adding error handling so if the graph ever fails, we'll not crash the app and show an error msg instead:
Screenshot 2023-05-26 at 15 56 27

To Test

  1. Open this trade 0xd78b614d0d5c39d55c516653e8674b73133d2c9bbdf302e5be036de7c6b304e6
  2. Check the graph visualization
  • It should not crash the app
  • It should show trades leading from COW to itself

@alfetopito alfetopito self-assigned this May 26, 2023
@vercel
Copy link

vercel bot commented May 26, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
explorer-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback

📖 Storybook ↗︎

@github-actions
Copy link

github-actions bot commented May 26, 2023

Pull Request Test Coverage Report for Build 5111096953

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 61.585%

Totals Coverage Status
Change from base Build 5055252700: 0.0%
Covered Lines: 872
Relevant Lines: 1231

💛 - Coveralls

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.

Hey @alfetopito , great!
The app is not crashed. The graph sometimes can be hard to read, but a user can zoom in and out it, and change a view.

Also, was not able to test a 'failed to lead' scenario. I tried blocking requests URLs and switched the internet off, it did not help me to reproduce the case.

Thanks

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.

As we discussed earlier, i think this graph will be way more visible if we create another node for the settlement.

This way, we can see a difference between the "CoW Protocol: Settlement" (or "CoW Protocol: Batch" --> we could brainstorm on names) and just "CoW Protocol" for this contract acting as a trader.

This way, we can see how it sends shitcoins to the "CoW Protocol: Settlement" and receive WETH

If we use the same node, its hard to visualise.

SOFT APPROVING, just because I think this PR fixes the issues, and this can be re-iterated

@alfetopito
Copy link
Collaborator Author

I will merge as this fix the app crashing and continue working on improving the display in another PR

@alfetopito alfetopito merged commit 8798c09 into develop May 29, 2023
@alfetopito alfetopito deleted the 491/fix-graph-for-settlement-contract-trades branch May 29, 2023 10:46
@tukantje tukantje mentioned this pull request Jun 1, 2023
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.

Explorer crashes when open a TX details
4 participants