Skip to content
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

Update undici dependency #948

Merged
merged 3 commits into from
Dec 19, 2023
Merged

Update undici dependency #948

merged 3 commits into from
Dec 19, 2023

Conversation

ericglau
Copy link
Member

@ericglau ericglau commented Dec 19, 2023

response.body.json(); previously returned any, but now returns unknown in newer version of undici. We should cast it to the expected type.

Copy link

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
undici 5.28.2 environment +1 1.25 MB matteo.collina

@ericglau ericglau changed the title Update unidici dependency Update undici dependency Dec 19, 2023
@ericglau ericglau enabled auto-merge (squash) December 19, 2023 20:42
Copy link
Member

@ernestognw ernestognw left a comment

Choose a reason for hiding this comment

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

LGTM with some comments:

I'm not convinced about the as <Type> casts. Following these recommendations, we should create functions for checking types, for example:

function isEtherscanResponseBody(input: any): json is EtherscanResponseBody {
  return 'status' in input;
} 

However, I also don't think we should accumulate these type-checking functions all over the place. The current state is good to me but mentioning in case you agree with "better" type-checking @ericglau

@ericglau ericglau merged commit 11e37eb into OpenZeppelin:master Dec 19, 2023
11 of 12 checks passed
@ericglau ericglau deleted the undici branch December 19, 2023 20:59
@ericglau
Copy link
Member Author

@ernestognw Agree it would be good to improve type checking, but perhaps that can be left until/unless there are changes in Etherscan's API or issues with the current approach. I don't a large benefit in adding checking at the moment, other than to give a better error message, but if errors occur then the issue might be quite apparent.

Note that Hardhat currently does a cast as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants