-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deployment debug tab on add order #887
base: main
Are you sure you want to change the base?
Conversation
So @findolor this looks great on the happy path but i found a couple of issues:
Buuuut, only some pairs may have an error. For example this one should only show an error for one of the pairs, but an error for any pair means the whole call fails. The other thing is we need to show partial traces, so if there is an error we still need the trace up until the error, as we do with quotes. |
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 comments for manual testing feedback
@findolor i wonder if it would be good to use tanstack for this. at the moment if you go to another tab, then back to the debug tab you have to wait for it to load again, even if you haven't changed any of the text. using tanstack would get us caching so it would load instantly. |
@findolor we're still missing partial stacks - if you get a revert for some quote you can still load up whatever stack was produced before the revert. You can see how i implemented that, i thought it would be the same here. So for example, in the below image i'd expect to be able to load up the stack and see 14, because that was produced before the revert. |
@findolor can we have the block paused by default? when the block is constantly updating it has to get a new fork every time which makes it quite slow to produce the debug. but if it's paused it's much faster (same fork every time so it's all local). |
@findolor also - i know we didn't want to upgrade anything to do with charting in this PR, but let's at least rename the "Run all scenarios" button to "Generate charts" |
Closes #867
Motivation
See issue: #867
Solution
Checks
By submitting this for review, I'm confirming I've done the following: