-
-
Notifications
You must be signed in to change notification settings - Fork 901
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
feat: Improve return type for toSignature #2928
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: ca1ca16 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@SebastienGllmt is attempting to deploy a commit to the Wevm Team on Vercel. A member of the Team first needs to authorize it. |
import { BaseError } from '../../errors/base.js' | ||
import type { ErrorType } from '../../errors/utils.js' | ||
|
||
type NormalizeSignatureParameters = string | ||
type NormalizeSignatureReturnType = string | ||
/** |
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.
There are multiple TODOs here and I'm not sure which one of these would have to be fixed to be considered good enough to merge (given that any type is better than no type, but the wrong type is also worse than no type)
I wanted to make the original commit for this PR contain as detailed information before we start truncating out cases to support the subset that makes sense
Here are the 4 options in increase level of work required (each option requires the previous option to be completed):
Option 1: we can make that the toSignature
function only returns a type for AbiFunction | AbiEvent
(since those cases are always correct) and then just return string
otherwise (instead of trying to parse the string)
Option 2: we make string
inputs work in some cases, but not all. This would require:
a. fixing the normalization of uint
to uint256
(this is a bug in normalizeSignature
and not in the type)
b. arguably, fixing the tuple(...)
notation handling (but if the JS code is also wrong, no point in making the type match it)
Option 3: we try to make all valid string
inputs work, but no guarantee on invalid inputs. This would require:
a. fixing tuple(uint)
handling (maybe this is an abitype concern? However, this syntax isn't part of their human-readable string encoding)
b. adding proper trimming logic on whitespace
Option 4: make all string
inputs work, and properly return never
on invalid input
AbiEvent, | ||
AbiFunction, | ||
ParseAbiItem, | ||
SignatureAbiItem, |
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.
note: this type (SignatureAbiItem
) depends on wevm/abitype#255 getting merged and released
5c14479
to
373eafa
Compare
Anything at all I can do to help move this forward? |
Let's use @jxom and I are going to do a larger refactor in the new year that moves a lot of Viem's internals over to Ox, but can proxy from Ox for now. |
Just to make sure we're clear, |
Integrates wevm/abitype#255 into Viem to improve the return type of
toSignature
This helps resolve these two discussions: #260 and #2706
A follow-up PR to this will have to be adding the option to pass the full signature to functions like
getContractEvents
andgetAbiItem
PR-Codex overview
This PR focuses on improving the type safety and return types of the
toSignature
andnormalizeSignature
functions in the codebase, enhancing the handling of ABI signatures.Detailed summary
toSignature
to use generics for better return types.normalizeSignature
with stricter type definitions.