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: simplify util types, add attest unit/bench examples #210

Merged
merged 8 commits into from
Jan 17, 2024

Conversation

ssalbdivad
Copy link
Contributor

@ssalbdivad ssalbdivad commented Nov 14, 2023

As discussed with @tmm, I took a look at AbiType's abi.ts and utils.ts modules. I also skimmed human-readable but can potentially delve in further in a future PR or during a pair programming session with a maintainer.

Overall, I was very impressed the quality of the types. There's a few core principles that could be helpful for simplifying some of the more complex types. I hope based on these changes and the comments I will add summarizing them, @tmm or someone similarly proficient with the type system would be able to apply these ideas throughout the code base.

From my initial testing, the types are already quite performant, so the main benefits would be readability, maintainability, and potentially optimized external static error messages.

Although I hadn't initially planned on it, I was looking for an external package to test attest integration and AbiType felt like a perfect fit.

I added an example of how types can be tested and/or benchmarked. Given the volume of your type-level tests, I think converting others as well could be very beneficial. The benchmarks could also be nice as a way to ensure there are no significant regressions as you make changes to your types, especially if you plan to apply some of the strategies I've highlighted across your repo!

I didn't have any specific changes I wanted to make in abi.ts so I just wanted to add some notes here:

abi.ts:

  • Types look very clean and should be very performant
  • Little optimization possible since no parsing is required.
  • Could consider changing a type like:
export type TypedData = Pretty<
  Record<string, readonly TypedDataParameter[]> & {
    // Disallow `TypedDataType` as key names (e.g. `address`)
    [_ in TypedDataType]?: never
  }
>

to leverage string error messages if you feel it would increase clarity at all for your users, e.g.:

export type TypedData = Pretty<
  Record<string, readonly TypedDataParameter[]> & {
    // Disallow `TypedDataType` as key names (e.g. `address`)
    [_ in TypedDataType]?: `'${_}' is not allowed as a key on TypedData`
  }
>

There's very little downside to something like this, since even if the user typed that exact message it would not satisfy the type due to the array intersection. However, the benefit is marginal depending on how clear it is contextually to users that the origin of the never is that the name of the key.

abi.test-d.ts

Very thorough, although honestly I'd say it's overkill to explicitly test non-generic types. Totally up to you and at worst it's a bit of unnecessary overhead, but given a type like:

export type MBits =
  | ''  | 8   | 16  | 24  | 32  | 40  | 48  | 56  | 64  | 72
  | 80  | 88  | 96  | 104 | 112 | 120 | 128 | 136 | 144 | 152
  | 160 | 168 | 176 | 184 | 192 | 200 | 208 | 216 | 224 | 232
  | 240 | 248 | 256

export type SolidityInt = `${'u' | ''}int${MBits}` 

The type itself is about as close to a spec as you can get- there's no real functionality to test. That said, again it's somewhat subjective, so feel free to disregard if you preferring having them.

Note: I enjoy that the PR summary doesn't check the changes in utils.ts because there are too many, then says the PR is mostly about attest😅


PR-Codex overview

This PR focuses on adding a new testing library called "@arktype/attest" and making related changes to the codebase.

Detailed summary

  • Added "@arktype/attest" as a dev dependency
  • Updated the global setup file for testing
  • Added type-testing code using "@arktype/attest"
  • Added a new script "typeperf" for type checking with extended diagnostics
  • Updated the package.json file with new dependencies and scripts

The following files were skipped due to too many changes: packages/abitype/src/utils.ts

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

Copy link

changeset-bot bot commented Nov 14, 2023

⚠️ No Changeset found

Latest commit: 0002c92

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 Nov 14, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
abitype ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 17, 2024 6:09pm

Copy link

commit-lint bot commented Nov 14, 2023

Contributors

ssalbdivad

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

  • @Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
  • @Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
  • @Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
  • @Commit-Lint merge disable will desactivate merge dependabot PR
  • @Commit-Lint review will approve dependabot PR
  • @Commit-Lint stop review will stop approve dependabot PR

packages/abitype/package.json Show resolved Hide resolved
packages/abitype/package.json Show resolved Hide resolved
packages/abitype/test/globalSetup.ts Show resolved Hide resolved
packages/abitype/src/utils.ts Show resolved Hide resolved
@@ -0,0 +1,34 @@
import { bench } from '@arktype/attest'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This gives you an idea of the general structure for a set of attest benchmarks.

It will allow you to chain .types() off the bench call. You just run the file as normal using something like ts-node, tsx etc. (hopefully bun but they may still have some issues with source maps).

When it executes for the first time or with no snapped value, it will populate the inline snapshot with a summary of the instantiation count.

Subsequent runs will summarize the delta between the current result and the snapped one.

If you want to configure it to fail above a certain threshold e.g. for CI, there are two settings you can pass through the CLI:

--benchPercentThreshold (defaults to 20)
--benchErrorOnThresholdExceeded (defaults to false)

If you set --benchErrorOnThresholdExceeded to true like tsx src/utils.bench.ts ----benchErrorOnThresholdExceeded, the process should return a non-zero exit code if any of the benchmarks have increased by an amount more than --benchPercentThreshold.

Note those flags haven't really been tested extensively, but figured I'd mentioned then since @tmm expressed an interest in CI integration.

packages/abitype/src/utils.ts Show resolved Hide resolved
packages/abitype/src/utils.ts Show resolved Hide resolved
import { test } from 'vitest'
import type { TypedDataToPrimitiveTypes } from './utils.js'

test('self-referencing', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This syntax closely parallels the type assertions vitest allows, with the benefit of being able to run them conditionally based on your environment since the assertion is still executed as normal at runtime.

Another significant benefit is that you can write these assertions alongside or in combination with your value assertions, so you don't need to have separate .d.ts tests.

This can be especially helpful when testing types and values that parallel one another, e.g. via APIs like .throwsAndHasTypeError

It also allows type/value snapshotting, object literal snapshots etc. (see the admittedly underdocumented README).

? []
: // Compare the original set of names to a "validated"
// set where each name is coerced to a string and undefined|"" are excluded
Components[number]['name'] extends Exclude<
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a replacement for _HasUnnamedAbiParameter. It leverages how unions compare to one another to ensure that after the named parameters are extracted, it remains unchanged.

I wasn't sure how you wanted to deal with someone specifying a top-level string, but that could be disallowed as a special case.

packages/abitype/src/utils.ts Show resolved Hide resolved
@Raiden1411
Copy link
Collaborator

This look very good. Was excited for the release of attest and glad to see it here. Happy to help in any way that I can

Copy link
Member

@tmm tmm left a comment

Choose a reason for hiding this comment

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

ty! this is all great stuff. making some small style updates in another PR.

@tmm tmm merged commit fbf2152 into wevm:main Jan 17, 2024
5 of 13 checks passed
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.

3 participants