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

Bug fix: Fix retry mechanism for fetching the Solidity compiler #5388

Merged
merged 20 commits into from
Aug 8, 2022

Conversation

eggplantzzz
Copy link
Contributor

@eggplantzzz eggplantzzz commented Aug 3, 2022

Currently Truffle has an issue with how it downloads the Solidity compiler. It currently has 2 sources (technically there are 3 url roots that Truffle uses but the first is a Truffle link that redirects to the second) that it fetches the compiler from. First, it fetches a list of the Solidity compilers available from the first source that successfully returns a list. After it has this list, it extracts the filename for the one it wants and sends it over to a function that makes the request. Here is where there is a bit of a problem.

Once this function receives the filename, it makes the request to the first source. If it fails, then it retries using the 2nd and 3rd url (if the 2nd fails). However, this could be a problem because different sources have different compiler versions available and the urls also may be different (for example, wasm vs. asm builds).

So this PR attempts to put these two steps together so that first a list is downloaded from a source. Once it gets the filename from the list it can request from the same source. If this fails, then it will retry starting with fetching the list from the next source, and so on. This should eliminate cases where there are discrepancies between what sources have available.

@cds-amal
Copy link
Member

cds-amal commented Aug 4, 2022

How should this be smoke tested?

@eggplantzzz
Copy link
Contributor Author

If you want to smoke test it then maybe you could put typos in the code for the compilerRoots urls to ensure that the later ones get used?

@haltman-at
Copy link
Contributor

Btw, may not be worth fixing in this PR, but probably worth taking note of issue #4261 as long as you're looking at this stuff.

Copy link
Contributor

@haltman-at haltman-at left a comment

Choose a reason for hiding this comment

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

Thanks Tyler! However I noticed a bug in getSolcFromCacheOrUrl, so I'm hitting request changes. I also have some comments on the style of the rest, but that's less important, obviously!

(Also see my comment above about #4261. :P )

Copy link
Contributor

@haltman-at haltman-at left a comment

Choose a reason for hiding this comment

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

OK, seems good to me! One minor style comment but you could also just ignore that. :P

@haltman-at
Copy link
Contributor

Also now that I've thought about this more... maybe #4261 should be closed? I think I misunderstood the logic earlier -- I think I put up that issue because it was just retrying. But actually it's retrying from a different URL! So maybe we don't want to use a retry library after all, since that's more for retrying the same operation? Maybe the way it's done here is just fine.

@eggplantzzz
Copy link
Contributor Author

Yeah you are probably right about closing that issue. It seems necessary since we definitely need to do something a bit more complex than just retrying the same URL.

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.

3 participants