-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Bug fix: Fix retry mechanism for fetching the Solidity compiler #5388
Conversation
How should this be smoke tested? |
If you want to smoke test it then maybe you could put typos in the code for the |
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. |
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.
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 )
packages/compile-solidity/src/compilerSupplier/loadingStrategies/VersionRange.ts
Outdated
Show resolved
Hide resolved
packages/compile-solidity/src/compilerSupplier/loadingStrategies/VersionRange.ts
Outdated
Show resolved
Hide resolved
packages/compile-solidity/src/compilerSupplier/loadingStrategies/VersionRange.ts
Outdated
Show resolved
Hide resolved
packages/compile-solidity/src/compilerSupplier/loadingStrategies/VersionRange.ts
Outdated
Show resolved
Hide resolved
packages/compile-solidity/src/compilerSupplier/loadingStrategies/VersionRange.ts
Outdated
Show resolved
Hide resolved
packages/compile-solidity/src/compilerSupplier/loadingStrategies/VersionRange.ts
Outdated
Show resolved
Hide resolved
Internal improvement: Add more types in compile-solidity
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.
OK, seems good to me! One minor style comment but you could also just ignore that. :P
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. |
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. |
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.