wevm / viem

TypeScript Interface for Ethereum
https://viem.sh
Other
2.51k stars 797 forks source link

refactor: add explicit annotations to SimulateContractReturnType #2557

Closed ssalbdivad closed 2 months ago

ssalbdivad commented 2 months ago

This PR adds explicit variance annotations to SimulateContractReturnType based on some type performance investigation done for Mud as documented here.

In this case, avoiding the variance checks reduced the check time for a simple repo by over a second locally.

I based the annotations on those that were inferred from the TS compiler as documented in the trace here (from the Mud PR):

image

Unfortunately, it is impossible to annotate a bivariant relationship directly, and treating chain as covariant required a // @ts-expect-error comment (I use this strategy somewhat often to cast variance in arktype, so no significant cause for concern).

I don't completely understand all the contexts in which this type is used or how common it is to compare multiple instances of SimulateContractReturnType that would cause these variance annotations to matter, so if you find you need to make adjustments to the annotations (e.g. by labeling more parameters as out only with error comments), please feel free to do so.

It is likely if you are interested in optimizing perf, there are more scenarios like this that exist that you could find via tracing where explicit annotations could result in significant downstream perf improvements.

I also took the opportunity to update @arktype/attest to the latest version under its new primary alias, @ark/attest.

Let me know if you have any questions or if there is anything I can chance!


PR-Codex overview

This PR updates dependencies related to testing and documentation tools. It also refactors type definitions for better clarity.

Detailed summary

The following files were skipped due to too many changes: pnpm-lock.yaml

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

changeset-bot[bot] commented 2 months ago

⚠️ No Changeset found

Latest commit: d6b2367268691a669d01068ac636ee7a973d9648

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

vercel[bot] commented 2 months ago

@ssalbdivad is attempting to deploy a commit to the Wevm Team on Vercel.

A member of the Team first needs to authorize it.

socket-security[bot] commented 2 months ago

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

Package New capabilities Transitives Size Publisher

🚮 Removed packages: npm/@arktype/attest@0.8.0

View full report↗︎

ssalbdivad commented 2 months ago

@tmm @jxom Is there any other information I can provide to get this merged?

It made such a big difference for @latticexyz and I'd be it could have a significant impact more broadly as well.

vercel[bot] commented 2 months ago

Deployment failed with the following error:

The provided GitHub repository does not contain the requested branch or commit reference. Please ensure the repository is not empty.