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

refactor: add explicit annotations to SimulateContractReturnType #2557

Merged
merged 6 commits into from
Aug 6, 2024

Conversation

ssalbdivad
Copy link
Contributor

@ssalbdivad ssalbdivad commented Jul 29, 2024

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

  • Updated @ark/attest to ^0.10.2
  • Refactored type definitions in simulateContract.ts
  • Updated various dependencies versions

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}

Copy link

changeset-bot bot commented Jul 29, 2024

⚠️ No Changeset found

Latest commit: d6b2367

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

Copy link

vercel bot commented Jul 29, 2024

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

A member of the Team first needs to authorize it.

Copy link

socket-security bot commented Jul 29, 2024

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

Package New capabilities Transitives Size Publisher

🚮 Removed packages: npm/@arktype/[email protected]

View full report↗︎

@@ -84,33 +84,34 @@ export type SimulateContractParameters<
>

export type SimulateContractReturnType<
abi extends Abi | readonly unknown[] = Abi,
functionName extends ContractFunctionName<
out abi extends Abi | readonly unknown[] = Abi,
Copy link
Member

Choose a reason for hiding this comment

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

Never knew about this syntax! Where can I find the docs?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, not adding anything meaningful to the PR, but I am also only learning of this now and this is really cool. Cool to see more nuanced use of typescript and the performance comparison is really interesting too. How did you generate that graph for tsc build btw?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Yuripetusko There's a command for it in the attest CLI.

It's a thin wrapper around tsc --generateTrace + https://github.com/microsoft/typescript-analyze-trace

@ssalbdivad
Copy link
Contributor Author

@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.

Copy link

vercel bot commented Aug 6, 2024

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.

@tmm tmm merged commit 8cc83cd into wevm:main Aug 6, 2024
1 check failed
@tmm tmm mentioned this pull request Dec 21, 2024
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.

4 participants