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

add new route in preparation for visual debugger PR #6075

Merged
merged 2 commits into from Jun 8, 2023
Merged

Conversation

eggplantzzz
Copy link
Contributor

This PR adds a route in preparation for getting #6058 merged.

The visual debugger will be fetching verified sources externally in the case that it doesn't have compilations for some of the contracts the tx touches. In order to implement this, this route is added. It allows dashboard to give the server a list of addresses and a network to use to attempt to fetch the source material. This route fetches what it can, compiles everything, and then returns it to dashboard.

Comment on lines 106 to 114
let config;
try {
config = Config.detect();
} catch (error) {
const notFound = "Could not find suitable configuration file.";
if (!error.message.includes(notFound)) {
throw error;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should throw? Since this config isn't actually used for fetch and compile, only to grab the etherscan key if it's there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I don't know. The point here is the same with the other try/catch. If we can't find a config, we want to move on and not care. However, if it throws in some unexpected way we want to know about it. In this case though it might not matter...maybe we should just leave the catch empty? Do we want to know when it fails for some other reason than not being able to find it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do nothing/log, and then move on to fetch and compile seems reasonable to me. But I understand your point with this and the try/catch block below though. If in react we also wrap the request in try/catch then my concerns are not important here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, in this case I think it makes sense to just ignore errors with Config.detect - the other one below, instead of throwing should probably 500 like you said

Comment on lines +137 to +141
} catch (error) {
if (!error.message.includes("No verified sources")) {
throw error;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also don't throw if no sources? It can be handled like how !result is handled below with res.json({ compilations: [] })

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of this is that fetchAndCompile throws with that error message in the case that it can't find verified sources. We want to not throw in that case, but rather throw when it fails in some other unexpected way. Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I was just thinking it'd be less work to write a case to receive a 500 in react? Maybe we can console.error it, which should show in the Dashboard terminal, and still send the empty compilations response?

@eggplantzzz eggplantzzz merged commit 7fa3b70 into develop Jun 8, 2023
10 checks passed
@eggplantzzz eggplantzzz deleted the new-route branch June 8, 2023 16:29
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.

None yet

3 participants